Repository navigation
mempool: verify scripts before evicting, and make eviction linear - #942
Merged
Merged
Conversation
satd counted sigops with rust-bitcoin 0.32's `Script::count_sigops` and `Transaction::total_sigop_cost`. In accurate mode those keep the last OP_1..OP_16 across later OP_CHECKSIG and OP_CHECKMULTISIG opcodes, where Core's `GetSigOpCount` reads the opcode immediately before each multisig opcode (src/script/script.cpp). An OP_CHECKMULTISIG that followed another signature opcode took the earlier number instead of 20, so P2SH redeem scripts and P2WSH witness scripts could count lower than Core counts them (`OP_1 OP_CHECKSIG OP_CHECKMULTISIG`: 2 instead of 21). This is rust-bitcoin issue #6368; no released rust-bitcoin version has the fix. Add `validation::sigops`, a port of Core's `GetSigOpCount(fAccurate)`, `GetSigOpCount(scriptSig)`, `CountWitnessSigOps`, `GetLegacySigOpCount`, `GetP2SHSigOpCount` and `GetTransactionSigOpCost`, with Core's script parsing, and Core's sigopcount_tests, script_p2sh_tests and transaction_tests vectors. Use it for the block limit in `connect_block` and `check_block`, the cost each mempool entry records (which the block template budgets by and getblocktemplate reports), the policy engine's `tx.sigops_cost`, and the Stratum coinbase bound. The submitpackage fee-bypass path recorded a sigop cost of 0 and no prevout metadata for the parents it admits, so the template counted them as free. It now records what `accept_transaction` records, through one shared helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A transaction arriving at a full pool evicted entries to make room for itself before its scripts were checked. Core checks the scripts (PolicyScriptChecks, ConsensusScriptChecks) before it adds the transaction and trims the pool (src/validation.cpp, AcceptSingleTransactionInternal), so a transaction whose scripts fail changes nothing. satd now does the same: the cheap fee-rate refusal stays ahead of the scripts, and eviction comes after them. Eviction itself rescanned the whole pool for every evicted entry and every descendant, checking each candidate against a Vec of what was already chosen. It now pops a heap built in one pass, finds descendants through the spends index, and tracks membership in a HashSet. Around the same path: - Eviction never takes the incoming transaction's own ancestors, which left the child in the pool spending an output nothing provided. The fee-rate bar is the cheapest entry it may actually evict. - Admission trims the pool down to its limit, as Core's TrimToSize does, instead of freeing only its own size. A lowered -maxmempool trims the pool at once on reload; before, every admission was refused until the pool drained. - A refusal after eviction reports the entries it evicted. - The specific script error behind a libbitcoinconsensus rejection is found by running the native engine over the failing input only, after the pool's write lock is released. - The orphanage's FIFO queue holds each live orphan once, keyed by an arrival sequence as Core's orphanage is, instead of keeping the txid of every orphan it ever held. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Eviction removes a held descendant of an evicted acting entry with it. That entry never emitted an Enter, so a LeaveEvicted for it is a Leave for a txid the standard stream never showed. The eviction now returns each entry's scope, and admission (success and refusal) and the -maxmempool reload trim emit LeaveEvicted only for entries that were acting. The test insert helpers that take a caller's transaction now index its spends, as every admission does, since eviction finds descendants through that index. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
bkeroack
changed the base branch from
fix/sigop-count-core-parity
to
release/0.6
October 8, 2026 18:11
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 14, 2026
bkeroack
added a commit
that referenced
this pull request
Oct 15, 2026
* Count sigops exactly as Bitcoin Core does satd counted sigops with rust-bitcoin 0.32's `Script::count_sigops` and `Transaction::total_sigop_cost`. In accurate mode those keep the last OP_1..OP_16 across later OP_CHECKSIG and OP_CHECKMULTISIG opcodes, where Core's `GetSigOpCount` reads the opcode immediately before each multisig opcode (src/script/script.cpp). An OP_CHECKMULTISIG that followed another signature opcode took the earlier number instead of 20, so P2SH redeem scripts and P2WSH witness scripts could count lower than Core counts them (`OP_1 OP_CHECKSIG OP_CHECKMULTISIG`: 2 instead of 21). This is rust-bitcoin issue #6368; no released rust-bitcoin version has the fix. Add `validation::sigops`, a port of Core's `GetSigOpCount(fAccurate)`, `GetSigOpCount(scriptSig)`, `CountWitnessSigOps`, `GetLegacySigOpCount`, `GetP2SHSigOpCount` and `GetTransactionSigOpCost`, with Core's script parsing, and Core's sigopcount_tests, script_p2sh_tests and transaction_tests vectors. Use it for the block limit in `connect_block` and `check_block`, the cost each mempool entry records (which the block template budgets by and getblocktemplate reports), the policy engine's `tx.sigops_cost`, and the Stratum coinbase bound. The submitpackage fee-bypass path recorded a sigop cost of 0 and no prevout metadata for the parents it admits, so the template counted them as free. It now records what `accept_transaction` records, through one shared helper. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * mempool: verify scripts before evicting, and make eviction linear A transaction arriving at a full pool evicted entries to make room for itself before its scripts were checked. Core checks the scripts (PolicyScriptChecks, ConsensusScriptChecks) before it adds the transaction and trims the pool (src/validation.cpp, AcceptSingleTransactionInternal), so a transaction whose scripts fail changes nothing. satd now does the same: the cheap fee-rate refusal stays ahead of the scripts, and eviction comes after them. Eviction itself rescanned the whole pool for every evicted entry and every descendant, checking each candidate against a Vec of what was already chosen. It now pops a heap built in one pass, finds descendants through the spends index, and tracks membership in a HashSet. Around the same path: - Eviction never takes the incoming transaction's own ancestors, which left the child in the pool spending an output nothing provided. The fee-rate bar is the cheapest entry it may actually evict. - Admission trims the pool down to its limit, as Core's TrimToSize does, instead of freeing only its own size. A lowered -maxmempool trims the pool at once on reload; before, every admission was refused until the pool drained. - A refusal after eviction reports the entries it evicted. - The specific script error behind a libbitcoinconsensus rejection is found by running the native engine over the failing input only, after the pool's write lock is released. - The orphanage's FIFO queue holds each live orphan once, keyed by an arrival sequence as Core's orphanage is, instead of keeping the txid of every orphan it ever held. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * mempool: report only acting entries evicted; test helpers index spends Eviction removes a held descendant of an evicted acting entry with it. That entry never emitted an Enter, so a LeaveEvicted for it is a Leave for a txid the standard stream never showed. The eviction now returns each entry's scope, and admission (success and refusal) and the -maxmempool reload trim emit LeaveEvicted only for entries that were acting. The test insert helpers that take a caller's transaction now index its spends, as every admission does, since eviction finds descendants through that index. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> (cherry picked from commit 97298b0)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the pool was at its
-maxmempoollimit,accept_transactionevicted entries to make room for the incoming transaction before it checked that transaction's scripts. A transaction whose scripts then failed had already removed entries, raised the rolling minimum fee, and its removals were never reported. Bitcoin Core runsPolicyScriptChecksandConsensusScriptChecksbefore it adds the transaction and trims the pool (src/validation.cpp:1382-1405,AcceptSingleTransactionInternal→FinalizeSubpackage→LimitMempoolSize), so in Core a transaction whose scripts fail changes nothing.The eviction itself (
evict_lowest_fee_entries) sorted the whole class and then, for every evicted entry and every descendant, scanned every entry in the pool, testing each one against aVecof what had already been chosen: roughly n·E² comparisons under the pool's write lock. In a debug build, evicting 600 entries from a 20,000-entry pool took 40 s.The same path had four more problems:
src/txmempool.cpp:868-897), never the parent alone.-maxmempooldecrease, stayed over, so every admission answered "mempool full" until the pool drained. Core'sTrimToSizetrims to the limit (src/txmempool.cpp:861-906).reload_policyswapped the config without trimming.LeaveEvictedevents for entries that were already gone.name_script_failureran the native engine over the whole transaction, under the pool's write lock, only to find the error's name.In the orphanage, the FIFO queue kept the txid of every orphan ever added:
remove, expiry and per-peer eviction left theirs behind, and only a full orphanage popped stale ones off the front.oldest_for_peerscanned that queue on every add from a peer at its 50-orphan quota.Fix
CheckFeeRateruns inPreChecks(src/validation.cpp:948). Then the scripts are checked, and only then is anything evicted.spendsindex (collect_children), and membership is aHashSet. The ranking (lowest modified feerate, descendants removed with their ancestor, the same rolling-minimum update) is unchanged; ties now break by txid instead of hash-map order.evict_lowest_fee_entries_exceptnever chooses an entry in a keep set; admission passes the incoming transaction's in-pool ancestors. The ancestor set is closed upwards, so nothing kept is a descendant of an evicted entry. The feerate bar is the cheapest entry the transaction may actually evict, and when there is none it is refused at once.class_bytes + tx_size - class_budget, the overage with the transaction in the pool, asTrimToSizedoes.reload_policytrims both classes to their new limits right away and reports acting-class evictions asLeaveEvicted { full_pool }. As at admission, quarantine-class evictions emit nothing on the standard stream.LeaveEvictedfor what it evicted.ScriptVerifiergainsverify_transaction_unnamedandname_failure. Their default implementations keep today's behaviour for every other verifier.ConsensusVerifier's unnamed verify is libconsensus alone, and a failure carries the coarseERR_SCRIPT.name_failureruns the native engine over the failing input only.ShadowVerifierpasses both to its primary. Mempool admission and thesubmitpackagefee-bypass path verify under the lock and name the failure after releasing it. Block connection andtestmempoolacceptare unchanged (testmempoolacceptalready verified after releasing the lock). The verdict never changes, and the reason string is the same as before.BTreeMapkeyed by an arrival sequence number, as Core keys its orphan announcements (m_entry_sequence,src/node/txorphanage.cpp:33-49). Removal deletes the orphan's key, so the queue holds exactly the live orphans.oldest_for_peertakes the minimum over that peer's own orphans, at mostmax_per_peerof them.maxmempooltrims the pool at once.Tests
All new tests are in new files:
node/src/mempool/pool_fullpool_tests.rs,node/src/mempool/orphanage_order_tests.rsandnode/src/validation/script_naming_tests.rs. Each was run against the unfixed code first. After the fix, each guard was reverted on its own and the named test failed again.a_script_failure_on_a_full_pool_evicts_nothing(with the must-succeed half: the same transaction with valid scripts evicts exactly one entry)a_child_keeps_its_parent_in_a_full_poola_child_that_outbids_only_its_parent_is_refusedMempoolFull(it had evicted its parent)a_full_pool_refusal_reports_its_evictions[]vs the evicted txid)lowering_maxmempool_trims_the_pool_at_oncean_admission_trims_an_over_limit_poolMempoolFulleviction_cost_follows_what_it_evicts(20,000 entries, evict 600, also checks that the 600 lowest feerates go)a_script_failure_is_named_after_the_pool_is_releasedERR_SCRIPT; naming before the release gives "the failure was named under the pool's write lock"naming_runs_only_the_failing_inputERR_SCRIPTinstead ofOP_RETURN was encounteredthe_unnamed_verify_leaves_the_name_for_later(cpp and rust-shadow)verify_transaction_names_the_failureevicting_a_parent_takes_every_descendanta_peer_at_its_quota_does_not_grow_the_queueremoved_and_expired_orphans_leave_the_queuea_returning_orphan_queues_as_the_newestthe_quota_evicts_the_peers_own_oldestacting_eviction_counts_only_same_class_bytes_toward_goalwas edited in place: its hand-built entries now index their spends, as every admission path does, because eviction finds descendants through that index.Local gate:
cargo clippy --all-targets --all-features -D warnings,cargo clippy -p node --no-default-features --all-targets -D warnings,cargo test -p node --lib,cargo test -p satd --test regtest(mempool, orphan, sendraw, testmempool, package, rbf, replace and sighup filters) andcargo test -p satd --test policy. On one run of the policy target,layer_c_allow_own_submission_but_quarantine_when_gossipedgotbad-txns-inputs-missingorspentfromsendrawtransactionwhile all 22 node pairs were running in parallel. It passed 3 of 3 times on its own, and the whole target passed on a rerun. Input resolution is not touched here.Notes
Release notes
When the mempool is full, an incoming transaction's scripts are now checked before anything is evicted to make room for it, and eviction never removes the incoming transaction's own parent. Eviction now costs time in proportion to what it removes rather than to the size of the pool, so admitting into a full pool no longer holds the mempool lock for long. Lowering
maxmempoolwith a config reload now trims the pool to the new size at once; before, the node refused new transactions until the pool drained below the new limit. The orphan pool's internal queue no longer grows with every orphan ever received.🤖 Generated with Claude Code