Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Compatibility: cannot unmarshal hex number with leading zero digits into Go struct #166

Closed
gislik opened this issue Sep 4, 2018 · 7 comments

Comments

@gislik
Copy link

gislik commented Sep 4, 2018

The ethclient fails parsing a transaction received from ganache over RPC.

Expected Behavior

It should parse the JSON response.

Current Behavior

json: cannot unmarshal hex number with leading zero digits into Go struct field txdata.nonce of type hexutil.Uint64

Possible Solution

ethclient expects the following transaction fields to hex encodings of a number without any leading zeros:

  • nonce
  • gasPrice
  • gasLimit
  • value
  • input
  • v
  • r
  • s

Therefore a nonce of 0x02 will fail to parse but 0x2 will succeed.

Steps to Reproduce (for bugs)

  1. Start a ganache on port 9545 (ganache-cli -p 9545).
  2. Connect using web3 (npx truffle console).
  3. web3.eth.sendTransaction({from: web3.eth.accounts[0], to: web3.eth.accounts[1], value: 0}) (nonce 0 will be encoded correctly as 0x0).
  4. web3.eth.sendTransaction({from: web3.eth.accounts[0], to: web3.eth.accounts[1], value: 0}) (nonce 1 will be encoded incorrectly as 0x01).
  5. Use the attached go program to lookup the tx hex returned from step 4.
  6. Program crashes.

Context

According to the JSON-RPC specification hex quantity values should be encoded as follows:

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)

ganache should adhere to the spec.

package main

import (
  "context"
  "log"
  "os"

  "github.com/ethereum/go-ethereum/common"
  "github.com/ethereum/go-ethereum/ethclient"
)

func main() {
  if len(os.Args) != 2 {
    log.Fatal("Usage: ./transaction <tx-hex>")
  }
  h := os.Args[1]
  conn, err := ethclient.Dial("http://localhost:9545")

  if err != nil {
    log.Fatalf("Failed to connect to the Ethereum client: %v", err)
  }

  t, _, err := conn.TransactionByHash(context.Background(), common.HexToHash(h))
  if err != nil {
    log.Fatal(err)
  }
  log.Printf("%#v\n", t)
}

Your Environment

  • Version used: All versions, including develop branch.
  • Operating System and version: Linux e 4.15.12-x86_64-linode105 SMP Thu Mar 22 02:13:40 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
gislik added a commit to gislik/ganache-core that referenced this issue Sep 4, 2018
gislik added a commit to gislik/ganache-core that referenced this issue Sep 4, 2018
* fix-leadingzeros:
  Fixing leading zeros for hex encoded quantities in tx trufflesuite#166
  Complete removal
  log reason string on transaction failure
@kaidiren
Copy link

kaidiren commented Oct 23, 2018

+1

ganache version 1.2.2 mac os
geth version 1.8.17

json: cannot unmarshal hex number with leading zero digits into Go value of type *hexutil.Big

@elv-gilles
Copy link

It seems that the documentation and the go client code do not agree (i have open 18152 for that).

From the code of (Transaction) V,R and S are all quantities:

type txdata struct {
      ...
	// Signature values
	V *big.Int `json:"v" gencodec:"required"`
	R *big.Int `json:"r" gencodec:"required"`
	S *big.Int `json:"s" gencodec:"required"`
     ...
}

This makes the current ganache-core (master) code fail since it might produce the following (see value of s):

{
    "hash": "0xaf4ac0efa94f00c4e1befb1bccf5a1449c94893f7f8154f3f1741aa1d98211ad",
    "nonce": "0x32",
    "blockHash": "0xc8f7702839f45d62ee51933115ed13e297aef7c10971943651ec9b2c62bf7218",
    "blockNumber": "0x33",
    "transactionIndex": "0x0",
    "from": "0x4aec2d14c2f3337d122d457c5b0bde8cf9a2c1b8",
    "to": "0x7a9c80e1b27c7bb3a59495eb48fa4dc5b17ed5d7",
    "value": "0x0",
    "gas": "0x7a1200",
    "gasPrice": "0x77359400",
    "input": "0xc287e0ed",
    "v": "0x1b",
    "r": "0x8ea36310f0ba52a8c3b368b6dd7e9795d32e2882243d38bbdbecce5fa6fe086b",
    "s": "0x0731356d897511278c6acdb0f23d768481543988359cc539165f6d224f1a30ae"
}

