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-1344: Update uint256 field size to uint64 #2263

Closed
wants to merge 2 commits into from

Conversation

fubuloubu
Copy link
Contributor

EIPS/eip-1344.md Outdated Show resolved Hide resolved
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

I think this looks good.

@axic
Copy link
Member

axic commented Sep 7, 2019

cc @holiman @shemnon @winsvega who are involved with test generation

@winsvega
Copy link
Contributor

winsvega commented Sep 8, 2019

Basically you mentioned all of us, who involved

Chainid tests here:
ethereum/tests#627

@chfast
Copy link
Member

chfast commented Sep 13, 2019

When merge?

@fubuloubu
Copy link
Contributor Author

I think this is ready to merge, although I'm not sure if every client lead is aware of this change.

cc @sorpaas @karalabe

@shemnon
Copy link
Contributor

shemnon commented Sep 13, 2019

So how would we do a reference test to validate this? or is it validatable via reference test?

@fubuloubu
Copy link
Contributor Author

Since it is possible to sign with with a chain ID > 64 bits (up to a value of ~255 bits IIRC), you could have an EIP-155 transaction validation that uses 65 bits and it should fail validation in some way. The opcode should also revert if the value of chain ID is > 64 bits.

@holiman
Copy link
Contributor

holiman commented Sep 13, 2019

The opcode should also revert if the value of chain ID is > 64 bits.

Nope. No revertals for this opcode. It simply should not be possible to configure the node with a chainid above 64 bits.

you could have an EIP-155 transaction validation that uses 65 bits and it should fail validation in some way

So yes, that would obviously never equal the 64-bit chainid

@karalabe
Copy link
Member

karalabe commented Sep 16, 2019

This suggestions seems a arbitrary, not fully defined and late to me.

  • The diff states that the chain id is a 64 bit value. Well, none of the previous EIPs that created chain ids mentioned this, so from their perspective, it's a 256bit int. What happens if I have a network that ha a large chain id? What will this EIP do? It completely avoids mentioning what the fallback is.
  • Why 64 bits? Why not 63, to make V fit into 64? If V won't fit into 64 either way, and if solidity numbers are 256bit ints anyway, what's the value of this arbitrary artificial limit.
  • We're talking about forking in 2 weeks, and we're making again changes to EIPs?

Imho the only meaningful way forward at this point is to keep the EIP as is, chain id being a 256 bit number, and if we think that it should be 64, then open a new EIP that redefines it and updates it throughout the Ethereum spec (e.g. signature schemes too) and not just one EIP.

@axic
Copy link
Member

axic commented Sep 16, 2019

To be fair this isn't something proposed last week, but has been under discussion since chaind was proposed and the current discussion stems from a month ago.

The problem with 256-bit chainid is that clients won't be able to handle it, unless their transaction encoding works on unrestricted bignums (and not just 256-bit bignums).

I really feel like this hardfork is rushed for no reason and dismissing every useful proposal just to be "done with the hard fork".

@karalabe
Copy link
Member

The problem with 256-bit chainid is that clients won't be able to handle it, unless their transaction encoding works on unrestricted bignums (and not just 256-bit bignums).

They need to handle that even now, since EIP155 already introduced this. So this PR limiting it to 64 bits makes it a weird cornercase that's not in line with EIP155. And this EIP not fixing EIP155 means that the whole fix is moot, because clients still need to handle large V-s properly.

@fubuloubu
Copy link
Contributor Author

@karalabe:

  1. This PR doesn't exist as a change to EIP-155, more a clarification. The change is to EIP-1344 because it had spelt out an exact data size for the EVM to return in it's errata. I do agree however that EIP-155 implementation is affected, but this should only make it clearer for clients because EIP-155 makes no considerations for size, even having an arbitrarily large size (except through the lack of an explanation).
  2. At all tables of Chain IDs we have seen, no network has picked a value larger than 64 bits. It should therefore be safe to cap at this amount.
  3. 63 bits could be better than 64 bits, because the math would fit into the smaller 64-bit integer. As it is, since unrestricted size on this parameter was not a clear assumption in EIP-155, there could be an overflow/encoding vulnerability in how different clients handle EIP-155. Because there was no expressed limit, there was never any testing done for much larger values either.

@tjayrush
Copy link

I am an un-involved observer, I have nothing to do with the hard-fork planning nor this EIP, and perhaps no claim to make the following comment, but I do have 39.25 years of experience with software development, so I'll go for it.

There are a few things that are quite obvious: (1) if 63 bits would be safer, then it should be 63 bits; (2) the spec should be explicit about this size of the item; (3) if you're going to change the spec, then there MUST be testing; and (4) this feels a lot like a change to an EIP without testing weeks for deployment.

What's the purpose of the EIP 1 process if it doesn't apply when we need it to not apply (clarifications included)?

@fubuloubu
Copy link
Contributor Author

fubuloubu commented Sep 16, 2019

So, without this change merged, the EIP is still well-formed enough to deploy as is, as it is consistent with any currently known network that uses EIP-155 (all of which use less than 64/256 bits).

Perhaps the greater issue is that @axic identified a scenario with handling EIP-155 (and EIP-1344 by extension) where chain ID being unlimited in size would cause a logical error that could be disruptive to how RLP encoding is performed on transactions.

This modification (which should also be made to EIP-155) should actually be a follow-up EIP that can be implemented outside of the hardfork process since the mainnet and every other known network would not run into this scenario, but we want to make sure to bound this size so the issue is never encountered.

I will withdraw this amended change and communicate to the client teams that have implemented this update so far to revert back to EIP-1344 returning a uint256 instead of a uint64, and move back to the assumption in EIP-155 that chain ID has a potentially unlimited size.

@fubuloubu
Copy link
Contributor Author

fubuloubu commented Sep 20, 2019

Got no response on this, so I am going to move ahead with the suggestion from @karalabe that this change proposal be dropped in favor of the implicit assumption from EIP-155 that chain ID is potentially "limitless" in size. This PR will therefore be dropped in favor of #2294, which would create this limit explicitly.

The original wording of EIP-1344 is still accurate and required because the EVM will return a 256-bit field in response to calling the CHAINID opcode. If a value for Chain ID is chosen above this limit, it will be undefined behavior and could lead to potential security issues for chains that make that choice. This does not affect Ethereum, or known Ethereum-based chains, so the implementation of EIP-1344 should remain unaffected.

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.

8 participants