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

Move atomic hash from Block to Header #11513

Merged
merged 30 commits into from
Aug 14, 2024
Merged

Move atomic hash from Block to Header #11513

merged 30 commits into from
Aug 14, 2024

Conversation

AskAlexSharov
Copy link
Collaborator

@AskAlexSharov AskAlexSharov commented Aug 7, 2024

Reason 1: NewEVMBlockContext -> engine.Author(header) -> *Bor ->

hash := header.Hash()
if address, known := sigcache.Get(hash); known {
   return address, nil
}

it's hard and not so useful to pass block through this abstractions.

So, RPC does hash every requested header here.

Reason 2: In E3: many RPC will read 1 transaction only. So, likely we will have many places where no Block object.

@AskAlexSharov AskAlexSharov requested a review from yperbasis August 7, 2024 11:04
@AskAlexSharov AskAlexSharov changed the title attempt to move atomic hash from Block to Header [wip] attempt to move atomic hash from Block to Header Aug 8, 2024
@AskAlexSharov
Copy link
Collaborator Author

AskAlexSharov commented Aug 8, 2024

@yperbasis what do you think about:
https://github.com/erigontech/erigon/pull/11513/files#diff-e177ddb5610d1f3426c15cb512999613785b20838954e48187883e6263bb282bR118

type.Block immutability was enforced by geth team. Maybe it's not so bad idea to enforce Header immutability also.
Only place where we need mutable header is: mining engine.Prepare/engine.Seal

There is also Finalize/FinalizeAndAssemble mutating Header: but I think Finalize must not mutate header - because Finalize called in execution and FinalizeAndAssemble called in mining. Changed Finalize.

@AskAlexSharov AskAlexSharov self-assigned this Aug 8, 2024
@@ -396,9 +396,6 @@ func (c *Clique) Finalize(config *chain.Config, header *types.Header, state *sta
func (c *Clique) FinalizeAndAssemble(chainConfig *chain.Config, header *types.Header, state *state.IntraBlockState,
txs types.Transactions, uncles []*types.Header, receipts types.Receipts, withdrawals []*types.Withdrawal, requests types.Requests, chain consensus.ChainReader, syscall consensus.SystemCall, call consensus.Call, logger log.Logger,
) (*types.Block, types.Transactions, types.Receipts, error) {
// No block rewards in PoA, so the state remains as is and uncles are dropped
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the same line from (c *Clique) Finalize

Copy link
Member

@yperbasis yperbasis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuRa & Merge consensus don't call block.WithSeal when building blocks. We could change them to ensure they do. But I think a better option is to set b.header.mutable to false at the end of NewBlock.

@yperbasis yperbasis requested review from somnathb1 and racytech August 9, 2024 08:27
yperbasis
yperbasis approved these changes Aug 14, 2024
@yperbasis yperbasis changed the title [wip] attempt to move atomic hash from Block to Header Move atomic hash from Block to Header Aug 14, 2024
@yperbasis yperbasis merged commit f4322d9 into main Aug 14, 2024
10 of 11 checks passed
@yperbasis yperbasis deleted the h_atomic branch August 14, 2024 09:23
yperbasis added a commit that referenced this pull request Aug 14, 2024
Rationale: one small step to converge Polygon and Ethereum block
production.

Also, now `(s *Merge) Seal` calls `block.WithSeal`, which is important
after PR #11513.
yperbasis added a commit that referenced this pull request Oct 24, 2024
Cherry pick #11513 into `release/2.61`

Co-authored-by: Alex Sharov <[email protected]>
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