Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

bug: JSON-RPC serialisation of hexadecimal strings in transaction receipt #1328

Closed
bguiz opened this issue Oct 6, 2020 · 8 comments
Closed
Labels
compatibility Ethereum compatibility core ⚛️

Comments

@bguiz
Copy link

bguiz commented Oct 6, 2020

What

  • change the serialisation of of the transactionIndex and status fields as follows:
    • transactionIndex: '0x0' --> transactionIndex: '0x00'
    • status: '0x1' --> status: '0x01'
    • see the details section below for a copy of a transaction receipt produced by rskj running on regtest

Why

  • Attain JSON-RPC spec compliance and implementation compatibility with geth
    • hexadecimal field values should be a 0x prefix followed by an even number of characters (0 through f), with a minimum of 2 characters (excluding the prefix).
  • Note that this will not result in any difference when the the transaction receipt is consumed by web3.js, which does not perform response validation in this scenario. However it may result in a rejection when the transaction receipt is consumed by other libraries that do perform response validation in this scenario, for example, ethers.js.

Refs

Details

{
  transactionHash: '0x207443dca7009383371745242e4a9cd4bf83f2ff70ddf0c827d27c8f02c5b28c',
  transactionIndex: '0x0',
  blockHash: '0x64f3a04bb67aea8684e1aea3f7673a5d6422cc018f592903490d61f6e456c507',
  blockNumber: '0xb4c',
  cumulativeGasUsed: '0xd93c0',
  gasUsed: '0xd93c0',
  contractAddress: '0xda7ce79725418f4f6e13bf5f520c89cec5f6a974',
  logs: [],
  from: '0xcd2a3d9f938e13cd947ec05abc7fe734df8dd826',
  to: null,
  root: '0x01',
  status: '0x1',
  logsBloom: '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'
}
@aeidelman aeidelman added compatibility Ethereum compatibility core ⚛️ labels Oct 6, 2020
@fedejinich
Copy link
Contributor

fedejinich commented Oct 6, 2020

nice catch @bguiz !

@pmprete
Copy link

pmprete commented Oct 10, 2020

Note that this will not result in any difference when the the transaction receipt is consumed by web3.js, which does not perform response validation in this scenario. However it may result in a rejection when the transaction receipt is consumed by other libraries that do perform response validation in this scenario, for example, ethers.js.

In case someone has this issue until this fix is merged, the problem happens with ethersproject and the solution is to change the provider filters

import { Web3Provider } from '@ethersproject/providers'

function getLibrary(provider: any): Web3Provider {
  const library = new Web3Provider(provider)
  library.pollingInterval = 15000
  // Fix transaction format  error from etherjs getTransactionReceipt as transactionReceipt format
  // checks root to be a 32 bytes hash when on RSK its 0x01
  const format = library?.formatter.formats
  if (format) format.receipt['root'] = format.receipt['logsBloom']
  Object.assign(library?.formatter, { format: format })

  return library
}

@patogallaiovlabs
Copy link
Contributor

After looking at the doc : https://eth.wiki/json-rpc/API (QUANTITY field type)
This is not an issue, we are working as expected.
Are we ready to close this issue? @bguiz
Thanks!

@bguiz
Copy link
Author

bguiz commented Oct 20, 2020

yep @patogallaiovlabs I'll close

BTW detailed description here: ethers-io/ethers.js#952 (comment)

(the bug appears to be that ethers.js does not implement transaction validation correctly, and rskj is spec compliant)

@ricmoo
Copy link

ricmoo commented Feb 1, 2021

Not sure if closed issues are monitored, but RSK incorrectly implements EIP-658.

If the status field is present, the root field should not be (and if it were present, it should be a bytes32, not a single byte nor quantity).

Reading the EIP, I can understand the confusion though.

For blocks where block.number >= BYZANTIUM_FORK_BLKNUM, the intermediate state root is replaced by a status code, 0 indicating failure

To better format this, it should probably have read:

For blocks where block.number >= BYZANTIUM_FORK_BLKNUM, the intermediate state receipt.root is replaced by a receipt.status code...

You can verify this against Geth, Parity, Besu (Hyperledger) and EthereumJS.

For now ethers is adding additional logic in the receipt parser to handle this, but it would also be ideal for upstream dependencies to implement the standard uniformly.

Thanks! :)

@bguiz
Copy link
Author

bguiz commented Feb 17, 2021

Re-opening so as to look into the following:

If the status field is present, the root field should not be

Additionally, in some cases (when a transaction fails) receipt.root is returned as "0x" (i.e. no digits after the x), which could also be related.

@ajlopez
Copy link
Contributor

ajlopez commented Feb 17, 2021

We never used the root field in RSK testnet nor mainnet. It was not A HARD FORK for us. So, I propose to remove the root field from transaction receipt DTO.

My code in https://github.com/ajlopez/rskj/tree/remtxrroot

@aeidelman
Copy link
Collaborator

@bguiz can we close this issue?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
compatibility Ethereum compatibility core ⚛️
Projects
None yet
Development

No branches or pull requests

7 participants