Return r, s, and v without leading 0's

[carver]

These values are quantities, not data. Other nodes return r, s, and v as hex-encoded quantites.

See #1170

[coveralls]

Coverage increased (+0.04%) to 85.736% when pulling ac28015 on carver:r-and-s-quantities into 746e440 on ethereum:1.0.

[frozeman]

I would actually like to get a third opinion on that. As bytes are technically invalid if they are uneven.
@chriseth, @axic and @holiman any opinion?

[frozeman]

The problem with this is what if the rs, value is 0x00001234 Then a even number is made uneven

[frozeman]

And are you sure passing a modified value into RLP.decode is not going to break anything?

[carver]

Do you mean RLP.encode?

It looks like it dynamically calculates the length, so I don't see any problems: https://github.com/MaiaVictor/eth-lib/blob/master/lib/rlp.js#L27

In fact, I believe leaving out 0 bytes at the beginning of an int in the RLP-encoded data is a more standard approach. (not sure if it's required though)

[frozeman]

The tests pass here.
But this would actually be an argument for padding the r,s values at all times with leading 0s

[frozeman]

Additionally allowing the odd numbers again will make this problem reappear: #1258

Though its very important that signatures are correctly validated!

[carver]

But this would actually be an argument for padding the r,s values at all times with leading 0s

Additionally allowing the odd numbers again will make this problem reappear: #1258

Let's separate the two places where r and s values are used:

1. Just before rlp encoding: https://github.com/ethereum/web3.js/pull/1288/files/ac28015f159b8bece403bcac356008cc19cb037c#diff-7bb2a20126193b9ecfe4723f83429c49R168
2. When returning from signTransaction: https://github.com/ethereum/web3.js/pull/1288/files/ac28015f159b8bece403bcac356008cc19cb037c#diff-7bb2a20126193b9ecfe4723f83429c49R177

This PR intends to not change the functionality of # 1 - the change there was only a refactor. That means that the #1258 problem should not reappear, and I agree that r and s values should be provided with an even number of hex characters. (because that's what this rlp library implementation expects)

This PR only changes the functionality of # 2, trimming all leading 0s. This brings it in line with what the nodes themselves return when returning (non-rlp-encoded) values of r and s.

[frozeman]

Ill merge this.
@malissa and @jellegerbrandy can you test the 1.0 branch when its merged?

[frozeman]

@carver this also should fix #1289. Can you confirm?

[carver]

@frozeman No, some more work would have to be done, maybe in web3 or maybe in eth-lib. #1290 is the test that should pass.

Some options I see, highest preference first:

• web3 could zero-pad r and s to 32 bytes before concatenating and handing off the signature to eth-lib
• eth-lib could add a new recovery option that accepts r, s, and v as individual arguments (splitting them is the only thing it has trouble with IIRC)
• (more of a strawman than a good idea:) eth-lib could detect short r and s, maybe by doing some modified RLP decoding

[carver]

On further investigation, it looks like eth-lib itself is responsible for encoding the signature. So the best solution is to fix eth-lib's Account.encodeSignature()

[carver]

Submitted a PR at VictorTaelin/eth-lib#4

That repo has 3 open pull requests that go back to October, and has never merged or closed an incoming pull request. Maybe eth-lib needs to be forked into the ethereum org to be maintained?

Edit: BTW, I confirmed that PR 4 does cause this PR's test to pass.

[frozeman]

I will talk to @MaiaVictor, but we could also leftpad this before sending it to eth-lib

[frozeman]

I merged eth-lib and the tests pass. Ill create a new version soon