Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't create reward account if no reward #7826

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Oct 29, 2024

PR description

If there is no coinbase reward, don't get/create the account. In a BFT chain with no mining rewards this causes every block to create & delete the mining node's account. The resulting state is that there is no balance (which is correct) but only after creating the account, then deleting it.

This change is common to both public and BFT chains, but I think it should be safe to skip the create/delete of the account state update if there is no state change to make.

@matkt
Copy link
Contributor

matkt commented Nov 4, 2024

it will not be a problem for the block pre EIP-158 ?

@matthew1001
Copy link
Contributor Author

it will not be a problem for the block pre EIP-158 ?

Good question. Maybe it's worth considering the following:

  1. Before this PR, a Bonsai node full syncing mainnet would create the account for the block reward, and if that reward was 0, would clear the account before persisting. So there would be no persisted account for the block reward either pre- or post- eip-158.
  2. The logic in the PR only applies for 0 block rewards. Are there any non-orphan or non-uncle blocks, pre- or post- eip-158 that have 0 mining reward?
  3. What would be the difference between an empty, persisted account, and non-existent account from the user's perspective?
    • Would eth_getBalance return a different value?
    • Would eth_transactionCount return a different value?
    • Would eth_getProof return a different value?
    • If the answer is yes, then perhaps it needs more consideration. But I think points 1 and 2 are still worth bearing in mind.

@matthew1001
Copy link
Contributor Author

One option would be to try and refactor this logic into BaseBftProtocolScheduleBuilder as a way of making it BFT-specific. But I'd prefer to do that only if we know this will break a mainnet client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants