-
Notifications
You must be signed in to change notification settings - Fork 794
Conversation
Signed-off-by: Julian Popescu <[email protected]>
Signed-off-by: Julian Popescu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes the nonce to be serialized without leading 0's as specified by the JSON-RPC spec, which therefore may cause deserialization problems with other libraries that expect a fixed byte array.
I don't think we should deviate from the spec just because other libraries do that.
When encoding quantities (integers, numbers): encode as hex, prefix with "0x", the most compact representation (slight exception: zero should be represented as "0x0").
@@ -81,7 +81,7 @@ pub struct Block<TX> { | |||
pub mix_hash: Option<H256>, | |||
/// Nonce | |||
#[cfg(not(feature = "celo"))] | |||
pub nonce: Option<U64>, | |||
pub nonce: Option<H64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does H64 serialize, deserialize compared to U64?
H64 is a hash type, and since nonce is an uint, this change doesn't seem necessary,
also H64 lacks common conversion trait impls for primitive types and I'm not sure it even as a as_u64
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H64 will encode as a byte array, and not like number so it won't serialize to the most compact representation and it will expect the length to be 16 on deserialization. The reason this change is necessary is because a client of anvil (in my experience here graph-node) may expect the the value to have a length of 16 (or 18 with 0x prefix) and will fail to deserialize something like 0x0. Because the specs say this value is Bytes8 https://github.com/ethereum/execution-specs/blob/master/src/ethereum/muir_glacier/eth_types.py#L118 and geth also implements it as such, I think failing to deserialize 0x0 for the nonce is reasonable
Did we encounter this as a bug in someone's code, or is this a proactive change? |
I just double checked the RPC docs for blocks, nonce is indeed so @jpopesculian you're totally right, and we should change this to H64, |
@gakonst we can merge this, the failing tests are due to installed Anvil which uses the old nonce format |
Motivation
geth
and other libraries represent the block nonce as a 8 byte fixed array.ethers-rs
uses an unsigned 64 bit integer to represent the nonce. This causes the nonce to be serialized without leading 0's as specified by the JSON-RPC spec, which therefore may cause deserialization problems with other libraries that expect a fixed byte array. This is the case withweb3
for example which fails to deserialize the nonce because it usesH64
Solution
Use
H64
instead ofU64
for noncePR Checklist