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

Time Based Upgrade Transitions #2456

Closed
wants to merge 6 commits into from
Closed

Time Based Upgrade Transitions #2456

wants to merge 6 commits into from

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jan 7, 2020

A process to specify network upgrades relative to a point in time instead of a fixed block number.

In brief change the fork process so that that network upgrades would activate at the second “round thousand” of blocks after a specific time on the network, as reported in the blockheader. Why the second round thousand? In case the first round thousand is within an ommer re-org and to keep miners from playing too many games lying about the block time.

A process to specify network upgrades relative to a point in time
instead of a fixed block number.

Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Explicitly add chossing a transiton time.  Reorder some of the spec.
Change words to make it clear that transition time trumps all.

Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>
Signed-off-by: Danno Ferrin <[email protected]>

- The upgrade has not activated already.
- The timestamp of the block is on or after the `TRANSITION_TIME`.
- The previous Transition Eligible Block was on or after the `TRANSITION_TIME`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ambiguous, since and I'm wondering if there's some typo...?
Because if the timestamp of N is on TRANSITION_TIME, then the timestamp of N-1 cannot also be on TRANSITION TIME. So as I read it, we can totally skip the second bullet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll tighten up the wording on the second bullet point. "the block" of the second bullet point needs to be a Transition Eligible Block. So point 2 is looking at block n where (n % 1000) == 0 and point 3 is looking at block n - TRANSISION_INCREMENT and hence ((n - TRANSITION_INCREMENT) % 1000 == 0)

@carver
Copy link
Contributor

carver commented Jan 11, 2020

I'm very in favor of finding a way to fork on timestamp 👍

The biggest con to this particular approach is losing the ability to look at a single header to decide which rules (VM) should be applied to its execution. Block-number-forking is conceptually clear, and lends itself to an API we're happy with in Trinity: get_vm(header). It would be a shame to lose that, especially when there might still be other options.

As far as I know, the two main concerns about direct timestamp forking are:

  1. It opens new uncle validation edge cases
  2. Some risk that miners will have a new incentive to manipulate timestamp away from current "true" time

So let's at least check if we can directly address these, first.