I was able to improve the situation by changing in ganache-core/lib/utils/txhelper.js function toJSON to use to.rpcQuantityHexString instead of to.hex :

  toJSON: function(tx, block) {
   ...
    if (tx.v && tx.v.length > 0 && tx.r && tx.r.length > 0 && tx.s && tx.s.length > 0) {
      resultJSON.v = to.rpcQuantityHexString(tx.v);
      resultJSON.r = to.rpcQuantityHexString(tx.r);
      resultJSON.s = to.rpcQuantityHexString(tx.s);
    }

    return resultJSON;
  },

@eshaben eshaben self-assigned this Dec 3, 2018
@eshaben
Copy link
Contributor

eshaben commented Dec 5, 2018

Thanks @gislik for submitting this issue! We have fixed the incorrect encoding of the nonce field in the transaction object. I am going to go ahead and close this issue since I have verified that the nonce field is now being correctly encoded in ganache-core version 2.3.1!

@elv-gilles, thank you for taking the time to comment here and for also submitting the issue with go-ethereum. It looks like there are definitely some inconsistencies. I can see your concerns, but since the JSON-RPC spec states that r and s are DATA fields and v is a QUANTITY field, we will make the recommended change to the v field as that should be a QUANTITY datatype. go-ethereum will need to make the required changes to the r and s fields to stay consistent with the spec.

@eshaben eshaben closed this as completed Dec 5, 2018
icherkashin added a commit to icherkashin/ganache-core that referenced this issue Apr 18, 2019
…ite#166)

Problem: Go-ethereum client cannot unmarshall json it receives from Ganache's
response to 'eth_getBlockByHash' request.

Best Known Cause: Go-etherum client interprets R and S as quantities, not data
( as evidenced by big.Int type )
(https://github.com/ethereum/go-ethereum/blob/master/core/types/transaction.go#L54)

Temporary solution: Encode R and S as RPC quantity, not data.

Justification: At the moment, it is easier to customize Ganache than to propose
changes to Go-etherum client. Moreover, it appears strange for the Ethereum
JSON RPC Specification to treat R and S as data, not quantities: after all, R
and S and integer-like objects in the context of the elliptic curve
cryptography, i.e. they admit operations such as addition, subtraction,
modulus, etc.

Proposal for long-term solution: From remarks above, it appears most natural to
change the Ethereum JSON RPC Specification to treat R and S as quantities and
change Ganache accordingly.
@sebastianst
Copy link

As go-ethereum ignores fixing the bad on their end, couldn't we, in the meantime, at least optionally enable ganache to serialize the r and s fields as QUANTITYs? I know that, in principal, ganache is following the spec here and geth is not, but interoperability with go-ethereum is also quite important. It is not uncommon for applications to implement workarounds to the wrong behavior of software with which interoperability is important.
I could think of, e.g., an environment variable like GANACHE_SERIALIZE_SIG_AS_QUANTITY=1 to enable this workaround. If there's an interest in that, I'd put together a PR to ganache-core that would extend cb376ee to use such an environment variable.
Or is there a more natural place to configure such a thing?

sebastianst added a commit to perun-network/ganache that referenced this issue May 6, 2019
@davidmurdoch
Copy link
Member

@sebastianst, It's very frustrating! We're toying with the idea of adding compatibility modes for nodes in Ganache, allowing for parity, geth, etc compatibility. To do so we'll need to change the architecture of ganache to make something like this maintainable for the long term (this refactoring is currently underway).

For now, I'll look into PRing the a fix directly into go-ethereum and then assemble a personal army of my fellow Trufflers, ConsenSys folks, and /r/ethdev redditors to attempt to garner attention from the repo maintainers. I'll also see if I can bring the issue up in the All Core devs call this Friday morning (added it to the agenda here: ethereum/pm#97 (comment)).

I've got some other high priority work I'm currently working on so I can't give you an ETA for the PR. However, if you'd like to PR the fix to geth (or the spec) yourself I can still attempt to put the personal army together to try to bring attention to the issue.

@sebastianst
Copy link

A compatibility mode would be a very nice a clean solution! And thanks for bringing up the issue in the next dev call! I think it makes sense to wait what comes out of the call regarding whether to change the spec or geth's implementation before working on it.

In the meantime, Perun will release workaround versions of Ganache: https://github.com/perun-network/ganache/releases/tag/v2.0.2-beta.0-perun-geth-sigfix

@fjl
Copy link
Contributor

fjl commented Jun 11, 2019

Xref: #432

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

No branches or pull requests

8 participants