ethereum/go-ethereum

Inconsistency in Transaction r,s,v between code and doc

elv-gilles opened this issue ยท 9 comments

The Transaction struct uses txdata to perform unmarchalling with fields r,s,v defined as *big.Int therefore making them all handled as quantities (see: Hex Value encoding). In particular leading zeroes are not allowed.

At the same time the API documentation (see getTransactionByHash) pretends V is a quantity and R, S are data.

txdata code:

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"`
     ...
}

doc:

v: QUANTITY - ECDSA recovery id
r: DATA, 32 Bytes - ECDSA signature r
s: DATA, 32 Bytes - ECDSA signature s

This makes some tools (e.g. Ganache) producing incorrect values.
Here an example (see the 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"
}

We've hit this bug a few times while troubleshooting issues in trufflesuite/ganache-core. I'm a bit troubled that it's been open for so long.

If geth is going to go against Postel's Law/the robustness principle and strictly enforce the DATA vs QUANTITY distinction on inputs, I'd think that it should be enforcing the correct types. Otherwise at best you're forcing a lot of logic that is predicated on the node vendor into RPC client libraries, and at worse you're breaking RPC client library compatibility with either geth, or other nodes.

Please fix this and adhere to ethereum's RPC spec. It is currently impossible to test code using go-ethereum against a ganache, which does adhere to the spec. Or change the spec and make r and s quantity types, which could be argued if you view them as ECC integer quantities in a finite field.

fjl commented

Just stumbled into this cluster of issues :). My 2c: geth and parity return R, S values as QUANTITY because they are treated as number by the consensus encoding of transactions. Making them DATA in RPC is weird. I understand that this causes frustration for people though.

Since ganache seems to be the outlier w.r.t. to encoding them on the server side, I'll send a PR to them to correct the issue there.

From what I understand, what keeps the Ganache team from switching to quantities isn't a missing PR. They just want the spec to be aligned with the geth&parity implementations. Then they will probably fix it immediately. So following your argument, the RPC spec needs to be fixed for r and s to be QUANTITYs :) They are currently DATAs.

I forgot, the commit for such a PR already exists: icherkashin/ganache-core@cb376ee
See also this discussion in ganache-core.
At the end of that thread there's a link to a version of Ganache that we released with this commit applied.

fjl commented

Reproducer: https://gist.github.com/fjl/69454fe6b56d04b3c4932d8673dedc63
Ganache PR: trufflesuite/ganache#432

I've tried this against a couple clients and they all return QUANTITY encoding. I'll bring up the spec change in the next AllCoreDevs call to make sure we're not tripping up anyone else, but it should
really be fixed in the spec instead.

Let's change the spec, we just discussed it on ACD, nobody objected.

@fjl please update the spec

fjl commented

I have updated the spec.