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

Change transaction signature value encoding to QUANTITY #432

Closed
wants to merge 1 commit into from
Closed

Change transaction signature value encoding to QUANTITY #432

wants to merge 1 commit into from

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Jun 11, 2019

Even though the Ethereum JSON-RPC spec says that transaction signature
values R and S are DATA, all clients encode them as QUANTITY. The
signature values are defined as numbers in the yellow paper, suggesting
QUANTITY as the correct encoding. This change ensures ganache is compatible
with Geth, Parity and any future spec change.

I found this issue through ethereum/go-ethereum#19461 on
the go-ethereum tracker. See https://gist.github.com/fjl/69454fe6b56d04b3c4932d8673dedc63
for a comprehensive reproducer script.

Even though the Ethereum JSON-RPC spec says that transaction signature
values R and S are DATA, but all clients encode them as QUANTITY. The
signature values are defined as numbers in the yellow paper, suggesting
QUANTITY as the correct encoding.

This change ensures ganache is compatible with Geth, Parity and any
future spec change.
@fjl
Copy link
Contributor Author

fjl commented Jun 11, 2019

There is no downside to merging this change, I think. Web3.js and others will likely accept any encoding for these values.

@davidmurdoch
Copy link
Member

Thanks for doing the leg work on this!

I understand this issue is a bit frustrating, but to be clear, this is NOT a bug in ganache-core, as we are compliant with the spec, so I'd really prefer that you didn't label it as such in github issues and comments going forward as it may paint us in a bad light in the eyes of developers that don't understand the context of the issue. :-)

I had brought this up in the all core devs call a few weeks ago and it ended up turning into a conversation about maintaining and further developing the JSON-RPC specification rather than just changing this one return type.

That said, I realize I'm being a bit stubborn about changing this, but I feel that if I don't stand my ground, at least a little bit, the spec will likely go unchanged and become even further incomplete. I'd love to be able to take on spec maintenance myself but I don't currently have the bandwidth to do so.

@fjl
Copy link
Contributor Author

fjl commented Jun 11, 2019

I agree the spec should change, and I will change it. Just want to bring it up in the call first.
You're right about ganache being correct and everyone else being wrong. It's not a bug in ganache, that was just my first reaction to the go-ethereum issue when I found out about this problem :). Sorry :).

@davidmurdoch
Copy link
Member

@fjl Are you aware of any progress or updates on this issue on either the geth or spec side?

@davidmurdoch
Copy link
Member

@fjl Any updates on getting the spec updated? I know on the last core devs call it was agreed that it should be changed. I'd do it my self but I don't have edit privs for https://github.com/ethereum/wiki/wiki/JSON-RPC#returns-28 :-)

@fjl
Copy link
Contributor Author

fjl commented Jul 15, 2019

I don't have edit permissions either. Will acquire them and update the spec today.

@fjl
Copy link
Contributor Author

fjl commented Jul 17, 2019

Changed! https://github.com/ethereum/wiki/wiki/JSON-RPC/_compare/c6bc8bf4657f8bf0bb839e2b4a69e518dc559c4d...f4e624855ae05371b3fb78e02f5052679063b0b2

@davidmurdoch
Copy link
Member

Closing in favor of #461. Thanks @fjl!

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

Successfully merging this pull request may close these issues.

2 participants