Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Account addresses in genesisStates should be prefixed with '0x' #32

Closed
vpulim opened this issue Nov 26, 2018 · 6 comments
Closed

Account addresses in genesisStates should be prefixed with '0x' #32

vpulim opened this issue Nov 26, 2018 · 6 comments

Comments

@vpulim
Copy link
Contributor

vpulim commented Nov 26, 2018

Many of the addresses in the alloc field in the genesisStates json files aren't prefixed with 0x. This causes state roots computed from these files to be incorrect when using ethereumjs/merkle-patricia-tree to load these states. The reason for this is that the Trie.prototype.put function https://github.com/ethereumjs/merkle-patricia-tree/blob/master/src/baseTrie.js#L87) calls ethUtil.toBuffer() on the account address which treats the address as a plain string instead of a hex string if it's not prefix with a 0x.

As a fix, we can either prefix all of the addresses with a 0x in the alloc field of each genesisStates file or modify genesisStates/index.js to programmatically add a '0x' prefix if its missing.

@holgerd77
Copy link
Member

I think we should directly fix the addresses, should be doable with some clever search-and-replace.

@holgerd77
Copy link
Member

I think the real solution to this is really to stop this nonsense and be more strict in the utility functions in ethereumjs-util and just throw on stuff like this on the next breaking release. Then people can handle things like that properly.

We should really put some dedicated thought into that, makes probably sense to do this along the TypeScript transition (or directly before as a first and separate step).

@holgerd77
Copy link
Member

What is with the values, there is also some hex-decimal mix on this. Just have checked, at least this is consistent along a separate chain genesis file (e.g. mainnet.json). Would you also say that this is at least correctly working for now? My assumption without following the code paths is that non-prefixed values are regarded as being decimal?

@vpulim
Copy link
Contributor Author

vpulim commented Nov 27, 2018

I think the real solution to this is really to stop this nonsense and be more strict in the utility functions in ethereumjs-util and just throw on stuff like this on the next breaking release. Then people can handle things like that properly.

We should really put some dedicated thought into that, makes probably sense to do this along the TypeScript transition (or directly before as a first and separate step).

Yes, totally agree. Great example of where TypeScript can really help.

@vpulim
Copy link
Contributor Author

vpulim commented Nov 27, 2018

What is with the values, there is also some hex-decimal mix on this. Just have checked, at least this is consistent along a separate chain genesis file (e.g. mainnet.json). Would you also say that this is at least correctly working for now? My assumption without following the code paths is that non-prefixed values are regarded as being decimal?

Since no one uses the genesisStates files yet, it's hard to make a judgment about if the values are working correctly. It's really up to the user of the genesisStates to properly handle the mixed types. I think there can be three possible types for account balances:

  1. Number: This is a simple case where the value is obviously a decimal integer, but it can't be used for large numbers (more than 53 bits)
  2. String w/ 0x prefix: This is obviously a hex value and should be loaded with BN to hold the number value. This can be used for numbers larger than 53 bits.
  3. String w/o 0x prefix: This is the confusing case and should be avoided (in my opinion). It's unclear whether this is a raw string value (not possible for account balances), a number encoded in hex but with the 0x prefix missing, or a regular decimal number as a string (not hex-encoded).

Until we switch to TypeScript, my preference would be to always use a 0x prefix and hex encode numbers when they are encoded as string, or just use a plain number value (not a string).

@holgerd77
Copy link
Member

Closed by #33.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants