Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

treat RLP 'r' and 's' fields as quantity, not string #1258

Merged
merged 3 commits into from
Jan 11, 2018

Conversation

jellegerbrandy
Copy link
Contributor

This fixes #1170 by using the patch provided by @ldn0x7dc in that issue

@coveralls
Copy link

coveralls commented Dec 27, 2017

Coverage Status

Coverage decreased (-0.005%) to 85.703% when pulling 70c0198 on Paratii-Video:1170-RLP-encoding into 6816ce5 on ethereum:1.0.

@carver
Copy link
Contributor

carver commented Dec 28, 2017

This change in output seems like the right thing to do, but it doesn't completely fix #1170. r and s fields should have single leading 0s trimmed as well (when returned individually in https://github.com/ethereum/web3.js/pull/1258/files#diff-7bb2a20126193b9ecfe4723f83429c49R168 ).

@jellegerbrandy
Copy link
Contributor Author

You are right of course; should be better now

@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage increased (+0.03%) to 85.741% when pulling 3f5ea28 on Paratii-Video:1170-RLP-encoding into 6816ce5 on ethereum:1.0.

@malissa
Copy link

malissa commented Jan 9, 2018

Would love to get this merged in!

malissa added a commit to malissa/web3.js that referenced this pull request Jan 9, 2018
@jellegerbrandy
Copy link
Contributor Author

Applying the patch in our code, and running it against parity, returns the following error (from Parity):

Invalid params: Invalid bytes format. Expected a 0x-prefixed hex string with even length.

Apparently, the issue is more complex than I had hoped. This issue seems related as well: ethereumjs/ethereumjs-tx#51

In our code, we have gone back to the "strip two leading zeros" code, and that seems to work fine against parity.

@malissa
Copy link

malissa commented Jan 10, 2018

@jellegerbrandy I also received an error (using geth) when stripping zeros resulted in an odd numbered string. I will try removing two leading zeros - should at least improve the number of times I receive this error which is A LOT.

@jellegerbrandy
Copy link
Contributor Author

I reverted the commit stripping all leading zeros, which clearly was mistaken. Apparently, geth and parity expect strings with an even number of characters that do not start with 0x00... the documentation does not help me much here, unfortunately..

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.005%) to 85.703% when pulling 992a87a on Paratii-Video:1170-RLP-encoding into 6816ce5 on ethereum:1.0.

@carver
Copy link
Contributor

carver commented Jan 10, 2018

RLP encoding is one byte at the smallest unit. Represented as a hex string, that's two hex characters at a time. So for the rlp encoding here, it should only trim null bytes (double 0 hex characters): https://github.com/ethereum/web3.js/pull/1258/files#diff-7bb2a20126193b9ecfe4723f83429c49R162

But for the r and s values, which are not RLP-encoded when returned individually, they should trim down all leading hex 0 characters. https://github.com/ethereum/web3.js/pull/1258/files#diff-7bb2a20126193b9ecfe4723f83429c49R168

@frozeman
Copy link
Contributor

Thanks for the fix! And sorry for the delay, i was not available over christmas.

@frozeman frozeman merged commit d252933 into web3:1.0 Jan 11, 2018
@malissa
Copy link

malissa commented Jan 11, 2018

Woot! Thanks for this!

@frozeman
Copy link
Contributor

It would be good to add a few test cases, does somebody have a tx and key which results in the 0x00.. r, s values?

@malissa
Copy link

malissa commented Jan 11, 2018

Of course now I am unable to get an error. I will comment here if I come across one.

@carver
Copy link
Contributor

carver commented Jan 11, 2018

I just posted short r & s values in #1170

@frozeman
Copy link
Contributor

@malissa you could use a previous commit :)

nachomazzara pushed a commit to nachomazzara/web3.js that referenced this pull request Jun 4, 2020
* treat RLP 'r' and 's' fields as quantity, not string

* strip _all_ leading zeros in rlp encoding

* Revert "strip _all_ leading zeros in rlp encoding"

This reverts commit 3f5ea28.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eth.accounts.signTransaction: expected r & s to be treated as quantity, not data
5 participants