search

hyperledger-web3j / web3j

Lightweight Java and Android library for integration with Ethereum clients
https://www.web3labs.com/web3j-sdk
Other
5.11k stars 1.69k forks source link

Add support for non-byte chain id values #234

Closed conor10 closed 4 years ago

conor10 commented 7 years ago

In the current web3j implementation, the ChainId is of type byte.

The reason for this is historical,to support EIP-155, where you place a byte value in the raw transaction object - see here.

However, further down the original EIP discussion it is mentioned that the v value when the transaction is RLP encoded shouldn't be limited to a single byte. So web3j could be enhanced to accommodate this.

This would mean enhancing the Transaction encoder so that a long value can be provided:

https://github.com/web3j/web3j/blob/master/crypto/src/main/java/org/web3j/crypto/TransactionEncoder.java#L34

The SignatureData object should not however be modified as this requires a byte value for v in transaction signing. This means that in the initial encode of the RawTransaction the chain id field should be RLP encoded, which would require modification of the current encode method.

The ChainId itself should probably become an enum wrapping the underlying long value.

fcorneli commented 6 years ago

@conor10 Any news on this issue? We would want to use a geth dev network with chainId 1337...

fcorneli commented 6 years ago

I've been looking into this one a bit. See PR https://github.com/web3j/web3j/pull/478 ChainId of type byte is really all over the place. Easiest to move forward here I guess is to first have a TransactionDecoder and proper signature validation within RawTransaction and work your way up from there.

fcorneli commented 6 years ago

Started working on that TransactionDecoder: https://github.com/e-Contract/web3j/tree/transaction-decoder

fcorneli commented 6 years ago

Seems like Sign.signedMessageToKey has a problem when the message has been signed with an explicit chain id.

fcorneli commented 6 years ago

TransactionDecoder ready: https://github.com/web3j/web3j/pull/491 Funny thing: change chainId within TransactionDecoderTest.testDecodingSignedChainId to something else than 1, and the test explodes. But anyway, it's already a starting point I guess...

fcorneli commented 6 years ago

You can change chainId within TransactionDecoderTest.testDecodingSignedChainId up to 46 without any problem. Above 46, the v requires more than one byte I guess, so it fails from there on.

fcorneli commented 6 years ago

Next step I guess is to allow for byte array v within Sign.SignatureData: https://github.com/e-Contract/web3j/tree/signaturedata-v-array

adridadou commented 6 years ago

any news on that? it's limiting if we want to use it for non public chains ...

d10r commented 5 years ago

Looks like this was fixed in https://github.com/web3j/web3j/pull/913, included in release 4.3.
Unfortunately the last android build is 4.2 though. (update: nomore the case, there's no android builds including this fix)