fix: change type of formats.receipt.root hash -> hex

[bguiz]

What

• Updated the validator for transaction receipts to validate root as hex instead of hash
• Added new test cases targeting the Formatter class directly
  • NOTE: There appears to be no current (direct) coverage for Formatter at the moment, so added this in a new test file

Why

• Currently formatter expects a transaction receipt's root property to be a hash, so 32 bytes
• However, that does not match definition used in geth,
  where this maps to the PostState fields within the Receipt struct,
  thus the current validation appears to be based on convention
• This is currently causing issue with transactions submitted to
  a non-Ethereum, but EVM and JSON-RPC compliant, network

Refs

• Relevant definition in geth implementation: https://github.com/ethereum/go-ethereum/blob/ce9a289/core/types/receipt.go#L49-L52
• Issue in which this was discovered bug: can't deploy to JSON-RPC network, where equivalent deployment via Remix and Truffle works NomicFoundation/hardhat#694 (comment)

[bguiz]

👋 @ricmoo @alcuadrado I am not sure about the conventions in this project, so I've made some guesses:

• whether to commit the typescript build output -> noticed that it was not git ignored, so committed them
• where to put the formatter test file -> put it in the same folder as the provider test file - and it appears most of the test cases are there anyway

LMK if I need to change anything!

[ricmoo]

You don't need to worry about committing any build files. The TypeScript is all you need to worry about. The /admin folder contains myriad scripts I use to generate all the other files in all the needed flavours and performs sanity checks. :)

I'll look in to this and read through your links. Thanks! :)

[bguiz]

You don't need to worry about committing any build files. The TypeScript is all you need to worry about. The /admin folder contains myriad scripts I use to generate all the other files in all the needed flavours and performs sanity checks. :)

Ah didn't think to look inside /admin - should I revert 435fd4d (where the build output is added), or leave it as it is now?

Semi-related: How do I trigger CI on this PR? I noticed that some PRs have them, but some (such as this one) do not.

I'll look in to this and read through your links. Thanks! :)

No problem

[ricmoo]

Oh no, don't worry about it. I usually go through a PR and pick and choose the necessary parts. And then I'll run the admin scripts. :)

I actually am not sure about the CI. I only really want it running on my own pushes. I obviously don't mind forks running their own actions on their forked repo, but it seems like a weird security issue to let other people's Actions show up on this repo as it could imply endorsement and expose artifacts that could contain malicious code.

The GitHub Actions are very weird, in how they selectively choose things and I cannot figure out how/why. It did originally allow another persons Workflow to run under my repos tab, and the name of the workflow is inconsistent on the workflow list...

[bguiz]

Oh no, don't worry about it. I usually go through a PR and pick and choose the necessary parts. And then I'll run the admin scripts. :)

OK cool - will leave as is.

I actually am not sure about the CI. I only really want it running on my own pushes. I obviously don't mind forks running their own actions on their forked repo, but it seems like a weird security issue to let other people's Actions show up on this repo as it could imply endorsement and expose artifacts that could contain malicious code.
The GitHub Actions are very weird, in how they selectively choose things and I cannot figure out how/why. It did originally allow another persons Workflow to run under my repos tab, and the name of the workflow is inconsistent on the workflow list...

Yeah, just checking in case I missed something - I was looking for a contributors' doc or something to that effect, so just asking you instead.

[ricmoo]

Good point, I’ll update the “contributing” link in the docs. :)

[bguiz]

Good point, I’ll update the “contributing” link in the docs. :)

Awesome.

BTW, any word on the actual contents of this patch?

[ricmoo]

I still need to look more into this and research before I can comment on the contents of the patch...

I've got a few other higher priority things I want to get to first, but I should have time next week to look into this. :)

[bguiz]

I've got a few other higher priority things I want to get to first, but I should have time next week to look into this. :)

OK sure ... the crux of the change is just 2 lines of code (hash -> hex), just saying!

[ricmoo]

Small changes can cause non-trivial issues and can have long-term impacts though. ;)

[bguiz]

Very true!

[bguiz]

Bump! Have you had a chance to check this out yet?

[bguiz]

@ricmoo any word?

[bguiz]

@ricmoo any word on this?

[bguiz]

@ricmoo are you looking at this PR?

[bguiz]

Adding this too:

The current validation logic is not compliant with EIP658, in which 0x00 and 0x01 are valid values for root.

Specific comments in this linked issue

[bguiz]

Taking a look at the JSON-RPC docs, I found these two references, copied below.
In combination, they indicate that the current implementation for the transaction receipt validator is only correct
for pre-Byzantium values of root;
however to cater to post-Byzantium values, 0x1 and 0x0 should also be allowed by the transaction receipt validator.

@ricmoo this means that the implementation is the PR needs to be modified:
formats.receipt.root is currently Formatter.allowNull(hex)
and should be changed to Formatter.allowNull(Formatter.or(hex, number)).
I haven't checked if there is a Formatter.or (or equivalent), and can create that if it does not exist.

I have yet to receive a response on this PR, so LMK if this is something you are open to.

---

https://eth.wiki/json-rpc/API#eth_gettransactionreceipt

It also returns either :

• root : DATA 32 bytes of post-transaction stateroot (pre Byzantium)
• status: QUANTITY either 1 (success) or 0 (failure)

https://eth.wiki/json-rpc/API#hex-value-encoding

HEX value encoding

At present there are two key datatypes that are passed over JSON: unformatted byte arrays and quantities. Both are passed with a hex encoding, however with different requirements to formatting:

When encoding QUANTITIES (integers, numbers): encode as hex, prefix with “0x”, the most compact representation (slight exception: zero should be represented as “0x0”). Examples:

• 0x41 (65 in decimal)
  

• 0x400 (1024 in decimal)
  

• WRONG: 0x (should always have at least one digit - zero is “0x0”)
  

• WRONG: 0x0400 (no leading zeroes allowed)
  

• WRONG: ff (must be prefixed 0x)
  

When encoding UNFORMATTED DATA (byte arrays, account addresses, hashes, bytecode arrays): encode as hex, prefix with “0x”, two hex digits per byte. Examples:

• 0x41 (size 1, “A”)
  

• 0x004200 (size 3, “\0B\0”)
  

• 0x (size 0, “”)
  

• WRONG: 0xf0f0f (must be even number of digits)
  

• WRONG: 004200 (must be prefixed 0x)
  

[ricmoo]

I've been trying to reproduce this without luck. Is there any hosted service and a transaction hash you can provide that causes this issue to occur?

I don't quite understand your interpretation, I think. For example:

• root : DATA 32 bytes of post-transaction stateroot (pre Byzantium)
• status: QUANTITY either 1 (success) or 0 (failure)

Means that either a) pre-byzatium uses the property root and have a 32 byte value (and status property is null) or b) post-byzantium uses the property status is a quantity value (and the root property is null). This is what ethers enforces.

I haven't seen anywhere though that allows root to take the values 0x0 or 0x1 or the byte equivalents (i.e. 0x00 or 0x01).

I don't mind relaxing this requirement (since the TypeScript definitions don't need to change), but I need to be able to reproduce it first.

I've found the transaction hash 0x207443dca7009383371745242e4a9cd4bf83f2ff70ddf0c827d27c8f02c5b28c in your issue here, but have no idea what network to connect to to get that.

Please send a transaction hash and JSON-RPC url I can use to test against. Thanks! :)

[bguiz]

I don't mind relaxing this requirement (since the TypeScript definitions don't need to change),

Great!

The way that I'd characterise this is that the Ethereum JSON-RPC docs allows for a looser validation logic than what is presently implemented in ethers.js, so relaxing said validation logic is a good outcome.

but I need to be able to reproduce it first.

Please send a transaction hash and JSON-RPC url I can use to test against. Thanks! :)

Sure, here is an example, for transaction 0xebd560ea53a5853a7c994b1614634d97a75b00171026ad4d16911eacef113496 on RSK Testnet:

