EIP-8141: blob-pool routing, sidecar, and production budget for blob-carrying frame transactions - #12709
EIP-8141: blob-pool routing, sidecar, and production budget for blob-carrying frame transactions#12709Marchhill wants to merge 2 commits into
Conversation
|
Claude finished @Marchhill's task in 9m 25s —— View job Review: EIP-8141 blob-pool routing & production blob budget
The core idea is right, and the production side is genuinely safe: because The problem is that routing was moved to Findings
Why 1–3 compoundNo validator requires the mempool form for type 6 ( Suggested test additions
I did not run a build or the test suite for this review — findings are from source reading. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| bool eip1559Enabled = _specProvider.GetCurrentHeadSpec().IsEip1559Enabled; | ||
| UInt256 effectiveGasPrice = tx.CalculateEffectiveGasPrice(eip1559Enabled, _headInfo.CurrentBaseFee); | ||
| TxDistinctSortedPool relevantPool = (tx.SupportsBlobs ? _blobTransactions : _transactions); | ||
| TxDistinctSortedPool relevantPool = (tx.CarriesBlobs ? _blobTransactions : _transactions); |
There was a problem hiding this comment.
High — the default (persistent) blob pool loses the transaction type, and that path is untested.
BlobsSupport defaults to StorageWithReorgs (TxPoolConfig.cs:14), so in production _blobTransactions is a PersistentBlobTxDistinctSortedPool. Its InsertCore stores new LightTransaction(fullBlobTx) in memory and writes the tx to the blob DB:
// PersistentBlobTxDistinctSortedPool.cs:62
if (base.InsertCore(hash, new LightTransaction(fullBlobTx), groupKey))
{
_blobTxCache.Set(fullBlobTx.Hash, fullBlobTx);
_blobTxStorage.Add(fullBlobTx);and LightTransaction hard-codes the type (LightTransaction.cs:18 and again in the deserializing ctor):
Type = TxType.Blob;Consequences for a routed type-6 tx:
- the in-memory blob-pool entry claims
TxType.Blob, so everySupportsBlobs-keyed branch that sees the light tx takes the type-3 path (and after a node restartLightTxDecoder.Decoderebuilds it as type 3 — the type is not persisted in the light record at all, only in the full-tx record); LightTxDecoder.Encodedereferencestx.MaxFeePerBlobGas!.Value, which is only non-null for frame txs that came off the wire;- every blob-less type-6 tx is written to the
FullBlobTxs/LightBlobTxscolumns and reloaded on every restart, even thoughResolveBlobinTxPoolTxSourcecan never produce it (noNetworkWrapper), so the slot and the disk entry are held indefinitely.
The three new routing tests all pin BlobsSupport = BlobsSupportMode.InMemory (TxPoolTests.Blobs.cs:1420/1439/1458), so none of this is covered. Please add a StorageWithReorgs case (submit → restart-equivalent reload → assert type/round-trip) or explicitly restrict routing to the in-memory mode until the sidecar lands.
There was a problem hiding this comment.
The in-memory routing/admission/eviction consistency is now fixed (filters, expiry pass, reorg and processed-tx accounting all key on CarriesBlobs). On the persistent-pool specifics, two of the cited failure modes don't hold as stated:
LightTxDecoder.Encodedoes not NRE here: only blob-carrying frame txs reach the blob pool, and validation now requires a non-nullMaxFeePerBlobGasfor them (the decoder also defaults it to 0), soMaxFeePerBlobGas!.Value/BlobVersionedHashes!are always non-null on this path.- Blob-less type-6 txs are not written to the blob DB: routing keys on
CarriesBlobs, so they go to the normal pool.
The real residual is that the light in-memory record is typed Blob and doesn't carry the frame body, so on restart a persisted blob-carrying frame tx reloads as a type-3 light entry (the full tx is still stored via the full TxDecoder). That's latent today: such a tx can't be included (production guard) or resolved (no sidecar). Persisting the type/body through LightTransaction+LightTxDecoder is a storage-format change entangled with the deferred type-6 sidecar wire format, so I'm leaving it out of this PR rather than encoding a format that isn't settled — and not adding a test that would pin the current reload behaviour.
| TxDistinctSortedPool otherTxTypePool = tx.SupportsBlobs ? txs : blobTxs; | ||
| // EIP-8141: route on the instance-level blob gate so a blob-carrying frame tx is treated as a | ||
| // blob-pool member (conflicts with the sender's normal-pool txs), mirroring type-3 routing. | ||
| TxDistinctSortedPool otherTxTypePool = tx.CarriesBlobs ? txs : blobTxs; |
There was a problem hiding this comment.
High — routing moved but the admission filters did not: a frame-blob tx is now admitted under normal-pool rules into the blob pool.
TxTypeTxFilter, AddCore, the snapshot/eviction/metrics sites were switched to CarriesBlobs, but every filter that picks a pool or a limit still keys on the type-level SupportsBlobs (false for type 6):
| site | effect for a blob-carrying frame tx |
|---|---|
FeeTooLowFilter.cs:44 |
fullness / worst-fee check runs against _txs, so the fee floor that protects a full blob pool is skipped |
BalanceTooLowFilter.cs:39 |
cumulative-cost bucket is read from _txs; TxTypeTxFilter guarantees that bucket is empty, so the sender's existing blob-pool frame txs are never counted — balance is checked per-tx only |
FutureNonceFilter.cs:15 |
uses MaxPendingTxsPerSender (default 0 = unlimited) instead of MaxPendingBlobTxsPerSender (default 16) |
GapNonceFilter.cs:24,29 |
gap allowance keyed on _txs.IsFull() and bucket count read from _txs — gaps are allowed in a pool whose eviction cascade exists precisely to prevent them (TxPool.cs:882) |
SizeTxFilter.cs:19 |
MaxTxSize instead of MaxBlobTxSize (bites once a sidecar exists) |
NotSupportedTxFilter.cs:21 |
with BlobsSupport=Disabled the tx is not rejected; it is routed to a capacity-0 blob pool and comes back as FeeTooLowToCompete instead of NotSupportedTxType |
Taken together with the fact that no validator requires the mempool form for type 6 (TxValidator.cs:94, and MempoolBlobTxProofVersionValidator early-returns on !SupportsBlobs), a single sender can push an unbounded number of blob-less type-6 txs — none individually priced against the blob pool and none charged for their siblings — into the 16K-slot persistent blob pool, each also written to the blob DB. Exposure is currently limited to devnets by the IsEip8141Enabled gate in NotSupportedTxFilter, but this should be closed (or the per-sender/balance/fee filters explicitly deferred with a marker) before the routing lands.
There was a problem hiding this comment.
Addressed. FeeTooLowFilter, BalanceTooLowFilter, FutureNonceFilter, GapNonceFilter, SizeTxFilter and NotSupportedTxFilter now key on CarriesBlobs, so a blob-carrying frame tx picks the blob pool/limit, counts against MaxPendingBlobTxsPerSender, and is rejected as NotSupportedTxType under BlobsSupport=Disabled. Added routing tests for the per-sender blob limit and the blobs-disabled rejection.
| updateTx(transactions, tx, null, lastElement); | ||
| // evict all following txs to prevent nonce gaps between blob tx | ||
| evictNextTxs |= tx.SupportsBlobs; | ||
| evictNextTxs |= tx.CarriesBlobs; |
There was a problem hiding this comment.
High — routing to the blob pool silently disables frame-tx expiry eviction.
The on-head eviction pass added for EIP-8141 only ever looks at the normal pool:
// TxPool.cs:496 (RemoveExpiredFrameTransactions)
Transaction[] snapshot = _transactions.GetSnapshot();and its fast-path counter is only fed by the normal pool's events (TxPool.cs:148-149):
_transactions.Inserted += OnInsertedTx; // increments _expiringFrameTxCount
_transactions.Removed += OnRemovedTx;So after this PR a blob-carrying frame tx with an expiry-verifier deadline (a) never increments _expiringFrameTxCount, so the pass short-circuits at TxPool.cs:489 even when such txs are pending, and (b) is not in the scanned snapshot anyway. An expired blob-carrying frame tx therefore stays in the (persistent) blob pool forever — it can never be included, ResolveBlob will never produce it, and nothing evicts it. That removes the only bound on the occupancy problem described in the TxTypeTxFilter comment.
Either scan _blobTransactions in the same pass and subscribe the blob pool's Inserted/Removed to the same handlers, or add a regression test asserting an expired blob-carrying frame tx is evicted on a new head.
There was a problem hiding this comment.
Addressed. The blob pool's Inserted/Removed now feed the same expiry counter, and RemoveExpiredFrameTransactions scans both pools. Added a regression test asserting an expired blob-carrying frame tx is evicted from the blob pool on a new head.
| Interlocked.Increment(ref _pendingTransactionsAdded); | ||
| if (tx.Supports1559) { Metrics.Pending1559TransactionsAdded++; } | ||
| if (tx.SupportsBlobs) { Metrics.PendingBlobTransactionsAdded++; } | ||
| if (tx.CarriesBlobs) { Metrics.PendingBlobTransactionsAdded++; } |
There was a problem hiding this comment.
Medium — the block-processing / reorg paths were not switched, so they now disagree with the routing.
Two sites still key on SupportsBlobs and are reached with the block's consensus-form tx:
RemoveProcessedTransactions(TxPool.cs:414) — a blob-carrying frame tx is not counted inblobTxs/blobsmetrics (inconsistent with theMetrics.PendingBlobTransactionsAddedincrement on this line) and is not written to the processed-blob-txs store, so it cannot be restored viaTryGetBlobTransactionsFromBlock.ReAddReorganisedTransactions(TxPool.cs:361) — becauseSupportsBlobsis false thecontinueis skipped, so on a reorg the tx is re-submitted from the block body, i.e. in bare consensus form with no sidecar, and now lands back in the blob pool (and, under the default persistent mode, back in the blob DB) as a permanently unusable entry.
Since blob-carrying frame txs cannot be produced yet, the cleanest resolution may be to make both sites use CarriesBlobs too so the two pools stay symmetric.
There was a problem hiding this comment.
Addressed. ReAddReorganisedTransactions and RemoveProcessedTransactions now key on CarriesBlobs, so a blob-carrying frame tx is not re-submitted in bare consensus form on a reorg and is counted/saved consistently with the blob metrics.
| // EIP8141: a blob-carrying frame tx (type 6) has no EIP-7594 sidecar network wrapper yet, so it | ||
| // cannot participate in the announce-by-hash / serve-with-sidecar blob gossip protocol. Withhold | ||
| // it from gossip until that wire format lands, rather than leaking it in bare consensus form. | ||
| tx.CarriesBlobs && !tx.SupportsBlobs |
There was a problem hiding this comment.
Medium — the gossip policy only covers the announce/broadcast path, not the request/response path.
ShouldGossipTransaction is consulted from TxBroadcaster only (TxBroadcaster.cs:87,350). A peer that already knows the hash and sends GetPooledTransactions is served from TryGetPendingTransaction, which now reaches the blob pool (TxPool.cs:975-978), and the send-side blob guards also key on the type:
// SyncPeerProtocolHandlerBase.cs:199 / :234
if (!tx.SupportsBlobs) //additional protection from sending full tx with blobSupportsBlobs is false for type 6, so the bare consensus form is still put on the wire — the exact leak this change is meant to prevent. Also TxBroadcaster.cs:123 stores the frame-blob tx in full (not as a LightTransaction) in the persistent-broadcast set, and TxBroadcaster.cs:370 will hand it back from TryGetPersistentTx. Consider gating on CarriesBlobs in those guards as well, or documenting that only announcements are withheld.
Low (style) — the ternary-returning-false is hard to read; the whole predicate collapses to:
| tx.CarriesBlobs && !tx.SupportsBlobs | |
| // EIP8141: a blob-carrying frame tx (type 6) has no EIP-7594 sidecar network wrapper yet, so it | |
| // cannot participate in the announce-by-hash / serve-with-sidecar blob gossip protocol. Withhold | |
| // it from gossip until that wire format lands, rather than leaking it in bare consensus form. | |
| tx.SupportsBlobs | |
| ? tx.GetProofVersion() == ChainHeadInfoProvider.CurrentProofVersion | |
| : !tx.CarriesBlobs; |
There was a problem hiding this comment.
The send-side guards (SyncPeerProtocolHandlerBase 199/234) now key on CarriesBlobs, so the announce/broadcast paths never put a blob-carrying frame tx on the wire in full form, and I simplified the gossip-policy ternary as suggested. FulfillPooledTransactionsRequest serves whatever the pool returns and predates this routing change; withholding there touches the request/response protocol path and the persistent-broadcast light storage, which I'm leaving with the deferred sidecar work — for now only announcements are withheld.
| // signature list) — signature/intrinsic-gas validators do not apply; per-frame gas and | ||
| // signature validation happen during processing. | ||
| // EIP8141: a blob-carrying frame tx currently has no EIP-7594 sidecar network wrapper, so no | ||
| // sidecar/proof validator is registered here (a wrapper on a frame tx is rejected as malformed |
There was a problem hiding this comment.
Low — this comment asserts a guarantee that is not enforced.
"a wrapper on a frame tx is rejected as malformed by NonBlobFieldsTxValidator's absence" is backwards: NonBlobFieldsTxValidator is deliberately not in the type-6 chain, and the absence of a validator cannot reject anything. What actually holds today is narrower and worth stating plainly: FrameTxDecoder never produces a NetworkWrapper, so a frame tx off the wire cannot carry one — and consequently nothing requires the mempool form for type 6 either (MempoolBlobTxValidator is type-3 only, and MempoolBlobTxProofVersionValidator early-returns at line 381 on !SupportsBlobs). Per .agents/rules/coding-style.md, a comment that contradicts the code is worse than none.
Note this is also the property that lets a bare, blob-less type-6 tx be admitted straight into the blob pool — see the TxTypeTxFilter comment.
There was a problem hiding this comment.
Reworded to state the actual invariant: FrameTxDecoder never decodes a network wrapper for a type-6 tx, so no sidecar/proof validator is registered and no mempool-form requirement exists yet.
| /// and decode paths key on. Mirrors the accounting predicate used in block validation and the blob | ||
| /// gas calculator so a blob-carrying frame tx counts towards the block blob budget like a type-3 tx. | ||
| /// </remarks> | ||
| public bool CarriesBlobs => BlobVersionedHashes is { Length: > 0 }; |
There was a problem hiding this comment.
Low — the new property duplicates predicates it says it mirrors.
The XML doc says it "mirrors the accounting predicate used in block validation and the blob gas calculator", but those inline copies are left in place, so the repo now has four spellings of the same rule:
BlockValidator.cs:353—if (transaction.BlobVersionedHashes is not { Length: > 0 })TransactionExtensions.cs:79—if (tx.BlobVersionedHashes is { Length: > 0 })BlockProcessor.BlockProductionTransactionPicker.cs:76—currentTx.Type == TxType.FrameTx && currentTx.BlobVersionedHashes is { Length: > 0 }
Folding those onto CarriesBlobs is exactly the kind of change that makes the new property pay for itself (AGENTS.md: prefer the option that removes code; DRY).
Low, separate — BlobTxPriorityComparer (Nethermind.Consensus/Comparers/BlobTxPriorityComparer.cs:28) still compares on SupportsBlobs, so inside the producer comparer a blob-carrying frame tx loses the blob tie-break to an equally-priced type-3 tx. Minor, but it contradicts the "metered/selected exactly like a type-3 tx" goal.
There was a problem hiding this comment.
Addressed. Folded BlockValidator, TransactionExtensions and both picker sites onto CarriesBlobs, and switched BlobTxPriorityComparer to CarriesBlobs so a blob-carrying frame tx gets the same producer tie-break as a type-3 tx.
…lob-carrying frame transactions Complete the non-consensus blob layer for EIP-8141 frame transactions on top of the consensus-correct accounting/validation/settlement in PR #12701. - Blob-pool routing: introduce an instance-level Transaction.CarriesBlobs gate (BlobVersionedHashes present) and route on it at the pool-membership sites so a blob-carrying frame tx (type 6) lands in the blob pool like a type-3 tx, without touching the type-level SupportsBlobs() that networking/decode paths key on. Type-3 routing is unchanged. TxTypeTxFilter, snapshot invalidation, eviction cascade, metrics, and ContainsTx follow the same gate. - Production blob-budget metering: routing feeds frame-blob txs through the blob selection path in TxPoolTxSource, so they are metered against the block blob budget (MaxBlobGasPerBlock / MaxProductionBlobCount) and a produced block never exceeds it. The normal-pool picker guard is kept as defense in depth for any frame-blob tx that reaches production without a resolvable sidecar. - Networking: withhold blob-carrying frame txs from gossip until the EIP-7594 sidecar wire format exists, rather than leaking them in bare consensus form. Deferred (documented markers): the EIP-7594 sidecar / network-wrapper wire format for type 6 (decode/encode + mempool acceptance of the wrapper form). Production of frame-blob txs is gated on it, since the blobs bundle cannot be built without the sidecar. Tests: frame-blob tx routes to the blob pool while a plain frame tx and a type-3 tx route unchanged; a frame-blob tx with a sidecar is metered/selected within the blob budget and excluded when it would overflow it.
…sistently Address review follow-ups on the blob-network PR: - switch the admission filters (fee/balance/future-nonce/gap-nonce/size/ not-supported) to the instance blob predicate so a blob-carrying frame tx obeys blob-pool limits and is rejected when blobs are disabled - scan the blob pool in the frame-expiry eviction pass and feed its inserts/removals to the expiry counter - align the reorg re-add and processed-tx accounting with the routing predicate - withhold blob-carrying frame txs from the peer send path and simplify the gossip policy predicate - fold duplicate blob predicates onto CarriesBlobs and correct the type-6 validator comment - add blob-pool routing regression tests (expiry eviction, blobs-disabled rejection, per-sender blob limit)
055d6d5 to
d0e510b
Compare
1 participant
Completes the deferred non-consensus blob layer for EIP-8141 blob-carrying frame transactions, stacked on top of #12701 (which added the consensus-correct blob accounting/validation/settlement/sig-hash). Addresses the
EIP8141:deferral markers left by #12701.What this does
Transaction.CarriesBlobsgate (BlobVersionedHashespresent); the pool routes on it so a blob-carrying frame tx (type 6) lands in the blob pool exactly like a type-3 tx. The type-levelSupportsBlobs()that networking/decode paths key on is left untouched, so type-3 routing is unchanged. The same gate drivesTxTypeTxFilterconflict detection, snapshot invalidation, eviction cascade, metrics, andContainsTx.TxPoolTxSourceand are metered against the block blob budget (MaxBlobGasPerBlock/MaxProductionBlobCount), so a produced block can never exceed it. The normal-pool picker guard is reworded and kept as defense-in-depth.SpecDrivenTxGossipPolicy) until the sidecar wire format lands, rather than leaking them in bare consensus form.Deferred (scope note)
The EIP-7594 sidecar / network-wrapper wire format for type 6 (decode/encode + mempool acceptance of the wrapper form) is deferred with documented
EIP8141:markers — the wire format is the risky piece and is left for a focused follow-up. Real-world production of frame-blob txs is gated on it (the blobs bundle cannot be built without the sidecar); the metering and selection logic here already work end-to-end once a wrapper is present, as the production test demonstrates.Tests
Draft.