From 53ee0471e54ef8bd89869522dcf8573dcfe4546b Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Sun, 9 Aug 2026 23:49:22 +0700 Subject: [PATCH 1/5] execution/stagedsync: skip the fee credit when the recorded set already carries it The apply loop re-credits every tx once per validation round, and a tx is revalidated whenever an earlier tx's write set moves under it. calcFees rebuilt an identical write set each round and the caller merged it back in, so a credit that never changed cost a WriteSet, four VersionedWrites, an Account, an O(tx writes) MergeInto and a ReleaseMaps every time. calcFees now takes the set an earlier fee merge produced for the tx and returns nothing when that set already carries the exact credit. The comparison covers the balance value, version and reason plus the AddressPath account, so a moved base balance or a half-recorded credit still re-credits. Comparing against feeMergeTemp rather than TxOut is what makes this safe: calcFees reads TxOut as the pre-credit balance, so folding into it re-adds the tip every round. BenchmarkCalcFees, 200000x n=6: redundant_recredit 313ns -> 92ns, 10 -> 2 allocs/op, 928B -> 192B/op. The first credit of a tx is unchanged. --- execution/stagedsync/exec3_fee_credit_test.go | 180 ++++++++++++++++++ execution/stagedsync/exec3_finalize_test.go | 8 +- execution/stagedsync/exec3_parallel.go | 152 +++++++++------ 3 files changed, 274 insertions(+), 66 deletions(-) create mode 100644 execution/stagedsync/exec3_fee_credit_test.go diff --git a/execution/stagedsync/exec3_fee_credit_test.go b/execution/stagedsync/exec3_fee_credit_test.go new file mode 100644 index 00000000000..c9976f560e3 --- /dev/null +++ b/execution/stagedsync/exec3_fee_credit_test.go @@ -0,0 +1,180 @@ +package stagedsync + +import ( + "testing" + + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/chain" + "github.com/erigontech/erigon/execution/state" + "github.com/erigontech/erigon/execution/types/accounts" +) + +// feeCreditRound mirrors one apply-loop validation round for a single tx: +// calcFees derives the credit, and a non-empty result is folded into the +// recorded write set the way nextResult does it. +type feeCreditRound struct { + result *execResult + task *taskVersion + vm *state.VersionMap + reader *mapStateReader + rules *chain.Rules + recorded *state.WriteSet + // credited tracks the recorded set once a fee merge produced it, which is + // blockExecutor.feeMergeTemp's job in the apply loop. + credited *state.WriteSet +} + +func newFeeCreditRound(t testing.TB, s *testFinalizeScenario) *feeCreditRound { + t.Helper() + + result := s.buildExecResult() + result.TxIn = copyReadSet(s.txIn) + result.TxOut = copyWrites(s.txOut) + result.CollectorWrites = copyWrites(s.collectorWrites) + + vm := state.NewVersionMap(nil) + vm.FlushVersionedWrites(result.TxOut, true, "") + + return &feeCreditRound{ + result: result, + task: result.Task.(*taskVersion), + vm: vm, + reader: s.makeReader(), + rules: s.rules, + recorded: result.TxOut, + } +} + +// run performs one round and returns the credit calcFees produced, nil when it +// found the recorded set already carries it. +func (r *feeCreditRound) run(t testing.TB) *state.WriteSet { + t.Helper() + + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, r.credited) + require.NoError(t, err) + if tip.IsEmpty() { + return nil + } + r.recorded = r.recorded.MergeInto(tip) + r.credited = r.recorded + return tip +} + +func TestCalcFees_SkipsRedundantReCredit(t *testing.T) { + t.Parallel() + r := newFeeCreditRound(t, simpleTransferScenario()) + + require.NotNil(t, r.run(t), "the first round must credit the tip") + require.Nil(t, r.run(t), + "re-crediting a set that already carries this exact credit rebuilds an identical "+ + "write set and re-runs the merge for nothing") +} + +func TestCalcFees_ReCreditsWhenPriorBalanceChanged(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + r := newFeeCreditRound(t, s) + + require.NotNil(t, r.run(t), "the first round must credit the tip") + + // A prior tx moved the coinbase balance, so the tip lands on a new base. + priorBalance := uint256.NewInt(7_000_000) + r.reader.accounts[s.coinbase] = &accounts.Account{Balance: *priorBalance, CodeHash: accounts.EmptyCodeHash} + + tip := r.run(t) + require.NotNil(t, tip, "a changed base balance must produce a fresh credit") + + credited := findBalance(tip, s.coinbase) + require.NotNil(t, credited) + require.Equal(t, *new(uint256.Int).Add(priorBalance, &s.feeTipped), credited.Val) +} + +func TestCalcFees_ReCreditsWhenAddressPathMissing(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "the first round must credit the tip") + + // A half-recorded credit is not a credit: the balance alone leaves + // downstream reads without an account record. + balanceOnly := &state.WriteSet{} + bw, ok := first.GetBalance(s.coinbase) + require.True(t, ok) + balanceOnly.SetBalance(s.coinbase, bw) + + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, balanceOnly) + require.NoError(t, err) + require.False(t, tip.IsEmpty(), "a recorded balance without its AddressPath sibling must be re-credited") + require.NotNil(t, findAddress(tip, s.coinbase)) +} + +func TestCalcFees_SkipsRedundantReCreditWithBurntContract(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + s.rules = &chain.Rules{IsSpuriousDragon: true, IsLondon: true} + s.burntAddr = fAddr("burntcontract") + s.feeBurnt = *uint256.NewInt(1000) + s.accts[s.burntAddr] = fMakeAccount(500_000, 0) + + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "the first round must credit the tip") + require.NotNil(t, findBalance(first, s.burntAddr), "London burns to the burnt contract") + + require.Nil(t, r.run(t), + "both halves of the credit are already recorded, so the round is a no-op") +} + +var feeCreditSink *state.WriteSet + +func BenchmarkCalcFees(b *testing.B) { + for _, bc := range []struct { + name string + recredit bool + }{ + {"first_credit", false}, + {"redundant_recredit", true}, + } { + b.Run(bc.name, func(b *testing.B) { + r := newFeeCreditRound(b, simpleTransferScenario()) + var credited *state.WriteSet + if bc.recredit { + require.NotNil(b, r.run(b)) + credited = r.credited + } + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, credited) + if err != nil { + b.Fatal(err) + } + feeCreditSink = tip + } + }) + } +} + +func TestCreditedWrites(t *testing.T) { + t.Parallel() + be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}} + txOut, merged := &state.WriteSet{}, &state.WriteSet{} + + require.Nil(t, be.creditedWrites(0, txOut), + "before any fee merge the recorded set is the worker's own output") + + be.recordFeeMerge(0, txOut, merged) + require.Same(t, merged, be.creditedWrites(0, merged)) + require.Nil(t, be.creditedWrites(0, txOut), + "a re-execution re-records the worker's TxOut, which carries no credit") + require.Nil(t, be.creditedWrites(1, merged), + "another tx's fee-merge product says nothing about this tx") + require.Nil(t, be.creditedWrites(2, nil), + "a tx with no writes at all must not read as credited") +} diff --git a/execution/stagedsync/exec3_finalize_test.go b/execution/stagedsync/exec3_finalize_test.go index e39fa202c4a..a48f1ee736c 100644 --- a/execution/stagedsync/exec3_finalize_test.go +++ b/execution/stagedsync/exec3_finalize_test.go @@ -565,7 +565,7 @@ func (s *testFinalizeScenario) runFinalizeTx(t *testing.T, priorCoinbaseBalance task := result.Task.(*taskVersion) - writes, err := result.calcFees(task, vm, reader, s.rules) + writes, err := result.calcFees(task, vm, reader, s.rules, nil) require.NoError(t, err) return writes } @@ -829,7 +829,7 @@ func TestFinalizeTxSimple_SenderIsCoinbase_AccumulatedAcrossTxs(t *testing.T) { vm.FlushVersionedWrites(result.TxOut, true, "") - writes, err := result.calcFees(task, vm, reader, s.rules) + writes, err := result.calcFees(task, vm, reader, s.rules, nil) require.NoError(t, err, "tx %d: calcFees", txIdx) // Flush finalize writes so the next tx sees them via versionMap. @@ -897,7 +897,7 @@ func TestFinalizeTxSimple_SenderIsCoinbase_ReExecutedIncarnation(t *testing.T) { // Now flush the re-executed TxOut at incarnation 1. vm.FlushVersionedWrites(result.TxOut, true, "") - writes, err := result.calcFees(task, vm, reader, s.rules) + writes, err := result.calcFees(task, vm, reader, s.rules, nil) require.NoError(t, err) coinbaseWrite := findBalance(writes, s.coinbase) @@ -989,7 +989,7 @@ func TestFinalizeTxSimple_AccumulatedFees(t *testing.T) { // Flush TxOut to versionMap (simulates line 1928). vm.FlushVersionedWrites(result.TxOut, true, "") - writes, err := result.calcFees(task, vm, reader, s.rules) + writes, err := result.calcFees(task, vm, reader, s.rules, nil) require.NoError(t, err) // Flush finalize writes to versionMap for next TX. diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index fa00620e13b..d3688fa5000 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -1907,6 +1907,7 @@ func (result *execResult) calcFees( vm *state.VersionMap, stateReader state.StateReader, chainRules *chain.Rules, + credited *state.WriteSet, ) (*state.WriteSet, error) { txIndex := task.Version().TxIndex taskVersion := task.Version() @@ -2009,85 +2010,101 @@ func (result *execResult) calcFees( return nil, nil } + coinbaseEntry := feeEntry{ + addr: result.Coinbase, + acc: feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce), + reason: tracing.BalanceIncreaseRewardTransactionFee, + deleted: coinbaseEmptyRemoval && coinbaseEmptyPre && newCoinbaseBalance.IsZero(), + emit: emitCoinbase, + } + burntEntry := feeEntry{ + addr: burntAddr, + acc: feeAddressAccount(burntAcc, newBurntBalance, 0), + reason: tracing.BalanceDecreaseGasBuy, + emit: emitBurnt, + } + // The apply loop re-credits a tx once per validation round, and the credit + // only moves when a prior tx's writes moved under it. An unchanged credit + // would rebuild a set identical to the one already recorded. + if coinbaseEntry.recordedIn(credited, taskVersion) && burntEntry.recordedIn(credited, taskVersion) { + return nil, nil + } + addWrites := &state.WriteSet{} if emitCoinbase { result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( result.Coinbase, coinbaseAcc, newCoinbaseBalance, tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) - if coinbaseEmptyRemoval && coinbaseEmptyPre && newCoinbaseBalance.IsZero() { - addWrites.SetSelfDestruct(result.Coinbase, &state.VersionedWrite[bool]{ - WriteHeader: state.WriteHeader{ - Address: result.Coinbase, - Path: state.SelfDestructPath, - Version: taskVersion, - }, - Val: true, - }) - } else { - addWrites.SetBalance(result.Coinbase, &state.VersionedWrite[uint256.Int]{ - WriteHeader: state.WriteHeader{ - Address: result.Coinbase, - Path: state.BalancePath, - Version: taskVersion, - Reason: tracing.BalanceIncreaseRewardTransactionFee, - }, - Val: newCoinbaseBalance, - }) - // Emit an AddressPath sibling write so downstream parallel txs - // reading this address see an account record. Serial's AddBalance - // implicitly creates the account on first credit; parallel calcFees - // must mirror that, otherwise getVersionedAccount returns nil for - // a freshly-credited coinbase (no pre-block storage entry, no - // versionMap AddressPath) and Empty() returns true — charging the - // stale CallNewAccountGas (+25000) for a CALL-with-value to the - // coinbase mid-tx. Mainnet block 25151825 tx 31's SD+CREATE2-on- - // coinbase MEV pattern surfaced this divergence. - addrAcc := feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce) - addWrites.SetAddress(result.Coinbase, &state.VersionedWrite[*accounts.Account]{ - WriteHeader: state.WriteHeader{ - Address: result.Coinbase, - Path: state.AddressPath, - Version: taskVersion, - }, - Val: addrAcc, - }) - } + coinbaseEntry.writeTo(addWrites, taskVersion) } if emitBurnt { result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( burntAddr, burntAcc, newBurntBalance, tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) - addWrites.SetBalance(burntAddr, &state.VersionedWrite[uint256.Int]{ - WriteHeader: state.WriteHeader{ - Address: burntAddr, - Path: state.BalancePath, - Version: taskVersion, - Reason: tracing.BalanceDecreaseGasBuy, - }, - Val: newBurntBalance, - }) - // Mirror the AddressPath emission above for the burnt address. - burntAddrAcc := feeAddressAccount(burntAcc, newBurntBalance, 0) - addWrites.SetAddress(burntAddr, &state.VersionedWrite[*accounts.Account]{ - WriteHeader: state.WriteHeader{ - Address: burntAddr, - Path: state.AddressPath, - Version: taskVersion, - }, - Val: burntAddrAcc, - }) + burntEntry.writeTo(addWrites, taskVersion) } return addWrites, nil } +// feeEntry is one address's share of a tip credit: the post-credit account, +// whose Balance is also the BalancePath value, or a delete when EIP-161 removes +// the emptied account instead. +type feeEntry struct { + addr accounts.Address + acc accounts.Account + reason tracing.BalanceChangeReason + deleted bool + emit bool +} + +// recordedIn reports whether ws already carries this entry verbatim. +func (e feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { + if !e.emit { + return true + } + if e.deleted { + sd, ok := ws.GetSelfDestruct(e.addr) + return ok && sd.Val && sd.Version == version + } + bw, ok := ws.GetBalance(e.addr) + if !ok || bw.Val != e.acc.Balance || bw.Version != version || bw.Reason != e.reason { + return false + } + aw, ok := ws.GetAddress(e.addr) + return ok && aw.Val != nil && *aw.Val == e.acc && aw.Version == version +} + +func (e feeEntry) writeTo(ws *state.WriteSet, version state.Version) { + if e.deleted { + ws.SetSelfDestruct(e.addr, &state.VersionedWrite[bool]{ + WriteHeader: state.WriteHeader{Address: e.addr, Path: state.SelfDestructPath, Version: version}, + Val: true, + }) + return + } + ws.SetBalance(e.addr, &state.VersionedWrite[uint256.Int]{ + WriteHeader: state.WriteHeader{Address: e.addr, Path: state.BalancePath, Version: version, Reason: e.reason}, + Val: e.acc.Balance, + }) + // The AddressPath sibling mirrors serial's AddBalance creating the account + // on first credit. Without it getVersionedAccount returns nil for a + // freshly-credited address and Empty() is true, charging the stale + // CallNewAccountGas for a CALL-with-value to the coinbase mid-tx. + acc := e.acc + ws.SetAddress(e.addr, &state.VersionedWrite[*accounts.Account]{ + WriteHeader: state.WriteHeader{Address: e.addr, Path: state.AddressPath, Version: version}, + Val: &acc, + }) +} + // feeAddressAccount builds the AddressPath value for a fee credit from read, the // address's pre-credit account. nonce applies only when there is no pre-state. -func feeAddressAccount(read *accounts.Account, balance uint256.Int, nonce uint64) *accounts.Account { +func feeAddressAccount(read *accounts.Account, balance uint256.Int, nonce uint64) accounts.Account { if read == nil { - return &accounts.Account{Balance: balance, Nonce: nonce, CodeHash: accounts.EmptyCodeHash} + return accounts.Account{Balance: balance, Nonce: nonce, CodeHash: accounts.EmptyCodeHash} } - return &accounts.Account{Balance: balance, Nonce: read.Nonce, Incarnation: read.Incarnation, CodeHash: read.CodeHash} + return accounts.Account{Balance: balance, Nonce: read.Nonce, Incarnation: read.Incarnation, CodeHash: read.CodeHash} } func (result *execResult) finalizeTx( @@ -2447,6 +2464,16 @@ func (be *blockExecutor) invalidBlockResult(err error) *blockResult { } } +// creditedWrites returns ws when an earlier fee merge produced it for tx, which +// makes it the one set already carrying the tx's fee credit. Anything else is +// the worker's own output, which calcFees reads as the pre-credit balance. +func (be *blockExecutor) creditedWrites(tx int, ws *state.WriteSet) *state.WriteSet { + if temp := be.feeMergeTemp[tx]; temp != nil && temp == ws { + return ws + } + return nil +} + // recordFeeMerge takes ownership of the set the fee merge just recorded for tx // and reclaims the one it superseded. Only a set an earlier fee merge created // may be released: prev is otherwise some execResult's TxOut, which stays live. @@ -2722,12 +2749,13 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r stateReader = state.NewCurrentCachedReaderV3(pe.rs.Domains().AsGetter(applyTx), be.blockStateCache) } } - tipWrites, err := txResult.calcFees(taskVer, be.versionMap, stateReader, txTask.Rules()) + existingWrites := be.blockIO.WriteSet(txVersion.TxIndex) + tipWrites, err := txResult.calcFees(taskVer, be.versionMap, stateReader, txTask.Rules(), + be.creditedWrites(tx, existingWrites)) if err != nil { return nil, err } if !tipWrites.IsEmpty() { - existingWrites := be.blockIO.WriteSet(txVersion.TxIndex) merged := existingWrites.MergeInto(tipWrites) be.blockIO.RecordWrites(txVersion, merged) be.recordFeeMerge(tx, existingWrites, merged) From 2b25b1adf31e5dda17bd0d02fb0fc18ad5524479 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Mon, 10 Aug 2026 00:10:49 +0700 Subject: [PATCH 2/5] execution/stagedsync: address review of the fee-credit skip Maintain CollectorWrites before the skip so the skipping and emitting paths leave the same state behind for the round. feeEntry.emit is now the only gate on whether an entry is written, the emptied-coinbase predicate is named once instead of spelled out twice, and the methods take a pointer so the entry is not copied per call. The benchmark never returned the emitted set, so the pools it checks maps out of stayed cold and the emit arm was measured against an allocation the apply loop does not pay. Releasing the maps each iteration puts the baseline at 202ns/544B/6 allocs, so the redundant round saves 54%, not 69%. Tests: the changed-balance case now mutates only the balance, the London case uses londonTransferScenario instead of rebuilding an inconsistent one, and a new test drives the re-execution invalidation through VersionedIO rather than hand-placed feeMergeTemp entries. --- execution/stagedsync/exec3_fee_credit_test.go | 42 +++++++++++++++---- execution/stagedsync/exec3_parallel.go | 40 ++++++++++-------- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/execution/stagedsync/exec3_fee_credit_test.go b/execution/stagedsync/exec3_fee_credit_test.go index c9976f560e3..347ddf152ed 100644 --- a/execution/stagedsync/exec3_fee_credit_test.go +++ b/execution/stagedsync/exec3_fee_credit_test.go @@ -8,7 +8,6 @@ import ( "github.com/erigontech/erigon/execution/chain" "github.com/erigontech/erigon/execution/state" - "github.com/erigontech/erigon/execution/types/accounts" ) // feeCreditRound mirrors one apply-loop validation round for a single tx: @@ -80,8 +79,10 @@ func TestCalcFees_ReCreditsWhenPriorBalanceChanged(t *testing.T) { require.NotNil(t, r.run(t), "the first round must credit the tip") // A prior tx moved the coinbase balance, so the tip lands on a new base. + // Only the balance changes — the rest of the account must stay put, or the + // test would pass even if recordedIn stopped comparing balances. priorBalance := uint256.NewInt(7_000_000) - r.reader.accounts[s.coinbase] = &accounts.Account{Balance: *priorBalance, CodeHash: accounts.EmptyCodeHash} + r.reader.accounts[s.coinbase].Balance = *priorBalance tip := r.run(t) require.NotNil(t, tip, "a changed base balance must produce a fresh credit") @@ -114,12 +115,7 @@ func TestCalcFees_ReCreditsWhenAddressPathMissing(t *testing.T) { func TestCalcFees_SkipsRedundantReCreditWithBurntContract(t *testing.T) { t.Parallel() - s := simpleTransferScenario() - s.rules = &chain.Rules{IsSpuriousDragon: true, IsLondon: true} - s.burntAddr = fAddr("burntcontract") - s.feeBurnt = *uint256.NewInt(1000) - s.accts[s.burntAddr] = fMakeAccount(500_000, 0) - + s := londonTransferScenario() r := newFeeCreditRound(t, s) first := r.run(t) @@ -156,6 +152,10 @@ func BenchmarkCalcFees(b *testing.B) { b.Fatal(err) } feeCreditSink = tip + // The apply loop recycles the emitted set's maps through + // recordFeeMerge; without this the pools stay empty and the + // emit arm is measured against a permanently cold pool. + tip.ReleaseMaps() } }) } @@ -178,3 +178,29 @@ func TestCreditedWrites(t *testing.T) { require.Nil(t, be.creditedWrites(2, nil), "a tx with no writes at all must not read as credited") } + +// TestCreditedWritesAfterReExecution drives the property the skip rests on +// through the real VersionedIO: a new worker result re-records its own TxOut, +// which stops the fee-merge product from being the recorded set. Without that, +// a re-executed tx would inherit the previous incarnation's credit. +func TestCreditedWritesAfterReExecution(t *testing.T) { + t.Parallel() + be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}, blockIO: state.NewVersionedIO(1)} + version := state.Version{TxIndex: 0} + recorded := func() *state.WriteSet { return be.blockIO.WriteSet(version.TxIndex) } + + txOut := &state.WriteSet{} + be.blockIO.RecordWrites(version, txOut) + require.Nil(t, be.creditedWrites(0, recorded()), + "the worker's own output carries no credit") + + merged := &state.WriteSet{} + be.blockIO.RecordWrites(version, merged) + be.recordFeeMerge(0, txOut, merged) + require.Same(t, merged, be.creditedWrites(0, recorded())) + + reTxOut := &state.WriteSet{} + be.blockIO.RecordWrites(version, reTxOut) + require.Nil(t, be.creditedWrites(0, recorded()), + "a re-executed tx must be credited again, not handed the stale credit") +} diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index d3688fa5000..1cd65a42e11 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -2003,18 +2003,31 @@ func (result *execResult) calcFees( // and normalizeWriteSet's sdSet filter drops them. coinbaseEmptyPre := (coinbaseAcc == nil || coinbaseAcc.Balance.IsZero()) && coinbaseNonce == 0 && coinbaseEmptyCodeHash && !coinbaseHasCodeHashWrite - emitCoinbase := newCoinbaseBalance != oldCoinbaseBalance || - (coinbaseEmptyRemoval && coinbaseEmptyPre && newCoinbaseBalance.IsZero()) + coinbaseEmptied := coinbaseEmptyRemoval && coinbaseEmptyPre && newCoinbaseBalance.IsZero() + emitCoinbase := newCoinbaseBalance != oldCoinbaseBalance || coinbaseEmptied if !emitCoinbase && !emitBurnt { return nil, nil } + // CollectorWrites is maintained before the skip below so that both paths + // leave the same state behind for the round. + if emitCoinbase { + result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( + result.Coinbase, coinbaseAcc, newCoinbaseBalance, + tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) + } + if emitBurnt { + result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( + burntAddr, burntAcc, newBurntBalance, + tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) + } + coinbaseEntry := feeEntry{ addr: result.Coinbase, acc: feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce), reason: tracing.BalanceIncreaseRewardTransactionFee, - deleted: coinbaseEmptyRemoval && coinbaseEmptyPre && newCoinbaseBalance.IsZero(), + deleted: coinbaseEmptied, emit: emitCoinbase, } burntEntry := feeEntry{ @@ -2031,18 +2044,8 @@ func (result *execResult) calcFees( } addWrites := &state.WriteSet{} - if emitCoinbase { - result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( - result.Coinbase, coinbaseAcc, newCoinbaseBalance, - tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) - coinbaseEntry.writeTo(addWrites, taskVersion) - } - if emitBurnt { - result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( - burntAddr, burntAcc, newBurntBalance, - tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) - burntEntry.writeTo(addWrites, taskVersion) - } + coinbaseEntry.writeTo(addWrites, taskVersion) + burntEntry.writeTo(addWrites, taskVersion) return addWrites, nil } @@ -2059,7 +2062,7 @@ type feeEntry struct { } // recordedIn reports whether ws already carries this entry verbatim. -func (e feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { +func (e *feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { if !e.emit { return true } @@ -2075,7 +2078,10 @@ func (e feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { return ok && aw.Val != nil && *aw.Val == e.acc && aw.Version == version } -func (e feeEntry) writeTo(ws *state.WriteSet, version state.Version) { +func (e *feeEntry) writeTo(ws *state.WriteSet, version state.Version) { + if !e.emit { + return + } if e.deleted { ws.SetSelfDestruct(e.addr, &state.VersionedWrite[bool]{ WriteHeader: state.WriteHeader{Address: e.addr, Path: state.SelfDestructPath, Version: version}, From b5e28320744bc047df2558ebc7be8d02317bd864 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Tue, 11 Aug 2026 09:34:46 +0700 Subject: [PATCH 3/5] execution/stagedsync: drop the fee credit with the result it belonged to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit recordWorkerWrites replaces the two RecordWrites calls in the result path so that installing a new worker output also clears feeMergeTemp[tx]. The skip no longer relies on the recorded-set pointer changing underneath it to notice a re-execution; provenance is dropped where it is lost. Move the CollectorWrites updates back below the no-op check and build each feeEntry only when it is emitted. An unchanged credit leaves CollectorWrites holding the identical values an earlier round put there, so maintaining it on a skipped round is pure cost — and for an EIP-161 emptied coinbase SetAccountBalanceOrDelete allocates a VersionedWrite every call. Cover the two paths that had none: the EIP-161 delete, and recordedIn against writeTo directly, so the pair cannot drift into a permanent skip or a permanent re-credit without a test failing. BenchmarkCalcFees, 200000x n=6: redundant_recredit 94ns -> 87ns against a 206ns first credit. --- execution/stagedsync/exec3_fee_credit_test.go | 65 +++++++++++++++++++ execution/stagedsync/exec3_parallel.go | 62 +++++++++++------- 2 files changed, 102 insertions(+), 25 deletions(-) diff --git a/execution/stagedsync/exec3_fee_credit_test.go b/execution/stagedsync/exec3_fee_credit_test.go index 347ddf152ed..ce026315759 100644 --- a/execution/stagedsync/exec3_fee_credit_test.go +++ b/execution/stagedsync/exec3_fee_credit_test.go @@ -8,6 +8,8 @@ import ( "github.com/erigontech/erigon/execution/chain" "github.com/erigontech/erigon/execution/state" + "github.com/erigontech/erigon/execution/tracing" + "github.com/erigontech/erigon/execution/types/accounts" ) // feeCreditRound mirrors one apply-loop validation round for a single tx: @@ -126,6 +128,69 @@ func TestCalcFees_SkipsRedundantReCreditWithBurntContract(t *testing.T) { "both halves of the credit are already recorded, so the round is a no-op") } +func TestCalcFees_SkipsRedundantReCreditOnEmptyRemoval(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + // A zero tip on an already-empty coinbase is the EIP-161 case: the credit + // is a delete rather than a balance write, and takes a different path + // through both recordedIn and writeTo. + s.feeTipped = uint256.Int{} + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "an emptied coinbase must still be touched") + sd, ok := first.GetSelfDestruct(s.coinbase) + require.True(t, ok, "the credit is a SelfDestructPath delete") + require.True(t, sd.Val) + + require.Nil(t, r.run(t), + "the delete is already recorded, so the round is a no-op") +} + +// TestFeeEntryWriteToIsRecordedIn pins the two halves against each other: if +// recordedIn stops accepting what writeTo produces the skip silently never +// fires, and if it accepts more the credit can be skipped without ever having +// been written. +func TestFeeEntryWriteToIsRecordedIn(t *testing.T) { + t.Parallel() + version := state.Version{TxIndex: 3, Incarnation: 1} + addr := fAddr("credited") + + entries := []feeEntry{ + { + addr: addr, + acc: accounts.Account{Balance: *uint256.NewInt(7), Nonce: 2, Incarnation: 1, CodeHash: accounts.EmptyCodeHash}, + reason: tracing.BalanceIncreaseRewardTransactionFee, + emit: true, + }, + { + addr: addr, + acc: accounts.Account{Balance: *uint256.NewInt(11), CodeHash: accounts.EmptyCodeHash}, + reason: tracing.BalanceDecreaseGasBuy, + emit: true, + }, + { + addr: addr, + reason: tracing.BalanceIncreaseRewardTransactionFee, + deleted: true, + emit: true, + }, + } + + for i := range entries { + e := &entries[i] + ws := &state.WriteSet{} + e.writeTo(ws, version) + + require.True(t, e.recordedIn(ws, version), + "entry %d: recordedIn must accept what writeTo wrote", i) + require.False(t, e.recordedIn(ws, state.Version{TxIndex: 3, Incarnation: 2}), + "entry %d: a credit stamped at another incarnation is not this credit", i) + require.False(t, e.recordedIn(&state.WriteSet{}, version), + "entry %d: an empty set carries no credit", i) + } +} + var feeCreditSink *state.WriteSet func BenchmarkCalcFees(b *testing.B) { diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 1cd65a42e11..54a0c311e14 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -2010,39 +2010,43 @@ func (result *execResult) calcFees( return nil, nil } - // CollectorWrites is maintained before the skip below so that both paths - // leave the same state behind for the round. + var coinbaseEntry, burntEntry feeEntry if emitCoinbase { - result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( - result.Coinbase, coinbaseAcc, newCoinbaseBalance, - tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) + coinbaseEntry = feeEntry{ + addr: result.Coinbase, + acc: feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce), + reason: tracing.BalanceIncreaseRewardTransactionFee, + deleted: coinbaseEmptied, + emit: true, + } } if emitBurnt { - result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( - burntAddr, burntAcc, newBurntBalance, - tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) - } - - coinbaseEntry := feeEntry{ - addr: result.Coinbase, - acc: feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce), - reason: tracing.BalanceIncreaseRewardTransactionFee, - deleted: coinbaseEmptied, - emit: emitCoinbase, - } - burntEntry := feeEntry{ - addr: burntAddr, - acc: feeAddressAccount(burntAcc, newBurntBalance, 0), - reason: tracing.BalanceDecreaseGasBuy, - emit: emitBurnt, + burntEntry = feeEntry{ + addr: burntAddr, + acc: feeAddressAccount(burntAcc, newBurntBalance, 0), + reason: tracing.BalanceDecreaseGasBuy, + emit: true, + } } // The apply loop re-credits a tx once per validation round, and the credit // only moves when a prior tx's writes moved under it. An unchanged credit - // would rebuild a set identical to the one already recorded. + // would rebuild a set identical to the one already recorded — including the + // CollectorWrites entries an earlier round already put there. if coinbaseEntry.recordedIn(credited, taskVersion) && burntEntry.recordedIn(credited, taskVersion) { return nil, nil } + if coinbaseEntry.emit { + result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( + result.Coinbase, coinbaseAcc, newCoinbaseBalance, + tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) + } + if burntEntry.emit { + result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( + burntAddr, burntAcc, newBurntBalance, + tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) + } + addWrites := &state.WriteSet{} coinbaseEntry.writeTo(addWrites, taskVersion) burntEntry.writeTo(addWrites, taskVersion) @@ -2470,6 +2474,14 @@ func (be *blockExecutor) invalidBlockResult(err error) *blockResult { } } +// recordWorkerWrites installs a worker result's write set and drops the fee +// credit along with it: the credit belonged to the incarnation this result +// supersedes, and crediting is what the apply loop does next. +func (be *blockExecutor) recordWorkerWrites(tx int, txVersion state.Version, writes *state.WriteSet) { + be.blockIO.RecordWrites(txVersion, writes) + delete(be.feeMergeTemp, tx) +} + // creditedWrites returns ws when an earlier fee merge produced it for tx, which // makes it the one set already carrying the tx's fee credit. Anything else is // the worker's own output, which calcFees reads as the pre-credit balance. @@ -2666,7 +2678,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r be.blockIO.RecordAccesses(txVersion, res.AccessedAddresses) if res.Version().Incarnation == 0 { - be.blockIO.RecordWrites(txVersion, res.TxOut) + be.recordWorkerWrites(tx, txVersion, res.TxOut) } else { prevWrites := be.blockIO.WriteSet(txVersion.TxIndex) hasWriteChange := res.TxOut.HasNewWrite(prevWrites) @@ -2680,7 +2692,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r } } - be.blockIO.RecordWrites(txVersion, res.TxOut) + be.recordWorkerWrites(tx, txVersion, res.TxOut) if hasWriteChange { be.validateTasks.pushPendingSet(be.execTasks.getRevalidationRange(tx + 1)) From d85b484fe7d51a6386f5f915a89ee82cdb998f95 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Tue, 11 Aug 2026 18:40:11 +0700 Subject: [PATCH 4/5] execution/stagedsync: reclaim the displaced credit, pin it to its incarnation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit recordWorkerWrites dropped the fee-merge product from feeMergeTemp without reclaiming it, so every re-execution of an already-credited tx sent the merged set's pooled maps to GC. feeMergeTemp now carries the version each credit was computed for, so a stale credit cannot be handed to calcFees even if it outlived the recorded set it lived in — the CollectorWrites entries an earlier round wrote belong to that same incarnation. recordFeeMerge does the MergeInto and the RecordWrites itself, which keeps the merge product and the tx's recorded write set from drifting apart and makes the "a temp is never an execResult's TxOut" ownership rule local to the one function that populates the map. The fee-credit tests move into exec3_finalize_test.go and exec3_fee_merge_temp_test.go, next to the fixtures and the blockExecutor white-box tests they belong with. --- execution/stagedsync/exec3_fee_credit_test.go | 271 ------------------ .../stagedsync/exec3_fee_merge_temp_test.go | 159 ++++++++-- execution/stagedsync/exec3_finalize_test.go | 234 ++++++++++++++- execution/stagedsync/exec3_parallel.go | 75 +++-- 4 files changed, 400 insertions(+), 339 deletions(-) delete mode 100644 execution/stagedsync/exec3_fee_credit_test.go diff --git a/execution/stagedsync/exec3_fee_credit_test.go b/execution/stagedsync/exec3_fee_credit_test.go deleted file mode 100644 index ce026315759..00000000000 --- a/execution/stagedsync/exec3_fee_credit_test.go +++ /dev/null @@ -1,271 +0,0 @@ -package stagedsync - -import ( - "testing" - - "github.com/holiman/uint256" - "github.com/stretchr/testify/require" - - "github.com/erigontech/erigon/execution/chain" - "github.com/erigontech/erigon/execution/state" - "github.com/erigontech/erigon/execution/tracing" - "github.com/erigontech/erigon/execution/types/accounts" -) - -// feeCreditRound mirrors one apply-loop validation round for a single tx: -// calcFees derives the credit, and a non-empty result is folded into the -// recorded write set the way nextResult does it. -type feeCreditRound struct { - result *execResult - task *taskVersion - vm *state.VersionMap - reader *mapStateReader - rules *chain.Rules - recorded *state.WriteSet - // credited tracks the recorded set once a fee merge produced it, which is - // blockExecutor.feeMergeTemp's job in the apply loop. - credited *state.WriteSet -} - -func newFeeCreditRound(t testing.TB, s *testFinalizeScenario) *feeCreditRound { - t.Helper() - - result := s.buildExecResult() - result.TxIn = copyReadSet(s.txIn) - result.TxOut = copyWrites(s.txOut) - result.CollectorWrites = copyWrites(s.collectorWrites) - - vm := state.NewVersionMap(nil) - vm.FlushVersionedWrites(result.TxOut, true, "") - - return &feeCreditRound{ - result: result, - task: result.Task.(*taskVersion), - vm: vm, - reader: s.makeReader(), - rules: s.rules, - recorded: result.TxOut, - } -} - -// run performs one round and returns the credit calcFees produced, nil when it -// found the recorded set already carries it. -func (r *feeCreditRound) run(t testing.TB) *state.WriteSet { - t.Helper() - - tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, r.credited) - require.NoError(t, err) - if tip.IsEmpty() { - return nil - } - r.recorded = r.recorded.MergeInto(tip) - r.credited = r.recorded - return tip -} - -func TestCalcFees_SkipsRedundantReCredit(t *testing.T) { - t.Parallel() - r := newFeeCreditRound(t, simpleTransferScenario()) - - require.NotNil(t, r.run(t), "the first round must credit the tip") - require.Nil(t, r.run(t), - "re-crediting a set that already carries this exact credit rebuilds an identical "+ - "write set and re-runs the merge for nothing") -} - -func TestCalcFees_ReCreditsWhenPriorBalanceChanged(t *testing.T) { - t.Parallel() - s := simpleTransferScenario() - r := newFeeCreditRound(t, s) - - require.NotNil(t, r.run(t), "the first round must credit the tip") - - // A prior tx moved the coinbase balance, so the tip lands on a new base. - // Only the balance changes — the rest of the account must stay put, or the - // test would pass even if recordedIn stopped comparing balances. - priorBalance := uint256.NewInt(7_000_000) - r.reader.accounts[s.coinbase].Balance = *priorBalance - - tip := r.run(t) - require.NotNil(t, tip, "a changed base balance must produce a fresh credit") - - credited := findBalance(tip, s.coinbase) - require.NotNil(t, credited) - require.Equal(t, *new(uint256.Int).Add(priorBalance, &s.feeTipped), credited.Val) -} - -func TestCalcFees_ReCreditsWhenAddressPathMissing(t *testing.T) { - t.Parallel() - s := simpleTransferScenario() - r := newFeeCreditRound(t, s) - - first := r.run(t) - require.NotNil(t, first, "the first round must credit the tip") - - // A half-recorded credit is not a credit: the balance alone leaves - // downstream reads without an account record. - balanceOnly := &state.WriteSet{} - bw, ok := first.GetBalance(s.coinbase) - require.True(t, ok) - balanceOnly.SetBalance(s.coinbase, bw) - - tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, balanceOnly) - require.NoError(t, err) - require.False(t, tip.IsEmpty(), "a recorded balance without its AddressPath sibling must be re-credited") - require.NotNil(t, findAddress(tip, s.coinbase)) -} - -func TestCalcFees_SkipsRedundantReCreditWithBurntContract(t *testing.T) { - t.Parallel() - s := londonTransferScenario() - r := newFeeCreditRound(t, s) - - first := r.run(t) - require.NotNil(t, first, "the first round must credit the tip") - require.NotNil(t, findBalance(first, s.burntAddr), "London burns to the burnt contract") - - require.Nil(t, r.run(t), - "both halves of the credit are already recorded, so the round is a no-op") -} - -func TestCalcFees_SkipsRedundantReCreditOnEmptyRemoval(t *testing.T) { - t.Parallel() - s := simpleTransferScenario() - // A zero tip on an already-empty coinbase is the EIP-161 case: the credit - // is a delete rather than a balance write, and takes a different path - // through both recordedIn and writeTo. - s.feeTipped = uint256.Int{} - r := newFeeCreditRound(t, s) - - first := r.run(t) - require.NotNil(t, first, "an emptied coinbase must still be touched") - sd, ok := first.GetSelfDestruct(s.coinbase) - require.True(t, ok, "the credit is a SelfDestructPath delete") - require.True(t, sd.Val) - - require.Nil(t, r.run(t), - "the delete is already recorded, so the round is a no-op") -} - -// TestFeeEntryWriteToIsRecordedIn pins the two halves against each other: if -// recordedIn stops accepting what writeTo produces the skip silently never -// fires, and if it accepts more the credit can be skipped without ever having -// been written. -func TestFeeEntryWriteToIsRecordedIn(t *testing.T) { - t.Parallel() - version := state.Version{TxIndex: 3, Incarnation: 1} - addr := fAddr("credited") - - entries := []feeEntry{ - { - addr: addr, - acc: accounts.Account{Balance: *uint256.NewInt(7), Nonce: 2, Incarnation: 1, CodeHash: accounts.EmptyCodeHash}, - reason: tracing.BalanceIncreaseRewardTransactionFee, - emit: true, - }, - { - addr: addr, - acc: accounts.Account{Balance: *uint256.NewInt(11), CodeHash: accounts.EmptyCodeHash}, - reason: tracing.BalanceDecreaseGasBuy, - emit: true, - }, - { - addr: addr, - reason: tracing.BalanceIncreaseRewardTransactionFee, - deleted: true, - emit: true, - }, - } - - for i := range entries { - e := &entries[i] - ws := &state.WriteSet{} - e.writeTo(ws, version) - - require.True(t, e.recordedIn(ws, version), - "entry %d: recordedIn must accept what writeTo wrote", i) - require.False(t, e.recordedIn(ws, state.Version{TxIndex: 3, Incarnation: 2}), - "entry %d: a credit stamped at another incarnation is not this credit", i) - require.False(t, e.recordedIn(&state.WriteSet{}, version), - "entry %d: an empty set carries no credit", i) - } -} - -var feeCreditSink *state.WriteSet - -func BenchmarkCalcFees(b *testing.B) { - for _, bc := range []struct { - name string - recredit bool - }{ - {"first_credit", false}, - {"redundant_recredit", true}, - } { - b.Run(bc.name, func(b *testing.B) { - r := newFeeCreditRound(b, simpleTransferScenario()) - var credited *state.WriteSet - if bc.recredit { - require.NotNil(b, r.run(b)) - credited = r.credited - } - - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, credited) - if err != nil { - b.Fatal(err) - } - feeCreditSink = tip - // The apply loop recycles the emitted set's maps through - // recordFeeMerge; without this the pools stay empty and the - // emit arm is measured against a permanently cold pool. - tip.ReleaseMaps() - } - }) - } -} - -func TestCreditedWrites(t *testing.T) { - t.Parallel() - be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}} - txOut, merged := &state.WriteSet{}, &state.WriteSet{} - - require.Nil(t, be.creditedWrites(0, txOut), - "before any fee merge the recorded set is the worker's own output") - - be.recordFeeMerge(0, txOut, merged) - require.Same(t, merged, be.creditedWrites(0, merged)) - require.Nil(t, be.creditedWrites(0, txOut), - "a re-execution re-records the worker's TxOut, which carries no credit") - require.Nil(t, be.creditedWrites(1, merged), - "another tx's fee-merge product says nothing about this tx") - require.Nil(t, be.creditedWrites(2, nil), - "a tx with no writes at all must not read as credited") -} - -// TestCreditedWritesAfterReExecution drives the property the skip rests on -// through the real VersionedIO: a new worker result re-records its own TxOut, -// which stops the fee-merge product from being the recorded set. Without that, -// a re-executed tx would inherit the previous incarnation's credit. -func TestCreditedWritesAfterReExecution(t *testing.T) { - t.Parallel() - be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}, blockIO: state.NewVersionedIO(1)} - version := state.Version{TxIndex: 0} - recorded := func() *state.WriteSet { return be.blockIO.WriteSet(version.TxIndex) } - - txOut := &state.WriteSet{} - be.blockIO.RecordWrites(version, txOut) - require.Nil(t, be.creditedWrites(0, recorded()), - "the worker's own output carries no credit") - - merged := &state.WriteSet{} - be.blockIO.RecordWrites(version, merged) - be.recordFeeMerge(0, txOut, merged) - require.Same(t, merged, be.creditedWrites(0, recorded())) - - reTxOut := &state.WriteSet{} - be.blockIO.RecordWrites(version, reTxOut) - require.Nil(t, be.creditedWrites(0, recorded()), - "a re-executed tx must be credited again, not handed the stale credit") -} diff --git a/execution/stagedsync/exec3_fee_merge_temp_test.go b/execution/stagedsync/exec3_fee_merge_temp_test.go index 942ae076439..55deb3f18af 100644 --- a/execution/stagedsync/exec3_fee_merge_temp_test.go +++ b/execution/stagedsync/exec3_fee_merge_temp_test.go @@ -27,7 +27,7 @@ import ( "github.com/erigontech/erigon/execution/types/accounts" ) -func feeMergeTestWrites(t *testing.T, addr accounts.Address, balance uint64) *state.WriteSet { +func feeMergeTestWrites(t testing.TB, addr accounts.Address, balance uint64) *state.WriteSet { t.Helper() ws := &state.WriteSet{} ws.SetBalance(addr, &state.VersionedWrite[uint256.Int]{ @@ -37,6 +37,15 @@ func feeMergeTestWrites(t *testing.T, addr accounts.Address, balance uint64) *st return ws } +func feeMergeTestExecutor(t testing.TB) *blockExecutor { + t.Helper() + return &blockExecutor{feeMergeTemp: map[int]feeMerge{}, blockIO: state.NewVersionedIO(2)} +} + +func feeMergeTestAddr(hex string) accounts.Address { + return accounts.InternAddress(common.HexToAddress(hex)) +} + // TestRecordFeeMergeReleasesSupersededTemp pins the three transitions the fee // merge goes through for one tx: the first merge has no temp to reclaim, a // revalidation round reclaims the temp it replaces, and a round whose input is @@ -45,34 +54,111 @@ func feeMergeTestWrites(t *testing.T, addr accounts.Address, balance uint64) *st func TestRecordFeeMergeReleasesSupersededTemp(t *testing.T) { t.Parallel() - addr := accounts.InternAddress(common.HexToAddress("0x1111111111111111111111111111111111111111")) - be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}} + addr := feeMergeTestAddr("0x1111111111111111111111111111111111111111") + be := feeMergeTestExecutor(t) + version := state.Version{TxIndex: 0} // First round: prev is the worker's TxOut, so nothing may be released. txOut := feeMergeTestWrites(t, addr, 1) - temp1 := feeMergeTestWrites(t, addr, 2) - be.recordFeeMerge(0, txOut, temp1) - require.Same(t, temp1, be.feeMergeTemp[0]) + tip1 := feeMergeTestWrites(t, addr, 2) + be.recordFeeMerge(version, txOut, tip1) + require.Same(t, tip1, be.feeMergeTemp[0].writes) + require.Same(t, tip1, be.blockIO.WriteSet(version.TxIndex), + "the merge product must be the tx's recorded write set") require.Equal(t, 1, txOut.Count(), "TxOut must survive the fee merge") // Revalidation round: prev is the temp the first round recorded, so it is // superseded and reclaimed. - temp2 := feeMergeTestWrites(t, addr, 3) - be.recordFeeMerge(0, temp1, temp2) + tip2 := feeMergeTestWrites(t, addr, 3) + be.recordFeeMerge(version, tip1, tip2) be.awaitMapReleases() - require.Same(t, temp2, be.feeMergeTemp[0]) - require.Equal(t, 0, temp1.Count(), "superseded fee-merge temp must be released") - require.Equal(t, 1, temp2.Count()) + require.Same(t, tip2, be.feeMergeTemp[0].writes) + require.Equal(t, 0, tip1.Count(), "superseded fee-merge temp must be released") + require.Equal(t, 1, tip2.Count()) // After a re-execution the recorded slot is the new TxOut again, so the // stale temp does not match prev and stays untouched. txOut2 := feeMergeTestWrites(t, addr, 4) - temp3 := feeMergeTestWrites(t, addr, 5) - be.recordFeeMerge(0, txOut2, temp3) + tip3 := feeMergeTestWrites(t, addr, 5) + be.recordFeeMerge(version, txOut2, tip3) be.awaitMapReleases() - require.Same(t, temp3, be.feeMergeTemp[0]) + require.Same(t, tip3, be.feeMergeTemp[0].writes) require.Equal(t, 1, txOut2.Count(), "TxOut must survive the fee merge") - require.Equal(t, 1, temp2.Count(), "a temp that is not prev must not be released") + require.Equal(t, 1, tip2.Count(), "a temp that is not prev must not be released") +} + +// TestRecordFeeMergeSkipsEmptyTip covers the round the skip produces: with no +// credit to fold in, the recorded set stays the worker's own output — which +// calcFees must keep reading as the pre-credit balance. +func TestRecordFeeMergeSkipsEmptyTip(t *testing.T) { + t.Parallel() + + addr := feeMergeTestAddr("0x5555555555555555555555555555555555555555") + be := feeMergeTestExecutor(t) + version := state.Version{TxIndex: 0} + + txOut := feeMergeTestWrites(t, addr, 1) + be.recordWorkerWrites(version, txOut) + be.recordFeeMerge(version, txOut, nil) + + require.Same(t, txOut, be.blockIO.WriteSet(version.TxIndex)) + require.Nil(t, be.creditedWrites(version, txOut), + "a skipped credit must not mark the worker's TxOut as carrying one") +} + +// TestRecordWorkerWritesDropsCreditedTemp drives the re-execution path through +// recordWorkerWrites: the new TxOut displaces the credited set, so the credit +// must be gone and the set it lived in reclaimed. +func TestRecordWorkerWritesDropsCreditedTemp(t *testing.T) { + t.Parallel() + + addr := feeMergeTestAddr("0x4444444444444444444444444444444444444444") + be := feeMergeTestExecutor(t) + version := state.Version{TxIndex: 0} + + txOut := feeMergeTestWrites(t, addr, 1) + be.recordWorkerWrites(version, txOut) + require.Nil(t, be.creditedWrites(version, be.blockIO.WriteSet(version.TxIndex)), + "the worker's own output carries no credit") + + tip := feeMergeTestWrites(t, addr, 2) + be.recordFeeMerge(version, txOut, tip) + require.Same(t, tip, be.creditedWrites(version, be.blockIO.WriteSet(version.TxIndex))) + + reTxOut := feeMergeTestWrites(t, addr, 3) + be.recordWorkerWrites(version, reTxOut) + be.awaitMapReleases() + + require.Nil(t, be.creditedWrites(version, be.blockIO.WriteSet(version.TxIndex)), + "a re-executed tx must be credited again, not handed the stale credit") + require.Equal(t, 0, tip.Count(), "the displaced fee-merge temp must be released") + require.Equal(t, 1, reTxOut.Count(), "the new TxOut must survive") +} + +// TestCreditedWritesPinsVersion pins the credit to the incarnation it was +// computed for, so a re-executed tx cannot inherit it even if the set it lives +// in is still the recorded one. +func TestCreditedWritesPinsVersion(t *testing.T) { + t.Parallel() + + addr := feeMergeTestAddr("0x6666666666666666666666666666666666666666") + be := feeMergeTestExecutor(t) + version := state.Version{TxIndex: 0} + + tip := feeMergeTestWrites(t, addr, 2) + be.recordFeeMerge(version, feeMergeTestWrites(t, addr, 1), tip) + require.Same(t, tip, be.creditedWrites(version, tip)) + + reExecuted := version + reExecuted.Incarnation = 1 + require.Nil(t, be.creditedWrites(reExecuted, tip), + "a credit computed for an earlier incarnation is not this incarnation's") + + otherTx := state.Version{TxIndex: 1} + require.Nil(t, be.creditedWrites(otherTx, tip), + "another tx's fee-merge product says nothing about this tx") + require.Nil(t, be.creditedWrites(version, nil), + "a tx with no writes at all must not read as credited") } // TestRecordFeeMergeReleaseKeepsSharedWrites pins what makes the release safe: @@ -81,21 +167,48 @@ func TestRecordFeeMergeReleasesSupersededTemp(t *testing.T) { func TestRecordFeeMergeReleaseKeepsSharedWrites(t *testing.T) { t.Parallel() - shared := accounts.InternAddress(common.HexToAddress("0x2222222222222222222222222222222222222222")) - fresh := accounts.InternAddress(common.HexToAddress("0x3333333333333333333333333333333333333333")) - be := &blockExecutor{feeMergeTemp: map[int]*state.WriteSet{}} + shared := feeMergeTestAddr("0x2222222222222222222222222222222222222222") + fresh := feeMergeTestAddr("0x3333333333333333333333333333333333333333") + be := feeMergeTestExecutor(t) + version := state.Version{TxIndex: 0} temp1 := feeMergeTestWrites(t, shared, 7) - be.recordFeeMerge(0, feeMergeTestWrites(t, shared, 1), temp1) + be.recordFeeMerge(version, feeMergeTestWrites(t, shared, 1), temp1) tipWrites := feeMergeTestWrites(t, fresh, 9) - merged := temp1.MergeInto(tipWrites) - require.Same(t, tipWrites, merged) - be.recordFeeMerge(0, temp1, merged) + be.recordFeeMerge(version, temp1, tipWrites) be.awaitMapReleases() require.Equal(t, 0, temp1.Count()) - vw, ok := merged.GetBalance(shared) + require.Same(t, tipWrites, be.blockIO.WriteSet(version.TxIndex)) + vw, ok := tipWrites.GetBalance(shared) require.True(t, ok, "entry shared from the released temp must still be reachable") require.Equal(t, uint64(7), vw.Val.Uint64()) } + +// TestCalcFeesRoundThroughBlockExecutor runs the round the way the apply loop +// does — creditedWrites in, recordFeeMerge out — so the skip is exercised +// against the real feeMergeTemp bookkeeping rather than the fixture's stand-in. +func TestCalcFeesRoundThroughBlockExecutor(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + r := newFeeCreditRound(t, s) + be := feeMergeTestExecutor(t) + version := r.task.Version() + be.recordWorkerWrites(version, r.result.TxOut) + + round := func() *state.WriteSet { + recorded := be.blockIO.WriteSet(version.TxIndex) + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, be.creditedWrites(version, recorded)) + require.NoError(t, err) + be.recordFeeMerge(version, recorded, tip) + return tip + } + + first := round() + require.False(t, first.IsEmpty(), "the first round must credit the tip") + require.Same(t, first, be.blockIO.WriteSet(version.TxIndex)) + require.True(t, round().IsEmpty(), "the recorded set already carries this credit") + require.Same(t, first, be.blockIO.WriteSet(version.TxIndex), + "a skipped round must leave the recorded set alone") +} diff --git a/execution/stagedsync/exec3_finalize_test.go b/execution/stagedsync/exec3_finalize_test.go index a48f1ee736c..4c091237525 100644 --- a/execution/stagedsync/exec3_finalize_test.go +++ b/execution/stagedsync/exec3_finalize_test.go @@ -200,7 +200,8 @@ func (s *testFinalizeScenario) makeReader() *mapStateReader { return r } -// buildExecResult creates an execResult for testing. +// buildExecResult creates an execResult for testing, with the scenario's IO +// copied in so repeated runs off one scenario see identical inputs. func (s *testFinalizeScenario) buildExecResult() *execResult { blockNum := s.header.Number.Uint64() @@ -238,6 +239,9 @@ func (s *testFinalizeScenario) buildExecResult() *execResult { }, Coinbase: s.coinbase, } + txResult.TxIn = copyReadSet(s.txIn) + txResult.TxOut = copyWrites(s.txOut) + txResult.CollectorWrites = copyWrites(s.collectorWrites) return &execResult{TxResult: txResult} } @@ -543,11 +547,6 @@ func findAddress(writes *state.WriteSet, addr accounts.Address) *state.Versioned func (s *testFinalizeScenario) runFinalizeTx(t *testing.T, priorCoinbaseBalance *uint256.Int) *state.WriteSet { t.Helper() result := s.buildExecResult() - result.TxIn = copyReadSet(s.txIn) - result.TxOut = copyWrites(s.txOut) - if s.collectorWrites != nil { - result.CollectorWrites = copyWrites(s.collectorWrites) - } vm := state.NewVersionMap(nil) reader := s.makeReader() @@ -818,9 +817,6 @@ func TestFinalizeTxSimple_SenderIsCoinbase_AccumulatedAcrossTxs(t *testing.T) { build() result := s.buildExecResult() - result.TxIn = copyReadSet(s.txIn) - result.TxOut = copyWrites(s.txOut) - result.CollectorWrites = copyWrites(s.collectorWrites) // Set this tx's version explicitly so versionMap reads land on // the right tx-index for the floor-read semantics. @@ -876,9 +872,6 @@ func TestFinalizeTxSimple_SenderIsCoinbase_ReExecutedIncarnation(t *testing.T) { // Set incarnation > 0 on the task to reflect re-execution. result := s.buildExecResult() - result.TxIn = copyReadSet(s.txIn) - result.TxOut = copyWrites(s.txOut) - result.CollectorWrites = copyWrites(s.collectorWrites) task := result.Task.(*taskVersion) task.version.Incarnation = 1 // re-execution @@ -978,9 +971,6 @@ func TestFinalizeTxSimple_AccumulatedFees(t *testing.T) { for txIdx := 1; txIdx <= 3; txIdx++ { result := s.buildExecResult() - result.TxIn = copyReadSet(s.txIn) - result.TxOut = copyWrites(s.txOut) - result.CollectorWrites = copyWrites(s.collectorWrites) result.ExecutionResult.FeeTipped = *tipPerTx task := result.Task.(*taskVersion) @@ -1782,3 +1772,217 @@ func TestCalcFees_EmitsAddressPathForCoinbase(t *testing.T) { require.Equal(t, coinbaseBalance.WriteHeader.Version, coinbaseAddress.WriteHeader.Version, "AddressPath sibling must share version with the BalancePath write") } + +// feeCreditRound mirrors one apply-loop validation round for a single tx: +// calcFees derives the credit, and a non-empty result is folded into the +// recorded write set the way nextResult does it. +type feeCreditRound struct { + result *execResult + task *taskVersion + vm *state.VersionMap + reader *mapStateReader + rules *chain.Rules + recorded *state.WriteSet + // credited tracks the recorded set once a fee merge produced it, which is + // blockExecutor.feeMergeTemp's job in the apply loop. + credited *state.WriteSet +} + +func newFeeCreditRound(t testing.TB, s *testFinalizeScenario) *feeCreditRound { + t.Helper() + + result := s.buildExecResult() + + vm := state.NewVersionMap(nil) + vm.FlushVersionedWrites(result.TxOut, true, "") + + return &feeCreditRound{ + result: result, + task: result.Task.(*taskVersion), + vm: vm, + reader: s.makeReader(), + rules: s.rules, + recorded: result.TxOut, + } +} + +// run performs one round and returns the credit calcFees produced, nil when it +// found the recorded set already carries it. The returned set is a copy: the +// merge folds the recorded set into the credit in place, so the credit itself is +// only observable before it. +func (r *feeCreditRound) run(t testing.TB) *state.WriteSet { + t.Helper() + + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, r.credited) + require.NoError(t, err) + if tip.IsEmpty() { + return nil + } + credit := copyWrites(tip) + r.recorded = r.recorded.MergeInto(tip) + r.credited = r.recorded + return credit +} + +func TestCalcFees_SkipsRedundantReCredit(t *testing.T) { + t.Parallel() + r := newFeeCreditRound(t, simpleTransferScenario()) + + require.NotNil(t, r.run(t), "the first round must credit the tip") + require.Nil(t, r.run(t), + "re-crediting a set that already carries this exact credit rebuilds an identical "+ + "write set and re-runs the merge for nothing") +} + +func TestCalcFees_ReCreditsWhenPriorBalanceChanged(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + r := newFeeCreditRound(t, s) + + require.NotNil(t, r.run(t), "the first round must credit the tip") + + // A prior tx moved the coinbase balance, so the tip lands on a new base. + // Only the balance changes — the rest of the account must stay put, or the + // test would pass even if recordedIn stopped comparing balances. + priorBalance := uint256.NewInt(7_000_000) + r.reader.accounts[s.coinbase].Balance = *priorBalance + + tip := r.run(t) + require.NotNil(t, tip, "a changed base balance must produce a fresh credit") + + credited := findBalance(tip, s.coinbase) + require.NotNil(t, credited) + require.Equal(t, *new(uint256.Int).Add(priorBalance, &s.feeTipped), credited.Val) +} + +func TestCalcFees_ReCreditsWhenAddressPathMissing(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "the first round must credit the tip") + + // A half-recorded credit is not a credit: the balance alone leaves + // downstream reads without an account record. + balanceOnly := &state.WriteSet{} + bw, ok := first.GetBalance(s.coinbase) + require.True(t, ok) + balanceOnly.SetBalance(s.coinbase, bw) + + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, balanceOnly) + require.NoError(t, err) + require.False(t, tip.IsEmpty(), "a recorded balance without its AddressPath sibling must be re-credited") + require.NotNil(t, findAddress(tip, s.coinbase)) +} + +func TestCalcFees_SkipsRedundantReCreditWithBurntContract(t *testing.T) { + t.Parallel() + s := londonTransferScenario() + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "the first round must credit the tip") + require.NotNil(t, findBalance(first, s.burntAddr), "London burns to the burnt contract") + + require.Nil(t, r.run(t), + "both halves of the credit are already recorded, so the round is a no-op") +} + +func TestCalcFees_SkipsRedundantReCreditOnEmptyRemoval(t *testing.T) { + t.Parallel() + s := simpleTransferScenario() + // A zero tip on an already-empty coinbase is the EIP-161 case: the credit + // is a delete rather than a balance write, and takes a different path + // through both recordedIn and writeTo. + s.feeTipped = uint256.Int{} + r := newFeeCreditRound(t, s) + + first := r.run(t) + require.NotNil(t, first, "an emptied coinbase must still be touched") + sd, ok := first.GetSelfDestruct(s.coinbase) + require.True(t, ok, "the credit is a SelfDestructPath delete") + require.True(t, sd.Val) + + require.Nil(t, r.run(t), + "the delete is already recorded, so the round is a no-op") +} + +// TestFeeEntryWriteToIsRecordedIn pins the two halves against each other: if +// recordedIn stops accepting what writeTo produces the skip silently never +// fires, and if it accepts more the credit can be skipped without ever having +// been written. +func TestFeeEntryWriteToIsRecordedIn(t *testing.T) { + t.Parallel() + version := state.Version{TxIndex: 3, Incarnation: 1} + addr := fAddr("credited") + + entries := []feeEntry{ + { + addr: addr, + acc: accounts.Account{Balance: *uint256.NewInt(7), Nonce: 2, Incarnation: 1, CodeHash: accounts.EmptyCodeHash}, + reason: tracing.BalanceIncreaseRewardTransactionFee, + emit: true, + }, + { + addr: addr, + acc: accounts.Account{Balance: *uint256.NewInt(11), CodeHash: accounts.EmptyCodeHash}, + reason: tracing.BalanceDecreaseGasBuy, + emit: true, + }, + { + addr: addr, + reason: tracing.BalanceIncreaseRewardTransactionFee, + deleted: true, + emit: true, + }, + } + + for i := range entries { + e := &entries[i] + ws := &state.WriteSet{} + e.writeTo(ws, version) + + require.True(t, e.recordedIn(ws, version), + "entry %d: recordedIn must accept what writeTo wrote", i) + require.False(t, e.recordedIn(ws, state.Version{TxIndex: 3, Incarnation: 2}), + "entry %d: a credit stamped at another incarnation is not this credit", i) + require.False(t, e.recordedIn(&state.WriteSet{}, version), + "entry %d: an empty set carries no credit", i) + } +} + +var feeCreditSink *state.WriteSet + +func BenchmarkCalcFees(b *testing.B) { + for _, bc := range []struct { + name string + recredit bool + }{ + {"first_credit", false}, + {"redundant_recredit", true}, + } { + b.Run(bc.name, func(b *testing.B) { + r := newFeeCreditRound(b, simpleTransferScenario()) + var credited *state.WriteSet + if bc.recredit { + require.NotNil(b, r.run(b)) + credited = r.credited + } + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + tip, err := r.result.calcFees(r.task, r.vm, r.reader, r.rules, credited) + if err != nil { + b.Fatal(err) + } + feeCreditSink = tip + // The apply loop recycles the emitted set's maps through + // recordFeeMerge; without this the pools stay empty and the + // emit arm is measured against a permanently cold pool. + tip.ReleaseMaps() + } + }) + } +} diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 54a0c311e14..6f775a8c49a 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -2036,12 +2036,12 @@ func (result *execResult) calcFees( return nil, nil } - if coinbaseEntry.emit { + if emitCoinbase { result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( result.Coinbase, coinbaseAcc, newCoinbaseBalance, tracing.BalanceIncreaseRewardTransactionFee, coinbaseEmptyRemoval) } - if burntEntry.emit { + if emitBurnt { result.CollectorWrites = result.CollectorWrites.SetAccountBalanceOrDelete( burntAddr, burntAcc, newBurntBalance, tracing.BalanceDecreaseGasBuy, state.EIP161EmptyRemoval(chainRules.IsEIP161Enabled(), chainRules.IsAura, burntAddr)) @@ -2291,10 +2291,10 @@ type blockExecutor struct { tasks []*execTask results []*execResult - // feeMergeTemp[tx] is the write set the fee merge created and recorded for - // tx. A revalidation round merges again and supersedes it; every other + // feeMergeTemp[txIndex] is the write set the fee merge created and recorded + // for a tx. A revalidation round merges again and supersedes it; every other // recorded set is some execResult's TxOut, which stays live. - feeMergeTemp map[int]*state.WriteSet + feeMergeTemp map[int]feeMerge mapReleasing sync.WaitGroup @@ -2444,7 +2444,7 @@ func newBlockExec(blockNum uint64, blockHash common.Hash, gasPool *protocol.GasP begin: time.Now(), stats: map[int]ExecutionStat{}, finalizedResults: map[int]*execResult{}, - feeMergeTemp: map[int]*state.WriteSet{}, + feeMergeTemp: map[int]feeMerge{}, settledInput: map[int]bool{}, estimateDeps: map[int][]int{}, preValidated: map[int]bool{}, @@ -2474,34 +2474,53 @@ func (be *blockExecutor) invalidBlockResult(err error) *blockResult { } } +// feeMerge is a recorded fee-merge product pinned to the tx version whose +// credit it carries. The pin is what keeps a credit from outliving the +// incarnation it was computed for. +type feeMerge struct { + writes *state.WriteSet + version state.Version +} + // recordWorkerWrites installs a worker result's write set and drops the fee // credit along with it: the credit belonged to the incarnation this result -// supersedes, and crediting is what the apply loop does next. -func (be *blockExecutor) recordWorkerWrites(tx int, txVersion state.Version, writes *state.WriteSet) { +// supersedes, and crediting is what the apply loop does next. The displaced +// merge product is feeMergeTemp's to reclaim — every other recorded set is some +// execResult's TxOut, which stays live. +func (be *blockExecutor) recordWorkerWrites(txVersion state.Version, writes *state.WriteSet) { be.blockIO.RecordWrites(txVersion, writes) - delete(be.feeMergeTemp, tx) + if temp, ok := be.feeMergeTemp[txVersion.TxIndex]; ok { + be.queueMapRelease(temp.writes) + delete(be.feeMergeTemp, txVersion.TxIndex) + } } -// creditedWrites returns ws when an earlier fee merge produced it for tx, which -// makes it the one set already carrying the tx's fee credit. Anything else is -// the worker's own output, which calcFees reads as the pre-credit balance. -func (be *blockExecutor) creditedWrites(tx int, ws *state.WriteSet) *state.WriteSet { - if temp := be.feeMergeTemp[tx]; temp != nil && temp == ws { +// creditedWrites returns ws when an earlier fee merge produced it for this tx +// version, which makes it the one set already carrying the version's fee +// credit. Anything else is the worker's own output, which calcFees reads as the +// pre-credit balance. +func (be *blockExecutor) creditedWrites(txVersion state.Version, ws *state.WriteSet) *state.WriteSet { + if temp, ok := be.feeMergeTemp[txVersion.TxIndex]; ok && temp.writes == ws && temp.version == txVersion { return ws } return nil } -// recordFeeMerge takes ownership of the set the fee merge just recorded for tx -// and reclaims the one it superseded. Only a set an earlier fee merge created -// may be released: prev is otherwise some execResult's TxOut, which stays live. +// recordFeeMerge folds a fee credit into the tx's recorded write set and +// reclaims the set it supersedes. Only a previous fee-merge product may be +// released: prev is otherwise some execResult's TxOut, which stays live. // MergeInto shares VersionedWrite pointers rather than the maps holding them, -// so pooling prev's maps leaves the writes merged now holds intact. -func (be *blockExecutor) recordFeeMerge(tx int, prev, merged *state.WriteSet) { - if temp := be.feeMergeTemp[tx]; temp != nil && temp == prev && merged != temp { - be.queueMapRelease(temp) +// so pooling those maps leaves the merged writes intact. +func (be *blockExecutor) recordFeeMerge(txVersion state.Version, prev, tipWrites *state.WriteSet) { + if tipWrites.IsEmpty() { + return } - be.feeMergeTemp[tx] = merged + merged := prev.MergeInto(tipWrites) + be.blockIO.RecordWrites(txVersion, merged) + if temp, ok := be.feeMergeTemp[txVersion.TxIndex]; ok && temp.writes == prev { + be.queueMapRelease(temp.writes) + } + be.feeMergeTemp[txVersion.TxIndex] = feeMerge{writes: merged, version: txVersion} } // ReleaseMaps clears every map before pooling it, which is O(entries), and a @@ -2678,7 +2697,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r be.blockIO.RecordAccesses(txVersion, res.AccessedAddresses) if res.Version().Incarnation == 0 { - be.recordWorkerWrites(tx, txVersion, res.TxOut) + be.recordWorkerWrites(txVersion, res.TxOut) } else { prevWrites := be.blockIO.WriteSet(txVersion.TxIndex) hasWriteChange := res.TxOut.HasNewWrite(prevWrites) @@ -2692,7 +2711,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r } } - be.recordWorkerWrites(tx, txVersion, res.TxOut) + be.recordWorkerWrites(txVersion, res.TxOut) if hasWriteChange { be.validateTasks.pushPendingSet(be.execTasks.getRevalidationRange(tx + 1)) @@ -2769,15 +2788,11 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r } existingWrites := be.blockIO.WriteSet(txVersion.TxIndex) tipWrites, err := txResult.calcFees(taskVer, be.versionMap, stateReader, txTask.Rules(), - be.creditedWrites(tx, existingWrites)) + be.creditedWrites(txVersion, existingWrites)) if err != nil { return nil, err } - if !tipWrites.IsEmpty() { - merged := existingWrites.MergeInto(tipWrites) - be.blockIO.RecordWrites(txVersion, merged) - be.recordFeeMerge(tx, existingWrites, merged) - } + be.recordFeeMerge(txVersion, existingWrites, tipWrites) } validity := be.versionMap.ValidateVersion(txVersion.TxIndex, be.blockIO, From b10479ed36917196a34c45d9f1767bf8c01b341f Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 12 Aug 2026 14:14:32 +0700 Subject: [PATCH 5/5] execution/stagedsync: make an absent fee entry unrepresentable, compare accounts by Equals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A zero feeEntry read as "already recorded", so a construction site that forgot emit would drop the credit with no error and no failing test. A nil *feeEntry is the absent entry now, and the deleted arm no longer carries the acc and reason it never writes. recordedIn compared the AddressPath value by struct equality, which also folds in Root and PrevIncarnation. Equals is what feeAddressAccount actually fills, so the skip cannot stop firing on a field neither side sets. The deleted arm keys on version and value alone, and a worker's own SELFDESTRUCT carries this same version, so it reads as the credit. That is harmless — the entry would have written that identical delete — but the comment claimed a fence that is not there, and the test built an unrealistic zero-version write to assert it. Both now state what the arm does. --- execution/stagedsync/exec3_finalize_test.go | 54 ++++++++++++++++----- execution/stagedsync/exec3_parallel.go | 38 ++++++++------- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/execution/stagedsync/exec3_finalize_test.go b/execution/stagedsync/exec3_finalize_test.go index 4c091237525..8c19a3904b0 100644 --- a/execution/stagedsync/exec3_finalize_test.go +++ b/execution/stagedsync/exec3_finalize_test.go @@ -1917,29 +1917,21 @@ func TestFeeEntryWriteToIsRecordedIn(t *testing.T) { version := state.Version{TxIndex: 3, Incarnation: 1} addr := fAddr("credited") - entries := []feeEntry{ + entries := []*feeEntry{ { addr: addr, acc: accounts.Account{Balance: *uint256.NewInt(7), Nonce: 2, Incarnation: 1, CodeHash: accounts.EmptyCodeHash}, reason: tracing.BalanceIncreaseRewardTransactionFee, - emit: true, }, { addr: addr, acc: accounts.Account{Balance: *uint256.NewInt(11), CodeHash: accounts.EmptyCodeHash}, reason: tracing.BalanceDecreaseGasBuy, - emit: true, - }, - { - addr: addr, - reason: tracing.BalanceIncreaseRewardTransactionFee, - deleted: true, - emit: true, }, + {addr: addr, deleted: true}, } - for i := range entries { - e := &entries[i] + for i, e := range entries { ws := &state.WriteSet{} e.writeTo(ws, version) @@ -1952,6 +1944,46 @@ func TestFeeEntryWriteToIsRecordedIn(t *testing.T) { } } +// TestFeeEntryNilIsAbsent pins the absent entry: an adjustment that does not +// touch this address writes nothing and reads as already recorded, so the skip +// turns on the entries that do exist. +func TestFeeEntryNilIsAbsent(t *testing.T) { + t.Parallel() + var absent *feeEntry + ws := &state.WriteSet{} + + absent.writeTo(ws, state.Version{TxIndex: 3}) + require.True(t, ws.IsEmpty(), "an absent entry has nothing to write") + require.True(t, absent.recordedIn(ws, state.Version{TxIndex: 3})) +} + +// TestFeeEntryDeletedMatchesAnyWriterAtThisVersion pins what the deleted arm +// accepts. It keys on version and value only, so a worker's own SELFDESTRUCT at +// this version reads as the credit — harmless, because the entry writes that +// same delete. +func TestFeeEntryDeletedMatchesAnyWriterAtThisVersion(t *testing.T) { + t.Parallel() + addr := fAddr("emptied") + version := state.Version{TxIndex: 3, Incarnation: 1} + e := &feeEntry{addr: addr, deleted: true} + + selfDestruct := func(v state.Version, val bool) *state.WriteSet { + ws := &state.WriteSet{} + ws.SetSelfDestruct(addr, &state.VersionedWrite[bool]{ + WriteHeader: state.WriteHeader{Address: addr, Path: state.SelfDestructPath, Version: v}, + Val: val, + }) + return ws + } + + require.False(t, e.recordedIn(selfDestruct(state.Version{TxIndex: 3}, true), version), + "a delete stamped at another incarnation is not this credit") + require.False(t, e.recordedIn(selfDestruct(version, false), version), + "a SelfDestruct write that is not a delete carries no empty-removal") + require.True(t, e.recordedIn(selfDestruct(version, true), version), + "any delete at this version counts, whichever writer produced it") +} + var feeCreditSink *state.WriteSet func BenchmarkCalcFees(b *testing.B) { diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 6f775a8c49a..27b1e85fa54 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -2010,28 +2010,27 @@ func (result *execResult) calcFees( return nil, nil } - var coinbaseEntry, burntEntry feeEntry + var coinbaseEntry, burntEntry *feeEntry if emitCoinbase { - coinbaseEntry = feeEntry{ - addr: result.Coinbase, - acc: feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce), - reason: tracing.BalanceIncreaseRewardTransactionFee, - deleted: coinbaseEmptied, - emit: true, + coinbaseEntry = &feeEntry{addr: result.Coinbase, deleted: coinbaseEmptied} + if !coinbaseEmptied { + coinbaseEntry.acc = feeAddressAccount(coinbaseAcc, newCoinbaseBalance, coinbaseNonce) + coinbaseEntry.reason = tracing.BalanceIncreaseRewardTransactionFee } } if emitBurnt { - burntEntry = feeEntry{ + burntEntry = &feeEntry{ addr: burntAddr, acc: feeAddressAccount(burntAcc, newBurntBalance, 0), reason: tracing.BalanceDecreaseGasBuy, - emit: true, } } // The apply loop re-credits a tx once per validation round, and the credit // only moves when a prior tx's writes moved under it. An unchanged credit // would rebuild a set identical to the one already recorded — including the - // CollectorWrites entries an earlier round already put there. + // CollectorWrites entries an earlier round already put there. A coinbase that + // is also the burnt address never matches: one write cannot carry both + // tracing reasons, so those blocks pay the full rebuild every round. if coinbaseEntry.recordedIn(credited, taskVersion) && burntEntry.recordedIn(credited, taskVersion) { return nil, nil } @@ -2054,20 +2053,23 @@ func (result *execResult) calcFees( return addWrites, nil } -// feeEntry is one address's share of a tip credit: the post-credit account, -// whose Balance is also the BalancePath value, or a delete when EIP-161 removes -// the emptied account instead. +// feeEntry is one address's share of a fee adjustment: the post-adjustment +// account, whose Balance is also the BalancePath value, or a delete when EIP-161 +// removes the emptied account instead. A nil *feeEntry is an address this +// adjustment does not touch. type feeEntry struct { addr accounts.Address acc accounts.Account reason tracing.BalanceChangeReason deleted bool - emit bool } -// recordedIn reports whether ws already carries this entry verbatim. +// recordedIn reports whether ws already carries this entry verbatim. Reason +// fences the balance arm from a worker's own balance write. The deleted arm has +// no fence: a worker's SELFDESTRUCT carries the same version and reads as +// recorded, which is harmless because the entry writes that identical delete. func (e *feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { - if !e.emit { + if e == nil { return true } if e.deleted { @@ -2079,11 +2081,11 @@ func (e *feeEntry) recordedIn(ws *state.WriteSet, version state.Version) bool { return false } aw, ok := ws.GetAddress(e.addr) - return ok && aw.Val != nil && *aw.Val == e.acc && aw.Version == version + return ok && aw.Val != nil && aw.Val.Equals(&e.acc) && aw.Version == version } func (e *feeEntry) writeTo(ws *state.WriteSet, version state.Version) { - if !e.emit { + if e == nil { return } if e.deleted {