curl \
  -X POST \
  -H "Content-Type:application/json" \
  --data '{"jsonrpc":"2.0","method":"eth_getTransactionReceipt","params":["0xebd560ea53a5853a7c994b1614634d97a75b00171026ad4d16911eacef113496"],"id":1}' \
  https://public-node.testnet.rsk.co

{
   "result" : {
      "from" : "0xca49c0dff330bc85b8c43fca88b404d1a0608de2",
      "root" : "0x01",
      "transactionIndex" : "0x0",
      "to" : "0xad29045d80177930253090a76f3be7a5f5ad565e",
      "status" : "0x1",
      "cumulativeGasUsed" : "0xdbcd",
      "blockNumber" : "0x1b207",
      "transactionHash" : "0xebd560ea53a5853a7c994b1614634d97a75b00171026ad4d16911eacef113496",
      "logs" : [
         {
            "transactionIndex" : "0x0",
            "blockNumber" : "0x1b207",
            "transactionHash" : "0xebd560ea53a5853a7c994b1614634d97a75b00171026ad4d16911eacef113496",
            "logIndex" : "0x0",
            "data" : "0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000645a686699000000000000000000000000000000000000000000000221c5d215e15f65bdc0000000000000000000000000000000000000000000000000000000005d5538740000000000000000000000006d71dbf6b75f52d0aabeb8466741a0c1319427e0",
            "address" : "0xad29045d80177930253090a76f3be7a5f5ad565e",
            "topics" : [
               "0x5a68669900000000000000000000000000000000000000000000000000000000",
               "0x000000000000000000000000ca49c0dff330bc85b8c43fca88b404d1a0608de2",
               "0x000000000000000000000000000000000000000000000221c5d215e15f65bdc0",
               "0x000000000000000000000000000000000000000000000000000000005d553874"
            ],
            "blockHash" : "0x3689cbd699762b3eebc24ff2782c662b835e1472338007d1e00c6b11583a1add"
         },
         {
            "transactionIndex" : "0x0",
            "transactionHash" : "0xebd560ea53a5853a7c994b1614634d97a75b00171026ad4d16911eacef113496",
            "logIndex" : "0x1",
            "blockNumber" : "0x1b207",
            "blockHash" : "0x3689cbd699762b3eebc24ff2782c662b835e1472338007d1e00c6b11583a1add",
            "address" : "0x6d71dbf6b75f52d0aabeb8466741a0c1319427e0",
            "data" : "0x000000000000000000000000000000000000000000000221c5d215e15f65bdc0",
            "topics" : [
               "0x296ba4ca62c6c21c95e828080cb8aec7481b71390585605300a8a76f9e95b527"
            ]
         }
      ],
      "gasUsed" : "0xdbcd",
      "logsBloom" : "0x00000010002000000000045000000000000000000000000000000000000000000000000000010010000000000000000000000000000000000000000000000000100000000000000000000000000000000400000000000000000000000000000000000800000000000000000080000000000002000000000001000000000000000000000400000000000000000000010000000000000000002000000020000000000000000000000000000000000000000080000000000480000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000",
      "contractAddress" : null,
      "blockHash" : "0x3689cbd699762b3eebc24ff2782c662b835e1472338007d1e00c6b11583a1add"
   },
   "jsonrpc" : "2.0",
   "id" : 1
}

The part that triggers the validation error in ethers.js is "root" : "0x01".

Let me know if you need any more info!

[ricmoo]

I've got a local fix for this, I'm just testing it now.

[ricmoo]

This should be addressed in 5.0.27. Please try it out and let me know if you still have any issues.

Thanks! :)

[ricmoo]

It sounds like this is working, so I'm going to close this. Please re-open if you have any more issues.

Thanks! :)

[bguiz]

Status update:

This now works some of the time, but not all of the time. Reopened this old related issue for further investigation - rsksmart/rskj#1328

[ricmoo]

@bguiz can you provide an example transaction which returns "0x"? What should that be converted to?