fix(core,eth/tracers): resolve coinbase owner per transaction#2285
fix(core,eth/tracers): resolve coinbase owner per transaction#2285gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect fee recipient behavior during block processing by ensuring the coinbase “owner” is resolved from state for each transaction (instead of being computed once before iterating the block’s transactions). This prevents fee credits from going to a stale owner if the owner mapping changes mid-block.
Changes:
- Move coinbase owner lookup into
ApplyTransactionWithEVMso it is refreshed per transaction. - Remove the now-unneeded
getCoinbaseOwnerhelper and adjust internal call sites accordingly. - Update unit tests to match the new
ApplyTransactionWithEVMfunction signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/state_processor.go | Computes coinbase owner inside ApplyTransactionWithEVM per-tx; removes helper; updates callers. |
| core/state_processor_test.go | Updates tests to call ApplyTransactionWithEVM with the new signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
62da702 to
971ebb3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
672da00 to
36fcd96
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
36fcd96 to
f39220b
Compare
|
is this potential fork? |
4577602 to
f0e8732
Compare
f266410 to
b6cce42
Compare
Resolve the coinbase owner inside ApplyTransactionWithEVM instead of passing a precomputed value from block processing. This keeps fee routing aligned with the active state for each transaction and makes the block execution and tracer paths use the same owner-resolution logic.
b6cce42 to
acf6ebd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A consensus difference is triggered only if all of the following conditions hold: The block is after The current block producer address, meaning the current Earlier in the same block, a transaction changes the owner result for that same current After that owner-changing transaction, the block still contains at least one later normal transaction that goes through the ordinary fee-credit path. That combination matters because the old code resolved the coinbase owner once at block start and reused it for the whole block, while In
This writes a new validator record with
When the threshold is reached, this deletes validator records for the invalidated owner’s candidates. That does not assign a new nonzero owner, but it does change the effective owner lookup result, because reading Other functions such as For the current block producer specifically, the more realistic trigger is This can only cause a consensus difference if, within the same block, the owner lookup result for the current block producer changes before later normal transactions are executed. In practice, for the current |
Proposed changes
Resolve the coinbase owner inside ApplyTransactionWithEVM instead of passing a precomputed value from block processing.
This keeps fee routing aligned with the active state for each transaction and makes the block execution and tracer paths use the same owner-resolution logic.
Ref: #649
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that