fix(eth/tracers): skip the EIP-8246 selfdestruct burn #35219, close #2832 by gzliudan · Pull Request #2833 · XinFinOrg/XDPoSChain

gzliudan · 2026-10-20T10:06:52Z

Summary

The supply live tracer (--vmtrace supply) counted a selfdestruct-to-self as burn.misc whenever the SELFDESTRUCT hook fired with from == to and a non-zero balance. EIP-8246 (Amsterdam) preserves that balance instead of burning it, so the tracer over-reported the burn on Amsterdam chains.

The EIP-8246 state change does not cover this path: hookedStateDB.Finalise no longer emits BalanceDecreaseSelfdestructBurn, but the tracer infers the burn from the OnEnter event instead of a balance hook.

Fix

Restore the chainConfig field and the OnBlockchainInit hook that upstream go-ethereum already has on this tracer. core/blockchain.go invokes OnBlockchainInit for every live tracer.
Cache IsAmsterdam(block.Number) in OnBlockStart and only record call.burn while the fork still burns the balance. XDC forks are block-based, so the block number alone decides.
Treat a missing chain config as pre-fork, keeping the existing accounting instead of silently dropping it.

Tests

eth/tracers/live/supply_test.go drives the hooks through a transaction whose top-level call executes SELFDESTRUCT with itself as beneficiary and checks the produced burn.misc:

pre-Amsterdam: the balance is reported as burnt;
Amsterdam active: no burn;
the fork block boundary in both directions;
a reverted selfdestruct: never counted;
missing chain config: pre-fork behaviour.

Removing the s.burnsBalance gate makes the Amsterdam cases fail (reported burn = 100, want none).

core/blockchain_livetracer_test.go pins that core.NewBlockChain delivers the chain configuration through OnBlockchainInit; dropping that call makes the test fail.

Upstream

go-ethereum has the same latent bug: its EIP-8246 commit (ethereum#35219) does not touch eth/tracers, and its selfdestruct supply test runs with params.TestChainConfig, where Cancun and Amsterdam are unset. There is no upstream fix to port; this is a local fix that could be proposed upstream.

Verification

make all passes; gofmt/goimports clean.
golangci-lint reports no new issues in ./eth/tracers/live/... and ./core/....
go test ./eth/tracers/... ./core/... -count=1 passes.

Merge order

The gate must not reach a chain that schedules Amsterdam before the consensus-side change does. Merge #2834 (EIP-8246 selfdestruct burn removal) first, or in the same batch. If this PR lands first, an Amsterdam-enabled dev/test chain running --vmtrace supply under-reports burn.misc for selfdestruct-to-self until #2834 is in.

if fix(core/state,eth/tracers): report the selfdestruct burn from the state layer #2870 lands first, the chainConfig/burnsBalance gate introduced here has no
consumer left and this PR should be closed instead of rebased;
if this one lands first, fix(core/state,eth/tracers): report the selfdestruct burn from the state layer #2870 has to resolve the conflict in supply.go and
supply_test.go and delete the gate as part of its own change.

Impact

Observability only, no consensus change. Production XDC networks do not schedule Amsterdam; the affected set is an Amsterdam-enabled dev/test chain with --vmtrace supply enabled.

Closes #2832

coderabbitai · 2026-10-20T10:07:00Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: d51ebb19-3350-4550-a493-c4cefcccf8b4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gzliudan · 2026-10-21T01:24:31Z

Review note after the latest push (cd6b8a0ddc).

One clarification on scope: the most recent external round inspected bb3f04d551, i.e. the tree from before the previous round was folded in. Two of its three findings were already covered in the then-current head c110369c09 - the commit message states the EIP-8246 delivery dependency, "Merge order" above names #2834, and core/blockchain_livetracer_test.go pins that core.NewBlockChain delivers the chain config through OnBlockchainInit.

Net change in this push:

A missing chain config no longer falls back silently. OnBlockStart logs once when chainConfig is nil. The fallback still means pre-fork accounting; only a mount point other than core.NewBlockChain can reach it, because that path always delivers a non-nil config. The warning is diagnostic and deliberately not asserted, so the tests stay independent of the log handler.
No other code change: the diff against the previous head is those 9 lines only.

Upstream still has nothing to port: go-ethereum's onEnter has no fork gate (same latent bug) and its onBlockchainInit only stores the config.

Checks on cd6b8a0ddc: make all clean, gofmt/goimports clean, golangci-lint --new-from-rev=c110369c09 ./eth/tracers/live/... reports 0 issues, go test ./eth/tracers/live/... passes. The warning fires exactly once across three blocks (verified with a throwaway probe that was removed before committing).

Merge order is unchanged: this still needs #2834 first or in the same batch.

The supply live tracer accounted a selfdestruct-to-self as burned whenever the SELFDESTRUCT hook fired with from == to and a non-zero balance. That rule predates EIP-8246: from Amsterdam onwards the balance is preserved, so the tracer over-reports burn.misc on Amsterdam chains. The EIP-8246 state change does not reach this path: hookedStateDB.Finalise no longer emits BalanceDecreaseSelfdestructBurn, but this inference runs off the OnEnter event instead of a balance hook. Restore the chain-config plumbing upstream go-ethereum already has for this tracer - a chainConfig field filled by the OnBlockchainInit hook, which core.NewBlockChain invokes for every live tracer - and cache IsAmsterdam(block.Number) in OnBlockStart, so OnEnter only records the burn while the fork still burns the balance. A missing chain config keeps the previous behaviour rather than silently dropping the accounting, and is logged once so the fallback cannot go unnoticed. The gate is only correct together with the consensus side of EIP-8246: on a chain that schedules Amsterdam without it, the balance is still destroyed while the tracer stops reporting it. Ship both changes together. go-ethereum has the same latent bug: its EIP-8246 commit does not touch eth/tracers, and its selfdestruct supply test runs with params.TestChainConfig where Cancun and Amsterdam are unset, so there is no upstream fix to port. Tests: eth/tracers/live covers the pre-Amsterdam burn, Amsterdam active, the fork boundary in both directions, a reverted selfdestruct and the missing chain config; removing the gate makes the Amsterdam cases fail. The warning for a missing chain config is diagnostic only and deliberately not asserted, so the test stays independent of the log handler. core/blockchain_livetracer_test.go pins that core.NewBlockChain delivers the chain config through OnBlockchainInit; dropping that call makes it fail. Verified: make all and gofmt/goimports are clean, golangci-lint reports no new issues, and go test ./eth/tracers/... and ./core/... pass. Ref: ethereum#35219 Closes XinFinOrg#2832

The supply tracer adds a BalanceDecreaseSelfdestructBurn reported by the state to burn.misc; that half of the accounting had no test in this tree, since the upstream supply tests (go-ethereum ethereum#29347) were never ported. Cover it at the hook boundary: the burn is accumulated, the other selfdestruct reasons are ignored, and the accounting does not depend on the fork because the state layer decides whether a balance is burnt. Ref: ethereum#35219

Copilot

🔵 Needs a closer look

Safe merging requires confirming the consensus prerequisite #2834 and coordinating with the overlapping alternative fix in #2870.

No open issues 🎉

What changed in this PR

Updates the supply live tracer to stop reporting selfdestruct-to-self balances as burnt after Amsterdam, aligning its accounting with EIP-8246 without changing consensus behavior.

Changes:

Restores chain-configuration initialization and adds a per-block Amsterdam gate.
Preserves pre-fork accounting when configuration is missing, with a one-time warning.
Adds tests for fork boundaries, reverts, balance-hook accounting, and delivery of chain configuration.

File	Description
eth/tracers/live/supply.go	Gates inferred selfdestruct burns on Amsterdam activation.
eth/tracers/live/supply_test.go	Tests fork-aware burn accounting and fallback behavior.
core/blockchain_livetracer_test.go	Verifies chain configuration reaches live tracers during initialization.

_{Review effort: Balanced}

…ate layer The supply live tracer inferred a selfdestruct burn from the SELFDESTRUCT enter event: a self-destruct to self was counted as burnt whenever that event carried a non-zero balance. The inference is wrong from EIP-6780 (Cancun) onwards, where a pre-existing contract is not destructed at all and keeps its balance, and from EIP-8246 (Amsterdam) onwards, where the balance is preserved even for a contract created in the same transaction. The tracer therefore over-reported burn.misc on every chain that reached Cancun. The state knows how much a destruction really burns, so let it report that: the balance a destruct clears is remembered on the account and reported at finalisation, together with the ether that arrives in the destructed account afterwards. A reverted destruct drops that memory again, so nothing is reported for it. The supply tracer drops its own inference and accounts the burn from the BalanceDecreaseSelfdestructBurn event alone. For that accounting to hold, the balance a destruct clears has to be the balance it destroys. The EIP-6780 SELFDESTRUCT already moves the balance with an explicit decrease/increase pair before the account is destroyed, so it has nothing left to clear when the balance went to another account, and clears exactly the balance when the account appointed itself. The pre-Cancun implementation credited the beneficiary first and cleared the account afterwards instead, which left the state unable to tell a transfer from a burn; it now moves the balance the same way. The final balances are unchanged, only the tracing picture becomes truthful. That event now carries the whole burnt balance instead of only the ether that arrived in an already destructed account, so its documented meaning is extended accordingly. The only production consumer of the event is the supply tracer. Tests: core/selfdestruct_burn_test.go drives the cases through block processing for every fork: up to Cancun a self-destruct to itself burns the balance and one to another account burns nothing, a pre-existing contract burns nothing from Cancun onwards and EIP-8246 keeps the balance. core/state covers the reverted destruct, the preserved balance, the copied state and successive destructs of one account. eth/tracers/live covers the tracer side: the enter event alone reports no burn, the balance hook reports the whole burn. dev-upgrade already carried the tracer-side fork gate from XinFinOrg#2833, which kept that inference on IsAmsterdam(block) and pulled in the chain config to do so. With the burn reported by the state layer the gate has nothing left to decide, so it goes together with the inference and the chain-config plumbing behind it; its enter-only assertion moves into the balance-change test.

…ate layer The supply live tracer inferred a selfdestruct burn from the SELFDESTRUCT enter event: a self-destruct to self was counted as burnt whenever that event carried a non-zero balance. The inference is wrong from EIP-6780 (Cancun) onwards, where a pre-existing contract is not destructed at all and keeps its balance, and from EIP-8246 (Amsterdam) onwards, where the balance is preserved even for a contract created in the same transaction. The tracer therefore over-reported burn.misc on every chain that reached Cancun. The state knows how much a destruction really burns, so let it report that: the balance a destruct clears is remembered on the account and reported at finalisation, together with the ether that arrives in the destructed account afterwards. A reverted destruct drops that memory again, so nothing is reported for it. The supply tracer drops its own inference and accounts the burn from the BalanceDecreaseSelfdestructBurn event alone. For that accounting to hold, the balance a destruct clears has to be the balance it destroys. The EIP-6780 SELFDESTRUCT already moves the balance with an explicit decrease/increase pair before the account is destroyed, so it has nothing left to clear when the balance went to another account, and clears exactly the balance when the account appointed itself. The pre-Cancun implementation credited the beneficiary first and cleared the account afterwards instead, which left the state unable to tell a transfer from a burn; it now moves the balance the same way. The final balances are unchanged, only the tracing picture becomes truthful. That event now carries the whole burnt balance instead of only the ether that arrived in an already destructed account, so its documented meaning is extended accordingly. The only production consumer of the event is the supply tracer. Tests: core/selfdestruct_burn_test.go drives the cases through block processing for every fork: up to Cancun a self-destruct to itself burns the balance and one to another account burns nothing, a pre-existing contract burns nothing from Cancun onwards and EIP-8246 keeps the balance. core/state covers the reverted destruct, the preserved balance, the copied state and successive destructs of one account. eth/tracers/live covers the tracer side: the balance hook alone reports the burn, every other balance reason is ignored. dev-upgrade already carried the tracer-side fork gate from XinFinOrg#2833, which kept that inference on IsAmsterdam(block) and pulled in the chain config to do so. With the burn reported by the state layer the gate has nothing left to decide, so it goes together with the inference and the chain-config plumbing behind it, along with the callstack wiring that only fed the removed inference.

…ate layer The supply live tracer inferred a selfdestruct burn from the SELFDESTRUCT enter event: a self-destruct to self was counted as burnt whenever that event carried a non-zero balance. The inference is wrong from EIP-6780 (Cancun) onwards, where a pre-existing contract is not destructed at all and keeps its balance, and from EIP-8246 (Amsterdam) onwards, where the balance is preserved even for a contract created in the same transaction. The tracer therefore over-reported burn.misc on every chain that reached Cancun. The state knows how much a destruction really burns, so let it report that: the balance a destruct clears is remembered on the account and reported at finalisation, together with the ether that arrives in the destructed account afterwards. A reverted destruct drops that memory again, so nothing is reported for it. The supply tracer drops its own inference and accounts the burn from the BalanceDecreaseSelfdestructBurn event alone. For that accounting to hold, the balance a destruct clears has to be the balance it destroys. The EIP-6780 SELFDESTRUCT already moves the balance with an explicit decrease/increase pair before the account is destroyed, so it has nothing left to clear when the balance went to another account, and clears exactly the balance when the account appointed itself. The pre-Cancun implementation credited the beneficiary first and cleared the account afterwards instead, which left the state unable to tell a transfer from a burn; it now moves the balance the same way. The final balances are unchanged, only the tracing picture becomes truthful. That event now carries the whole burnt balance instead of only the ether that arrived in an already destructed account, so its documented meaning is extended accordingly. The only production consumer of the event is the supply tracer. Tests: core/selfdestruct_burn_test.go drives the cases through block processing for every fork: up to Cancun a self-destruct to itself burns the balance and one to another account burns nothing, a pre-existing contract burns nothing from Cancun onwards and EIP-8246 keeps the balance. The same test asserts the final balances of the contract and of the other account, so the burn accounting cannot drift into moving the ether elsewhere. core/state covers the reverted destruct, the preserved balance, the copied state and successive destructs of one account. eth/tracers/live covers the tracer side: the balance hook alone reports the burn, every other balance reason is ignored. dev-upgrade already carried the tracer-side fork gate from XinFinOrg#2833, which kept that inference on IsAmsterdam(block) and pulled in the chain config to do so. With the burn reported by the state layer the gate has nothing left to decide, so it goes together with the inference and the chain-config plumbing behind it, along with the callstack wiring that only fed the removed inference.

…ate layer (#2870) * fix(core/state,eth/tracers): report the selfdestruct burn from the state layer The supply live tracer inferred a selfdestruct burn from the SELFDESTRUCT enter event: a self-destruct to self was counted as burnt whenever that event carried a non-zero balance. The inference is wrong from EIP-6780 (Cancun) onwards, where a pre-existing contract is not destructed at all and keeps its balance, and from EIP-8246 (Amsterdam) onwards, where the balance is preserved even for a contract created in the same transaction. The tracer therefore over-reported burn.misc on every chain that reached Cancun. The state knows how much a destruction really burns, so let it report that: the balance a destruct clears is remembered on the account and reported at finalisation, together with the ether that arrives in the destructed account afterwards. A reverted destruct drops that memory again, so nothing is reported for it. The supply tracer drops its own inference and accounts the burn from the BalanceDecreaseSelfdestructBurn event alone. For that accounting to hold, the balance a destruct clears has to be the balance it destroys. The EIP-6780 SELFDESTRUCT already moves the balance with an explicit decrease/increase pair before the account is destroyed, so it has nothing left to clear when the balance went to another account, and clears exactly the balance when the account appointed itself. The pre-Cancun implementation credited the beneficiary first and cleared the account afterwards instead, which left the state unable to tell a transfer from a burn; it now moves the balance the same way. The final balances are unchanged, only the tracing picture becomes truthful. That event now carries the whole burnt balance instead of only the ether that arrived in an already destructed account, so its documented meaning is extended accordingly. The only production consumer of the event is the supply tracer. Tests: core/selfdestruct_burn_test.go drives the cases through block processing for every fork: up to Cancun a self-destruct to itself burns the balance and one to another account burns nothing, a pre-existing contract burns nothing from Cancun onwards and EIP-8246 keeps the balance. The same test asserts the final balances of the contract and of the other account, so the burn accounting cannot drift into moving the ether elsewhere. core/state covers the reverted destruct, the preserved balance, the copied state and successive destructs of one account. eth/tracers/live covers the tracer side: the balance hook alone reports the burn, every other balance reason is ignored. dev-upgrade already carried the tracer-side fork gate from #2833, which kept that inference on IsAmsterdam(block) and pulled in the chain config to do so. With the burn reported by the state layer the gate has nothing left to decide, so it goes together with the inference and the chain-config plumbing behind it, along with the callstack wiring that only fed the removed inference. * chore(eth/tracers): add the missing license header to the supply tracer * docs(core): correct the pre-Byzantium finalisation comment

gzliudan mentioned this pull request Oct 20, 2026

feat(core/vm,core/state): EIP-8246 selfdestruct burn removal #35219, close #2766 #2834

Merged

gzliudan requested review from AnilChinchawale, anunay-xin, benjamin202410, liam-lai and wanwiset25 October 20, 2026 10:41

gzliudan force-pushed the fix-supply-tracer-eip8246-burn branch 3 times, most recently from c110369 to cd6b8a0 Compare October 21, 2026 01:24

gzliudan mentioned this pull request Oct 21, 2026

implement EIP-8246 SELFDESTRUCT burn removal (amsterdam) #2766

Closed

gzliudan force-pushed the fix-supply-tracer-eip8246-burn branch 2 times, most recently from 194468c to e0f3542 Compare October 22, 2026 17:20

gzliudan mentioned this pull request Oct 22, 2026

fix(core/state,eth/tracers): report the selfdestruct burn from the state layer #2870

Merged

wanwiset25 approved these changes Oct 26, 2026

View reviewed changes

gzliudan force-pushed the fix-supply-tracer-eip8246-burn branch 2 times, most recently from 568b653 to f439b84 Compare October 26, 2026 05:32

anunay-xin approved these changes Oct 26, 2026

View reviewed changes

benjamin202410 approved these changes Oct 26, 2026

View reviewed changes

gzliudan force-pushed the fix-supply-tracer-eip8246-burn branch from f439b84 to 43e7fc7 Compare October 26, 2026 23:30

gzliudan force-pushed the fix-supply-tracer-eip8246-burn branch from 43e7fc7 to 1021cfc Compare October 26, 2026 23:50

This was referenced Oct 27, 2026

eth/tracers: port the upstream supply tracer end-to-end tests #2904

Open

fix(eth/tracers): fix supply tracer uncle accounting #31882 #2905

Open

gzliudan requested a balanced review from Copilot October 27, 2026 07:47

Copilot started reviewing on behalf of gzliudan October 27, 2026 07:48

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

gzliudan merged commit c2725ee into XinFinOrg:dev-upgrade Oct 27, 2026
14 checks passed

gzliudan deleted the fix-supply-tracer-eip8246-burn branch October 27, 2026 07:57

gzliudan mentioned this pull request Oct 28, 2026

eth/tracers: supply tracer counts the EIP-8246 selfdestruct as a burn #2832

Closed

Navigation Menu

Repository navigation

fix(eth/tracers): skip the EIP-8246 selfdestruct burn #35219, close #2832 - #2833

fix(eth/tracers): skip the EIP-8246 selfdestruct burn #35219, close #2832#2833
gzliudan merged 2 commits into
XinFinOrg:dev-upgradefrom
gzliudan:fix-supply-tracer-eip8246-burn

gzliudan commented Oct 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading

Review skipped

Uh oh!

gzliudan commented Oct 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

gzliudan commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Tests

Upstream

Verification

Merge order

Related

Impact

Uh oh!

coderabbitai Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

gzliudan commented Oct 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gzliudan commented Oct 20, 2026 •

edited

Loading

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading