fix(batch-builder): defer transactions already packed by a peer batch (#1329) - #1330
Open
MavenRain wants to merge 3 commits into
Open
fix(batch-builder): defer transactions already packed by a peer batch (#1329)#1330MavenRain wants to merge 3 commits into
MavenRain wants to merge 3 commits into
Conversation
…#1329) One signed transaction submitted to K validators fills K certified batches and pays once, because execution skips the later copies for free and nothing on the vote path tells the local builder that a peer already packed the transaction. When a peer batch passes validation, remember its transaction hashes on the worker pool for PEER_BATCH_DEFER_TTL (10 s, the default batch_vote_timeout) and let the builder skip them, holding the sender's later nonces for the current build through mark_invalid. When every pending transaction is deferred the build task seals nothing for that tick instead of broadcasting an empty batch. Entries are never refreshed and stay immune to re-arming until twice the TTL, so a byzantine peer adds at most one TTL of delay per two TTLs and cannot censor. The set is capped at PEER_BATCH_SEEN_MAX_TXS (65,536); a full window drops new hashes until entries age out and never evicts early, so a flood of peer batches can only switch the deferral off. Closes #1329. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
Replace the two `match <bool> { true => .., false => .. }` blocks that
#1329 added with plain `if`/`else`:
- batch.rs `build_batch`: the peer-deferred vs gas-limit branch.
- lib.rs `spawn_execution_task`: the empty-batch vs seal branch.
No behavior change. The arm bodies are unchanged apart from indentation.
rustfmt (1.94) reports the crate clean.
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
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.
Closes #1329.
Problem
One signed transaction submitted to K committee validators lands in K batches. Every copy passes peer validation, every batch gets certified, and execution pays for the first copy only: the later copies are skipped with
InvalidTxSkipReason::NonceTooLowat no cost to the sender (crates/tn-reth/src/env/execution.rs). The batch builder is timer driven (crates/batch-builder/src/lib.rs, onemax_batch_delaytick), and nothing on the vote path tells the local builder that a peer already packed a transaction. So one 21,000-gas transfer paid once can occupy 21,000 gas of batch capacity on every validator, and a sender can amplify that to a full batch per validator with a burst of transfers.No attacker is required: a wallet that broadcasts to several RPC endpoints (a common client pattern) produces the same amplification. A deliberate sender can do it to every validator at once.
Fix
Local, protocol neutral, always on. When this node validates a peer batch (any of the three
validate_batchcall sites: the vote path, the prefetched gossip batch, and a synced batch that is not yet certified), it remembers the batch's transaction hashes. The local builder skips a remembered hash and, throughmark_invalid, that sender's later nonces for the current build; they wait as long as the deferred nonce does, which they could not execute ahead of anyway. When every pending transaction is deferred, the build task seals nothing for that tick instead of broadcasting an empty batch, which peers would score as a fatal validation failure. Once the peer batch executes, the pool drops the transaction; the memory only matters while the peer batch is in flight or lost.Threat model:
PEER_BATCH_DEFER_TTLis 10 s, the defaultbatch_vote_timeout(crates/config/src/node.rs): a peer batch without its quorum by then has been abandoned by its producer, and one with quorum is on its way into a header.PEER_BATCH_SEEN_MAX_TXSis 65,536 hashes, about 46 full batches of 21,000-gas transfers atmax_batch_gas, the same order as the engine's repack window cap.Not in this PR, offered as follow-ups: sender-slot routing at RPC ingress (
owning_validatorincrates/tn-reth/src/forward.rsalready computes the slot, but the RPC add path bypasses the Telcoin pool wrapper), and a config knob for the TTL if operators want one. #1268 (repack monitor) keeps measuring what remains.Changes
crates/tn-reth/src/peer_batch.rs(new):PeerBatchTxs, a cheap-clone handle over a bounded, insertion-ordered seen set withrecord/is_deferred(the clock is read under the lock, so the order stays time-sorted across concurrent validators; explicit-Instantvariants are private to the module's tests), plus the two constants and their derivations.crates/batch-builder/src/lib.rs: the build task returnsBuildOutcome::NothingToSealinstead of sealing when the built batch holds no transactions; the run loop treats it as a quiet tick (no pool update, no error log, no refusal backoff).crates/tn-reth/src/txn_pool.rs:WorkerTxPoolcarries aPeerBatchTxs; theTxPooltrait gainsrecord_peer_batchandis_peer_deferred;BestTxns::peer_deferredmarks the candidate invalid with a hand-rolledPeerBatchDeferrederror (is_bad_transactionfalse), so the sender's descendants are held for this build only.crates/batch-validator/src/validator.rs:validate_batchrecords the decoded hashes on full success only. Trait signature unchanged.crates/batch-builder/src/batch.rs: the selection loop skips deferred hashes first;BatchBuilderOutputreports the count andBatchBuilderMetricsexposes it aspeer_deferred_txs_total.crates/batch-builder/src/test_utils.rs:TestPoolimplements the two new trait methods, and its best-transactions stand-in now tracks invalid senders the way reth's iterator does, so descendants are skipped in tests too.crates/tn-reth/src/lib.rs: exports the new module and its two constants, and re-exportsSenderIdfor the test pool.tn-rethandbatch-builderdescribe the deferral next to the existing note that duplicates across workers stay tolerated at execution.Testing
Under the pinned toolchain (
rust-toolchain.toml, 1.94) with the shared target dir:New tests, all deterministic (explicit
Instants,wait_untilfor convergence, no sleeps):peer_batchunit tests (eight): fresh hash deferred, unknown hash not deferred, expiry at exactly the TTL, no refresh on re-record, immune window until twice the TTL, re-arm after that, a full window drops new hashes until entries age out, a full window cannot re-arm an immune entry, one oversized record keeps only the first cap hashes.batch.rsunit test withTestPool: sender A's first nonce is recorded, the build packs only sender B, both of A's transactions stay out, the deferred count is one.lib.rsunit test: a pool whose only pending transaction is deferred makes the build task return the no-work outcome and send nothing to the worker.build_batches.rsintegration test: a valid peer batch holding tx1 goes throughBatchValidator::validate_batch, the next built batch holds only tx2, and the pool reports tx1 deferred and tx2 not.Mutation check, three rounds: with
is_peer_deferredforced to false on both pools, and separately with the builder's skip neutralised, the builder unit test and the integration test fail; with the empty-batch guard removed, thelib.rstest fails; with oldest-first eviction reintroduced, the immune-entry test fails. Restored, all pass.🤖 Generated with Claude Code