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.

Sign up for GitHub

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

Jump to bottom

Add EIP-6122: Forkid checks based on timestamps #6122

Merged
merged 11 commits into from
Jan 31, 2023
Merged

Add EIP-6122: Forkid checks based on timestamps #6122

merged 11 commits into from
Jan 31, 2023

Conversation

MariusVanDerWijden
Copy link
Member

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@github-actions github-actions bot added c-new Creates a brand new proposal e-number Waiting on EIP Number assignment s-draft This EIP is a Draft s-final This EIP is Final t-meta t-networking labels Dec 13, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Dec 13, 2022 •
edited
Loading

A critical exception has occurred:
Message: pr 6122 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

@github-actions github-actions bot removed t-meta e-number Waiting on EIP Number assignment s-final This EIP is Final labels Dec 13, 2022
@MariusVanDerWijden MariusVanDerWijden changed the title Forkid shanghai EIP-6122: Forkid checks based on timestamps Dec 13, 2022
holiman
holiman reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
holiman
holiman reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
holiman
holiman reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
holiman
holiman reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
g11tech
g11tech previously approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm!

@g11tech g11tech mentioned this pull request Dec 13, 2022
Merged
axic
axic reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
axic
axic reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Show resolved Hide resolved
axic
axic reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
@poojaranjan poojaranjan mentioned this pull request Dec 13, 2022
Closed
djrtwo
djrtwo reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

A handful of nits. Agreed with @axic that we should call out explicitly that

  1. timestamps are strictly much larger than block numbers and thus it's an easy transition to utilize the same uint64 type and previous logic
  2. that once you go to timestamps, this standard does not support going back to block number

EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
@Pandapip1 Pandapip1 changed the title EIP-6122: Forkid checks based on timestamps EIP-6122: Forbid checks based on timestamps Dec 13, 2022
Pandapip1
Pandapip1 reviewed Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
@Pandapip1 Pandapip1 changed the title EIP-6122: Forbid checks based on timestamps EIP-6122: Forkid checks based on timestamps Dec 13, 2022
@Pandapip1 @axic @djrtwo
Apply suggestions from code review
db75b59 
Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
@Pandapip1 Pandapip1 changed the title EIP-6122: Forkid checks based on timestamps Add EIP-6122: Forkid checks based on timestamps Dec 13, 2022
smartprogrammer93
smartprogrammer93 suggested changes Dec 13, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md
}
```

Here's a suite of tests of the different states a Mainnet node might be in and the different remote fork identifiers it might be required to validate and decide to accept or reject:
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A summary of rejection cases are as follows

  1. Hash of remote must be one of the hashes that the local node is aware of (past fork or future fork)
  2. if Hash of remote is older than current Hash of local, remote Next must be equal to next fork (that exists in local)
  3. if remote Next is smaller than local current (BlockNumber or Timestamp), local must have forked at that Next or reject remote.

Please correct me if i am wrong

@siladu siladu mentioned this pull request Dec 13, 2022
Closed
lightclient
lightclient reviewed Dec 14, 2022
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
@Pandapip1 @lightclient @smartprogrammer93
Fix typos highlighted in code review
6aaa54b 
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Ahmad Bitar <33181301+smartprogrammer93@users.noreply.github.com>
@siladu siladu mentioned this pull request Dec 15, 2022
Merged
@g11tech g11tech mentioned this pull request Dec 19, 2022
Merged
@jframe jframe mentioned this pull request Dec 20, 2022
Merged
2 tasks
@poojaranjan poojaranjan mentioned this pull request Jan 9, 2023
Closed
25 tasks
SamWilsn
SamWilsn reviewed Jan 10, 2023
View reviewed changes
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Outdated Show resolved Hide resolved
EIPS/eip-6122.md Show resolved Hide resolved
@MariusVanDerWijden MariusVanDerWijden dismissed a stale review via 739c54e January 11, 2023 11:58
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jan 11, 2023
@github-actions
Copy link

The commit 2189707 (as a parent of c7f49e0) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jan 11, 2023
mkalinin
mkalinin reviewed Jan 11, 2023
View reviewed changes
EIPS/eip-6122.md
The following additional rules are applied:

- Forks by timestamp MUST be scheduled at or after the forks by block (on mainnet as well as on private networks).
- An implementation of forkid verification of remote peer needs to filter the incoming forkids first by block then by timestamp.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that a client need to try comparing a block number of local head in the first place and then if it doesn't work fallback to the timestamp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we compare by block first and if the last block we have is past the remote block we fallback to the timestamp

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! We might want to add details on how exactly forkid validation rules are changed by this EIP to make this statement clearer

@terencechain terencechain mentioned this pull request Jan 16, 2023
Closed
@smartprogrammer93 smartprogrammer93 mentioned this pull request Jan 27, 2023
Merged
12 tasks
@gakonst gakonst mentioned this pull request Jan 30, 2023
Merged
@eth-bot eth-bot enabled auto-merge (squash) January 31, 2023 01:16
@eth-bot eth-bot merged commit ba361d7 into ethereum:master Jan 31, 2023
iseriohn pushed a commit to iseriohn/EIP-NFT-Rights-Management that referenced this pull request Feb 16, 2023
@MariusVanDerWijden @Pandapip1
@axic @djrtwo @lightclient @smartprogrammer93 @iseriohn
Add EIP-6122: Forkid checks based on timestamps (ethereum#6122)
df4c761 
* EIPS: add forkid by timestamp draft

* EIPS: add forkid by timestamp

* eip-6122: rename

* eip-6122: add martins comments, linter fixes

* Fix typo

* Fix typo introduced by me

* Apply suggestions from code review

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

* Fix typos highlighted in code review

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Ahmad Bitar <33181301+smartprogrammer93@users.noreply.github.com>

* eip-6122: fix comments

* eip-6122: fix comments

* eip-6122: fix comments

---------

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Ahmad Bitar <33181301+smartprogrammer93@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pandapip1 Pandapip1 Pandapip1 left review comments

@lightclient lightclient lightclient left review comments

@mkalinin mkalinin mkalinin left review comments

@djrtwo djrtwo djrtwo left review comments

@holiman holiman holiman left review comments

@axic axic axic left review comments

@smartprogrammer93 smartprogrammer93 smartprogrammer93 requested changes

@eth-bot eth-bot eth-bot approved these changes

@SamWilsn SamWilsn SamWilsn approved these changes

@g11tech g11tech g11tech left review comments

Assignees
No one assigned
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.