For example, maybe adding a rule that "uncles must have a timestamp older than the header that includes them" would get us most of the way there. (So we don't have to deal with uncles on a future fork when the including header is on an old fork). Also, we probably need to formalize that we only validate uncle PoW, and according to the rules of the uncle header's VM.

Also, some more analysis about miner incentives and timestamp-tweaking capabilities seems worthwhile, but my intuition is that the small-time gaming that might happen is not catastrophic. Any significant deviation would require a majority coalition of miners, as far as I can tell.

@shemnon
Copy link
Contributor Author

shemnon commented Jan 21, 2020

@carver can you copy your comment to the discussion-to thread at https://ethereum-magicians.org/t/time-based-upgrade-transitions/3902 please?

@Souptacular
Copy link
Contributor

@shemnon Do you want this merged?

@vbuterin
Copy link
Contributor

I would propose reducing the lookback from 1024 to 256 or less. The reason is that the 1024 lookback implicitly adds the last 1024 blocks of history into the state, increasing the data that clients are required to hold; this could lead to edge cases for clients that haven't synced history yet. A lookback of 256 has no such risk, because the last 256 blocks are already required to be accessible to the BLOCKHASH opcode.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

This looks good for getting merged as a draft aside from the extraneous comments (new policy is to remove them).

Comment on lines +25 to +26
<!--The motivation is critical for EIPs that want to change the Ethereum protocol. It should clearly explain why the existing protocol specification is inadequate to address the problem that the EIP solves. EIP submissions without sufficient motivation may be rejected outright.-->

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!--The motivation is critical for EIPs that want to change the Ethereum protocol. It should clearly explain why the existing protocol specification is inadequate to address the problem that the EIP solves. EIP submissions without sufficient motivation may be rejected outright.-->

Comment on lines +67 to +70
palindromes. Retaining the mainnet tradition is easy, but specifying a palindrome algorithm for
testnets seemed excessive. As a compromise round decimal numbers (ending in `000`) are proposed for
mainnet and round hexadecimal numbers (ending in `0x000`, `0x400`, `0x800`, or `0xc00`) is proposed
for testnets.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessarily complex. The specific block number choice is pretty arbitrary in all cases, and having a different strategy for mainnet and testnet seems like a waste of engineering effort for what is essentially the color of the boathouse being different in mainnet and testnets.

Comment on lines +107 to +108

<!--All EIPs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The EIP must explain how the author proposes to deal with these incompatibilities. EIP submissions without a sufficient backwards compatibility treatise may be rejected outright.-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!--All EIPs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The EIP must explain how the author proposes to deal with these incompatibilities. EIP submissions without a sufficient backwards compatibility treatise may be rejected outright.-->

Comment on lines +144 to +149
## Test Cases

<!--Test cases for an implementation are mandatory for EIPs that are affecting consensus changes. Other EIPs can choose to include links to test cases if applicable.-->

> To be completed once Eligible for Inclusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Test Cases
<!--Test cases for an implementation are mandatory for EIPs that are affecting consensus changes. Other EIPs can choose to include links to test cases if applicable.-->
> To be completed once Eligible for Inclusion

This isn't currently a required suggestion, consider just leaving it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Test Cases
<!--Test cases for an implementation are mandatory for EIPs that are affecting consensus changes. Other EIPs can choose to include links to test cases if applicable.-->
> To be completed once Eligible for Inclusion
## Test Cases
TBD

Alternatively, if you want to leave it in so you don't forget that is fine as well, just remove the comment.

Comment on lines +150 to +155
## Implementation

<!--The implementations must be completed before any EIP is given status "Final", but it need not be completed before the EIP is accepted. While there is merit to the approach of reaching consensus on the specification and rationale before writing code, the principle of "rough consensus and running code" is still useful when it comes to resolving many discussions of API details.-->

> To be completed once Eligible for Inclusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Implementation
<!--The implementations must be completed before any EIP is given status "Final", but it need not be completed before the EIP is accepted. While there is merit to the approach of reaching consensus on the specification and rationale before writing code, the principle of "rough consensus and running code" is still useful when it comes to resolving many discussions of API details.-->
> To be completed once Eligible for Inclusion
Suggested change
## Implementation
<!--The implementations must be completed before any EIP is given status "Final", but it need not be completed before the EIP is accepted. While there is merit to the approach of reaching consensus on the specification and rationale before writing code, the principle of "rough consensus and running code" is still useful when it comes to resolving many discussions of API details.-->
> To be completed once Eligible for Inclusion
## Implementation
TBD

Same comment as above.

Comment on lines +157 to +158

<!--All EIPs must contain a section that discusses the security implications/considerations relevant to the proposed change. Include information that might be important for security discussions, surfaces risks and can be used throughout the life cycle of the proposal. E.g. include security-relevant design decisions, concerns, important discussions, implementation-specific guidance and pitfalls, an outline of threats and risks and how they are being addressed. EIP submissions missing the "Security Considerations" section will be rejected. An EIP cannot proceed to status "Final" without a Security Considerations discussion deemed sufficient by the reviewers.-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!--All EIPs must contain a section that discusses the security implications/considerations relevant to the proposed change. Include information that might be important for security discussions, surfaces risks and can be used throughout the life cycle of the proposal. E.g. include security-relevant design decisions, concerns, important discussions, implementation-specific guidance and pitfalls, an outline of threats and risks and how they are being addressed. EIP submissions missing the "Security Considerations" section will be rejected. An EIP cannot proceed to status "Final" without a Security Considerations discussion deemed sufficient by the reviewers.-->

@shemnon
Copy link
Contributor Author

shemnon commented Sep 28, 2020

@shemnon Do you want this merged?

I no longer participate in the EIP process.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Sep 28, 2020

I no longer participate in the EIP process.

I'm interpreting this statement to mean that the Author no longer wishes to pursue this EIP and I'll close the PR. @shemnon please re-open this PR if I have misunderstood your meaning! If someone else wishes to pursue this specification, just create a new PR with yourself as the author and a new EIP number (usually the PR number).

@MicahZoltu MicahZoltu closed this Sep 28, 2020
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.

7 participants