internal/ethapi: quantity-encode storage keys in eth_getProof response
#27309
Conversation
holiman
changed the title
eth_getProof response
|
Updated the test to match the changed encoding. Note that I believe this will impact hive RPC compatibility tests (given the changed RPC output). But given that this is a more-strict encoding of the same data, it should not impact any properly implement RPC clients |
internal/ethapi/api.go
Outdated
| @@ -680,6 +680,7 @@ func (s *BlockChainAPI) GetProof(ctx context.Context, address common.Address, st | |||
| // create the proof for the storageKeys | |||
| for i, hexKey := range storageKeys { | |||
| key, err := decodeHash(hexKey) | |||
| keyInt := (*hexutil.Big)(new(big.Int).SetBytes(key[:])) | |||
There was a problem hiding this comment.
It's a bit unorthodox to do this operation, involving key, before checking/handling the err. It's safe, in this instance, but not "The Way" :)
There was a problem hiding this comment.
my go is far from idiomatic
|
Having thought about this one some more, I have somewhat changed my stance. Originally, I thought that there was no reason to ever return non-canonical keys, period. If user submits some "invalid" And for that particular case, I still think so. However, there's another situation that can happen too, which is when the user expects the input/output to be 32-byte hashes, fully prefixed by zeroes. As we have it right now, the user can submit keys as 32-byte hashes, and obtain response proofs containing 32-byte hashes. So right now, our API isn't opinionated, it allows either type. I think this is kind of neat, with the only drawback that it also allows totally off-standard fomats like One way to handle this would be to check they key it is "hash" (full 32-byte zero-prefixed),
|
|
So the new behavior becomes
This means that correctly-formatted output cannot be identified with a regex, without knowing the input length, which seems like a wart in the specification (which uses regex definitions) |
|
I think @holiman did not mean the specification should be changed. The spec says it's a quantity and it should remain one. He just wants to preserve the current behavior for people calling this API method with 32-byte inputs. It makes sense, sort of. |
|
Should be done now. @holiman PTAL. |
ethereum#27309) This changes the eth_getProof method implementation to re-encode the requested storage keys, canonicalizing them in the response. For backwards-compatibility reasons, go-ethereum accepts non-canonical hex keys. Accepting them is fine, but we should not mirror invalid inputs into the output. Closes ethereum#27306 --------- Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Felix Lange <fjl@twurst.com>
ethereum#27309) This changes the eth_getProof method implementation to re-encode the requested storage keys, canonicalizing them in the response. For backwards-compatibility reasons, go-ethereum accepts non-canonical hex keys. Accepting them is fine, but we should not mirror invalid inputs into the output. Closes ethereum#27306 --------- Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Felix Lange <fjl@twurst.com>
ethereum#27309) This changes the eth_getProof method implementation to re-encode the requested storage keys, canonicalizing them in the response. For backwards-compatibility reasons, go-ethereum accepts non-canonical hex keys. Accepting them is fine, but we should not mirror invalid inputs into the output. Closes ethereum#27306 --------- Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Felix Lange <fjl@twurst.com>
… response (ethereum#27309)" This reverts commit ff8659c.
… response (ethereum#27309)" This reverts commit ff8659c.
… response (ethereum#27309)" This reverts commit ff8659c.
Closes #27306
I am not completely familiar with the json marshalling system. I've tried to pattern match on other code, but may have gotten something wrong