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

2681: Clarify that 2^64-1 is invalid as a transaction nonce #4437

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

axic
Copy link
Member

@axic axic commented Nov 9, 2021

@axic axic requested a review from holiman November 9, 2021 16:04
@axic
Copy link
Member Author

axic commented Nov 9, 2021

@holiman @gumb0 can you please check?

EIPS/eip-2681.md Outdated Show resolved Hide resolved
@gumb0
Copy link
Member

gumb0 commented Nov 9, 2021

Point 2 in Backwards Compatibility becomes not exactly accurate now. Maybe just change to "restriction is partially in place".

(types.txdata.AccountNonce name looks also outdated by now, but maybe not worth to change it. But just in case it's types.TxData.nonce() function now)

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM (with @gumb0 's suggestion)

@axic
Copy link
Member Author

axic commented Nov 9, 2021

@gumb0 @holiman merged the suggestions

@MicahZoltu @lightclient need also an editor approval (didn't set the PR non-draft in case the bot would auto-merge since I'm an editor)

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.

I'm pretty uncomfortable with making a normative change to a final EIP, even if the change makes sense. I would be more comfortable with a new EIP that makes 2^64-1 an invalid nonce (I doubt anyone would try to stop it from going through).

@@ -25,7 +25,7 @@ Lastly, this facilitates a minor optimisation in clients, because the nonce no l

Introduce two new restrictions retroactively from genesis:

1. Consider any transaction invalid, where the nonce exceeds `2^64-1`.
1. Consider any transaction invalid, where the nonce exceeds or equals to `2^64-1`.
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 a normative change, which makes me fairly uncomfortable making it as an edit to a final EIP. One could create a new EIP that adds 2^64-1 to the set of allowed nonces, but policy is that final EIPs can have non-normative changes only.

@holiman
Copy link
Contributor

holiman commented Nov 10, 2021

I'm pretty uncomfortable with making a normative change to a final EIP, even if the change makes sense. I would be more comfortable with a new EIP that makes 2^64-1 an invalid nonce (I doubt anyone would try to stop it from going through).

Not sure what you mean. As I see it, it's a clarification, not a change.

So the base idea is that nonces are limited between 0-2^64-1. This means that any transaction, which has nonce 2^64-1 is valid. However, when executing it, it will push the nonce to 2^64 (flipping the nonce back to 0 for the sender). This must be rejected, and the tx excluded.

So: the tx is "atomically" valid, but adding it causes an invalid state. Hence, we can shortcut this and say "tx with nonce 2^64-1 is invalid"

@MicahZoltu
Copy link
Contributor

when executing it, it will push the nonce to 2^64 (flipping the nonce back to 0 for the sender).

While I agree that the behavior you have described is reasonable, it is not spec compliant as currently written. A spec compliant client would increment the nonce to 2^64 and then allow no more transactions from that account going forward. I think the change proposed here is a good idea, I just argue that it is a normative change as it would cause a previously spec compliant client to become spec non-compliant and thus a new EIP is required.

The only specification details we have currently for this behavior is the following, which does not actually say that the nonce cannot be 2^64, it only says that a transaction submitted with a nonce >=2^64 is invalid.

Introduce two new restrictions retroactively from genesis:

  1. Consider any transaction invalid, where the nonce exceeds 2^64-1.
  2. The CREATE and CREATE2 instructions to abort with an exceptional halt, where the account nonce is 2^64-1.

@holiman
Copy link
Contributor

holiman commented Nov 10, 2021

A spec compliant client would increment the nonce to 2^64 and then allow no more transactions from that account going forward.

The abstract of the EIP in full is literally:

Limit account nonce to be between 0 and 2^64-1.

So I'd say that the change clarifies the technical implementation, so that it can no longer be mis-interpreted in the way you describe it. Because such a mis-interpretation goes against the description stated in the abstract

@MicahZoltu
Copy link
Contributor

I would accept a PR to this EIP that updates the abstract to correctly reflect the specification. The specification is the root of truth for the EIP, the abstract is meant to be a human readable summary. In this case the abstract doesn't match, so it should be fixed.

@holiman
Copy link
Contributor

holiman commented Nov 10, 2021

... Does it in any way change your opinion if I point out that the title is "Limit account nonce to 2^64-1", and it's by that title it has been discussed and approved?
Because if we change the abstract to be according to spec, we should also change the title to

"Limit account nonce to be max 2^64-1 for contracts, and 2^64 for EOA accounts"

I don't think such a change is in line with the thinking that went into writing it (as the eip authors can confirm), and more importantly, nor with the discussions that led to it's approval.

Both alternatives are pretty shitty really, IMO one is less shitty than the other.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Ugh this is unfortunate we didn't catch it before it become final. I am personally in favor of accepting this PR because I think 1) it really is clarifying the true purpose of the EIP. However, with respect to the Specification section it is a non-normative change. 2) I think it will cause more harm than good to create a new EIP with the small modification as it will be another strange artifact in Ethereum history for future developers to try and follow.

@axic
Copy link
Member Author

axic commented Nov 10, 2021

In an editor capacity I agree with @lightclient that merging this is causing the least confusion in the mid/long term. Potentially bringing this to ACD for wider attention could be useful, or even just privately reaching out to other clients whether they actually started implementing this.

As an alternate solution I considered the idea of an Errata section a while back, which could be added to final EIPs listing post-finalisation fixes. Not sure we want to complicate matters that much for this change though, but could definitely work for something like the EIP-820 vs EIP-1820 case.

@MicahZoltu
Copy link
Contributor

As a reminder, EIP editorial process isn't a dictatorship and we (for the first time in a long time) seem to actually have 3 active editors. Since @axic and @lightclient seem to think that a normative change to this EIP is reasonable in this case, I can just 🙈 and they can merge. 😬

@lightclient lightclient marked this pull request as ready for review November 11, 2021 05:26
@lightclient lightclient merged commit 14966f8 into ethereum:master Nov 11, 2021
PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
…#4437)

* 2681: Clarify that 2^64-1 is invalid as a transaction nonce

* Update EIPS/eip-2681.md

Co-authored-by: Andrei Maiboroda <[email protected]>

* Update eip-2681.md

Co-authored-by: Andrei Maiboroda <[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.

5 participants