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

EIP-2935 Historical block hashes #9991

Merged
merged 18 commits into from
May 9, 2024
Merged

EIP-2935 Historical block hashes #9991

merged 18 commits into from
May 9, 2024

Conversation

somnathb1
Copy link
Contributor

@somnathb1 somnathb1 commented Apr 18, 2024

@somnathb1 somnathb1 marked this pull request as draft April 18, 2024 23:54
@somnathb1 somnathb1 added pectra The Prague/Electra protocol upgrade do-not-merge PR that is in a merge-able state but is waiting for something else to take place before merging labels Apr 18, 2024
@yperbasis yperbasis added the imp2 Medium importance label Apr 23, 2024
@somnathb1
Copy link
Contributor Author

On hold for a few days until EIP doc is updated

@yperbasis yperbasis added imp1 High importance and removed imp2 Medium importance labels Apr 24, 2024
@somnathb1 somnathb1 marked this pull request as ready for review April 29, 2024 00:22
@somnathb1 somnathb1 changed the title [WIP] EIP-2935 Historical block hashes EIP-2935 Historical block hashes Apr 30, 2024
@yperbasis yperbasis changed the base branch from release/2.60 to main May 1, 2024 07:26
@yperbasis yperbasis removed the do-not-merge PR that is in a merge-able state but is waiting for something else to take place before merging label May 3, 2024
consensus/misc/eip2935.go Outdated Show resolved Hide resolved
consensus/misc/eip2935.go Outdated Show resolved Hide resolved
consensus/misc/eip2935.go Outdated Show resolved Hide resolved
if parent.Time < config.PragueTime.Uint64() {
p := parent.Number.Uint64()
window := params.BlockHashHistoryServeWindow - 1
if p < window {
Copy link
Member

@yperbasis yperbasis May 3, 2024

Choose a reason for hiding this comment

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

It's probably equivalent, but I find the pseudo-code in the spec easier to read. What I mean that instead of this p < window branch you have a early break in the loop. Something like:

for i := 0; i < params.BlockHashHistoryServeWindow - 1; i++ {
  ancestorNumber := parent.Number.Uint64() - 1
  if ancestorNumber == 0 { // stop at genesis block
    break
  }
  storeHash(big.NewInt(ancestorNumber), parent.ParentHash, state)
  parent = headerReader.GetHeader(parent.ParentHash, ancestorNumber)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an optimization to not have to do big.Int to uint64(), or any big.Int operation that many times

Copy link
Member

Choose a reason for hiding this comment

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

I see. Between optimization and clarity I'd choose clarity until profiling confirms that the performance degradation is material, but OK.

core/vm/instructions.go Show resolved Hide resolved
@somnathb1 somnathb1 requested a review from yperbasis May 7, 2024 00:11
core/vm/eips.go Outdated Show resolved Hide resolved
core/vm/operations_acl.go Outdated Show resolved Hide resolved
core/vm/instructions.go Outdated Show resolved Hide resolved
@somnathb1 somnathb1 requested a review from yperbasis May 7, 2024 22:12
core/vm/operations_acl.go Outdated Show resolved Hide resolved
@yperbasis yperbasis merged commit 0ea45b7 into main May 9, 2024
10 checks passed
@yperbasis yperbasis deleted the som/eip2935 branch May 9, 2024 07:25
yperbasis added a commit that referenced this pull request Oct 1, 2024
Cherry pick #9991

Co-authored-by: Somnath <snb895@outlook.com>
AskAlexSharov pushed a commit that referenced this pull request Oct 21, 2024
Cherry pick #9991

Co-authored-by: Somnath <snb895@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imp1 High importance pectra The Prague/Electra protocol upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants