[F97] feat: keep speculative ops in pool, but hard skip them - #5233
Conversation
|
@damip can you take a quick look and see if it's consistant with what you wanted for the reexecution followup (see the main PR comment)? |
damip
left a comment
There was a problem hiding this comment.
Good direction, two things to change before merge. Rule for both: block production must be fast and must never block on anything. Dropping or duplicating a few tx is always preferable to slowing down block production.
-
get_block_operationsmust not call execution. It runs in the factory thread, inside the pool read guard, right at slot time. The PR addsget_execution_statuses()there: oneget_ops_exec_statuson every pool op id (up to 600k) behindexecution_state.read(), while the execution worker holds the write lock for the whole ofexecute_candidate_slot/execute_final_slot/ read-only requests. Andactive_history.get_ops_exec_statusis O(pool x non-final slots).block_opt_channel_timeoutonly boundstry_read_foron the pool lock, not this. Today the factory path never touches execution (refresh runs on the buffer in the pool worker) and it must stay that way.
Fix: anexecuted: boolonOperationInfo, set inrefresh()from the statuses already fetched there, andget_block_operationsjust doescontinueon the flag. Zero cost on the production path, travels throughreplace_withfor free. Staleness is at most 5 s and the next block of the same thread is 16 s away, so no duplicate in practice. If one slips in, execution skips it. Fine. -
The balance filters undo the fix in the common case.
get_sender_balancesuses the candidate balance, which already contains the speculative execution. Alice has 10, sends an op spending 9: candidate is 1 after speculative execution, next refreshmax_spending <= 1fails inprefilter_ops, op evicted before any rollback.eliminate_balance_overflowsdoes the same when the sender has other pending ops (marked ops score last, so they are cut first). Only ops small relative to what is left survive. The test does not see it because the mock balance is 1e9.
Fix: skip both balance checks for ops with a live mark (not selectable anyway, their spend is already in the candidate balance). When the mark disappears the balance is back and the next refresh evaluates them normally. Add a test with a realistic balance: op spending most of it, speculative mark, rollback, selectable again.
@Leo-Besancon on your question: hard skip is right, keep it. Scoring down only would still put every speculatively executed op in the next 2-3 blocks of its thread until the including block is final, each copy ignored by execution, zero fee, pure waste for everyone. The 1/1000 factor is only useful as truncation priority when the pool is full, which is what you did.
Not consensus, node-local, no MIP: execution ignores re-included ops (IncludeOperationError("operation was executed previously"), errors do not interrupt the slot), block validity is unchanged. Close #5028 on merge.
resync_checkflagNote: In this version I hard skip speculative ops (and kept the 1/1000° that was there in commented code to drop them in priority if the pool is full). See more details in the discussion below