core, txpool, miner, eth/gasprice, internal/ethapi: reserved blockspace follow-ups - #2376
Conversation
…m the parent header (POS-3669)
… for unspent gas (POS-3671)
… reserved receipts (POS-3670)
# Conflicts: # cmd/keeper/go.mod
…or a clean compare
…ool occupancy Reserved-blockspace senders (POS-3674) are eviction-immune in the txpool with no aggregate ceiling: a client with enough whitelisted addresses could occupy unbounded pending+queued slots, starving normal fee-paying senders network-wide since the reserved set is registry-derived and therefore consensus-uniform. Adds a combined pending+queued reservedOccupancy counter on LegacyPool, capped by config.ReservedMaxOccupancyPercent (default 50% of GlobalSlots+GlobalQueue). Two-layered by design: an incremental O(1) counter updated at every mutation site, plus a periodic from-scratch recompute in reset() that self-heals any drift every reorg cycle. Admission is gated with a new ErrReservedOccupancyExceeded; a reorg-time backstop trims the largest reserved account via the same prque idiom truncatePending already uses. The new config knob is wired through internal/cli/server like its sibling pool-size settings. POS-3681.
…or a clean compare
…or a clean compare
… suggestions and expose normal-region gas-used ratio
…s with per-client quota utilization
…ge at the fork boundary
…or a clean compare # Conflicts: # consensus/bor/bor.go # consensus/bor/bor_test.go # core/reserved_validation_test.go # core/types/block.go # core/types/block_test.go # miner/worker.go
…or a clean compare # Conflicts: # core/blockchain.go # core/blockchain_test.go # eth/api_debug.go # miner/worker.go
…alidation) for a clean compare
The still-in-pool invariant was judged against both nodes' pools while mined-ness came only from node0's canonical chain. The producing node drops a tx from its pool the moment its own head includes it, which can be several hundred ms before node0 imports that block, so the assertion raced with block propagation (and with tip-fork reinjection windows). A tx now counts as healthy on a node if it is pending in that node's pool or canonical on that node's own chain, and a drop only fails the test after persisting across consecutive polls. A genuine balance eviction is permanent, so it still trips the threshold.
…he reserved set The registry defines feeMode 1 (routed: fee paid, credited to the producer) as reserved for a future external-block-producer world; the spec's zero-fee handling applies to feeMode 0 only. The reader carried FeeMode as metadata but never consulted it, so a routed client's senders inherited the free-mode waiver end to end and mined at effectiveGasPrice 0, identically to feeMode 0. Resolve fee mode at snapshot build, the single choke point every consumer derives from: non-free clients stay out of the effective set, so their senders pay standard fees, their quotas leave EffectiveCapacity, and no downstream surface (EVM waiver, txpool, sequencing, header stamping) needs its own gate. The Snapshot's now-meaningless FeeMode plumbing is removed. registrytest gains a CreateClient helper and caller-provided state (the latter also serves the witness-completeness tests in the next commit) to pin the exclusion against the real registry bytecode.
…d witnesses registryreader.BuildSnapshot read the registry against a throwaway statedb.Copy(), and Copy deep-clones the attached witness: every trie node the read touched landed in the discarded clone and never reached the witness shipped to peers. A block's witness only carried the registry's storage when the block's own transactions happened to touch it, so the first transaction-free block after a registry shape change (an ordinary createClient) permanently wedged every stateless node with "missing trie node" for the registry account; with stateless validators holding voting power, span rotation lost quorum and the whole chain halted. Extract the copy-read-collect protocol the span and state-sync reads already inlined twice in consensus/bor into state.StateDB.ReadIsolated, and route all three call sites through it: run the reads on a reset copy, record them into the live witness (StartPrefetcher + IntermediateRoot), and re-register them on the live state so they enter its FlatDiff read surface (PropagateReadsTo). The shared helper also stops the copy's prefetcher on error paths (previously leaked at the bor.go sites), skips the witness machinery when none is being produced, and detaches the witness while copying instead of cloning it just to replace the clone. A registrytest regression test pins the contract end to end: a witness produced during a snapshot build must let a consumer rebuild the identical snapshot from the witness's node set alone, exactly as a stateless verifier does.
Five pipeline tests reassigned engine, exitCh, or speculativeWorkCh on a live worker whose background goroutines read those fields concurrently, failing go test -race deterministically. Stop the worker's goroutines first (idempotently, so the fixture cleanup can still run) or build a bare worker directly with the wrapped engine where no goroutines are needed. The closed-exitCh swaps become the real thing: a stopped worker's exitCh is genuinely closed.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (76.52%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## reserved-blockspace-block-building #2376 +/- ##
======================================================================
+ Coverage 55.30% 55.52% +0.21%
======================================================================
Files 917 919 +2
Lines 167053 167873 +820
======================================================================
+ Hits 92396 93210 +814
+ Misses 69136 69102 -34
- Partials 5521 5561 +40
... and 27 files with indirect coverage changes
🚀 New features to boost your workflow:
|
| func (pool *LegacyPool) effectiveCost(addr common.Address, tx *types.Transaction) *big.Int { | ||
| if pool.isReserved(addr) { | ||
| return tx.Value() | ||
| } | ||
| return tx.Cost() |
There was a problem hiding this comment.
q on this one: effectiveCost gives value only pricing just because the sender is a registered reserved client, without checking if this specific tx's gas actually fits their quota.
I understand that the comment above says this is intentional, but doesn't that mean a reserved sender can get fee-free pricing AND the eviction protection on a tx that was never going to execute fee-free anyway? Feels like it opens the door to spamming the pool pretty cheaply. Might be worth a quota check at admission time too, wdyt?
| pendingList := pool.pending[from] | ||
|
|
||
| // Reserved-blockspace occupancy cap: reject outright, before any | ||
| // Discard/eviction attempt, if this sender is reserved and admitting the | ||
| // transaction would occupy a genuinely new slot (as opposed to a same- | ||
| // nonce replacement, which leaves combined occupancy unchanged) that | ||
| // pushes aggregate reserved occupancy over its cap. Runs unconditionally, | ||
| // independent of overall pool fullness — unlike the Underpriced exemption | ||
| // below, which only applies once the pool is already globally full. | ||
| if reserved && pool.isNewReservedSlot(pendingList, from, tx) { | ||
| if pool.reservedOccupancy+1 > pool.reservedOccupancyCap() { | ||
| stage0Duration = time.Since(stage0Time) | ||
| return false, ErrReservedOccupancyExceeded | ||
| } |
There was a problem hiding this comment.
The reservedOccupancyCap() is computed against GlobalSlots+GlobalQueue, which is the same denominator the pool uses for its real slot based fullness check (with numSlots(tx)). However, every place that bumps reservedOccupancy just does +1/-1 per tx, never numSlots(tx). So a reserved sender sending big calldata txs (multiple slots each) would only count as "1" against a cap that's measured in slots? Keep me honest here, but could someone blow past the intended 50% pool share pretty easily, right? Maybe worth a check
| return nil, err | ||
| } | ||
| if !c.Active || c.EffectiveFrom > effectiveAt { | ||
| if !c.Active || c.EffectiveFrom > effectiveAt || c.FeeMode != FeeModeFree { |
There was a problem hiding this comment.
looks like non-free (feeMode!=0) clients still get read from the registry every single block before getting filtered out. Not a big deal today since it's gated behind whoever controls the whitelist, but if that list grows with a bunch of non-free addresses, seems like unnecessary per-block work for addresses that can never be reserved anyway. Maybe we should filter earlier in the read path? Not sure how much this matters in practice tho
|
|
||
| var reserved map[common.Hash]struct{} | ||
| for _, receipt := range receipts { | ||
| if isReservedReceipt(receipt) { |
There was a problem hiding this comment.
Here and in feehistory, I think this might hit on the state-sync txs, because it's inferring "reserved" from EffectiveGasPrice == 0, but the state-sync tx also reports 0. Could we end up skewing eth_feeHistory a bit on blocks that have both? Seems like internal/ethapi/api.go uses the actual index table instead, which seems safer. Shall we reuse that here too?
| payload := h.Extra[ExtraVanityLength : len(h.Extra)-ExtraSealLength] | ||
|
|
||
| if chainConfig.Bor != nil && chainConfig.Bor.IsAustin(h.Number) { | ||
| var blockExtraData BlockExtraDataPostAustin |
There was a problem hiding this comment.
If we add the ReservedCapacity as a new field, does this mean an old node can't parse a header once that field shows up? Probably fine since nothing's live yet and we have a HF, but might be worth an addition in the PR description / docs reminding whoever runs the next devnet round that every node needs the new binary before any of them start producing.
| @@ -2612,9 +2627,12 @@ func (bc *BlockChain) InsertReceiptChain(blockChain types.Blocks, receiptChain [ | |||
| headers = append(headers, block.Header()) | |||
| } | |||
|
|
|||
| // Write all chain data to ancients. | |||
| // Write all chain data to ancients. These blocks arrive via receipt | |||
| // sync rather than local execution, so there is no reserved-tx | |||
| // classification to carry; the nil entries make writeAncientBlock | |||
| // record that explicitly. | |||
| td := bc.GetTd(first.Hash(), first.NumberU64()) | |||
| writeSize, err := rawdb.WriteAncientBlocks(bc.db, blockChain, receiptChain, borReceipts, td) | |||
| writeSize, err := rawdb.WriteAncientBlocks(bc.db, blockChain, receiptChain, borReceipts, make([]rlp.RawValue, len(blockChain)), td) | |||
There was a problem hiding this comment.
Do blocks that come in via receipt sync get reservedTxIndexes hardcoded to nil? I mean it would make sense, as no local execution is there to derive it from, but doesn't that mean EffectiveGasPrice for reserved txs on those blocks is just permanently wrong, with no way to backfill? Might be a non-issue if we never actually run that sync mode across the reserved-blockspace range, but wanted to double check.
|
Shall we improve diffguard? Also worth merging develop back to unlock the kurtosis related tests |
2 participants
Summary
Follow-up hardening on top of the core Reserved Blockspace protocol change (#2302): value-only-balance txpool admission and fallback-fee handling (POS-3671), an aggregate reserved-sender occupancy cap to prevent normal-sender starvation (POS-3681), gas-price-oracle/
eth_feeHistoryexclusion of fee-free reserved transactions plus a newnormalGasUsedRatiofield (POS-3675/3676),chain/reserved/*andworker/reserved/*observability metrics (POS-3679), and execution-path produce/import determinism coverage at the fork boundary across the serial and both BlockSTM processors (POS-3672). Also carries the wire-format reconciliation needed when the Austin hard fork changed the header extra-data shape mid-implementation, and two correctness fixes found via live devnet testing (see below).A combined delivery-and-test-status primer for both this branch and the base protocol branch, written for the still-open production go/no-go decision:
Reserved Blockspace - Delivery & Test Status
Executed tests
-racesuites across every touched package, and all 9tests/borreserved integration tests green.registryreader.BuildSnapshotread the registry against a state copy whose witness never reached the real per-block witness -cf9f3e82c). Also foundfeeMode=1clients were mining fee-free identically tofeeMode=0, contradicting the contract's own documented semantics (124087f55).Rollout notes
Consensus-affecting (gated behind
Bor.ReservedBlockspaceBlock, same as the base branch). No operator-facing config changes beyond the existingReservedMaxOccupancyPercenttxpool knob (default 50%, CLI-wired). Both devnet-found issues above are fixed on this branch; production readiness still depends on the smart-contract team's audited registry contract and genesis-contracts parity, neither of which is part of this PR - see the primer's "still outstanding" section.