Skip to content
This repository has been archived by the owner on Apr 16, 2019. It is now read-only.

support full 32bit chain_id, cast (uint64_t)v #386

Closed
wants to merge 1 commit into from

Conversation

hackmod
Copy link
Contributor

@hackmod hackmod commented Jul 23, 2018

with the following PR trezor/trezor-common#174 (64bit signature_v)
r,s value can be calculated successfully.

I'm not sure why v value is truncated to 32bit. but anyway,
with this fix, r, s values are correctly obtained for full ranged 32bits chain_id.

for some larger chain_id, signature v value is truncated to 32bits, but anyway,
simply recalculate v value at the client side javascript level, signing could be done successfully with full ranged 32bits chain_id.

@hackmod
Copy link
Contributor Author

hackmod commented Jul 23, 2018

image

(trezor testsuite, chainId support added, recalc v - https://github.com/EthersocialNetwork/bip39/tree/trezor-chainid-testsuite)

@prusnak
Copy link
Member

prusnak commented Jul 24, 2018

Sorry, I am against this change. I think that 2 billions chain_ids are enough for everyone and using higher values than 2147483630 might break lots of other software.

@prusnak prusnak closed this Jul 24, 2018
@hackmod
Copy link
Contributor Author

hackmod commented Jul 24, 2018

  • currently, go-ethereum(geth) does not restrict chain_id less then 2147483630 at all. this is defacto standard. Trezor simply break it
  • Pirl (https://pirl.io ) use chainid 3125659152 about 11 month ago after their EIP155 hardfork. Pirl users with a trezor HD wallet can't use their Trezor by this issue.

It is more resonable for Trezor to support full range 32-bit chainId rather than the etheruem EIP155 specific encoding.

so it should be considered as a bug I think.

@prusnak
Copy link
Member

prusnak commented Jul 24, 2018

That's unfortunate, but I am not fixing this.

@hackmod
Copy link
Contributor Author

hackmod commented Jul 25, 2018

just to be clear, current status of official firmware.

  • only chainId < 255 supported. (official firmware)
  • recently, this bug found and fixed.

how about to fix send_signature() like as following.

method 1

diff --git a/firmware/ethereum.c b/firmware/ethereum.c
index a5cf7a3..a3c1689 100644
--- a/firmware/ethereum.c
+++ b/firmware/ethereum.c
@@ -203,7 +203,11 @@ static void send_signature(void)

        msg_tx_request.has_signature_v = true;
        if (chain_id) {
-               msg_tx_request.signature_v = v + 2 * chain_id + 35;
+               if (chain_id > 255) {
+                       msg_tx_request.signature_v = v;
+               } else {
+                       msg_tx_request.signature_v = v + 2 * chain_id + 35;
+               }
        } else {
                msg_tx_request.signature_v = v + 27;
        }
  • chain_id < 256 cases: normal cases - return v(signature bit) + 2 * chain_id + 35
  • chain_id > 255 cases: simply return v(signature bit)
    • no need to additional fix.
    • additional fix needed at the javascript level to obtain 2 * chain_id + 35 + v(signature bit)
  • compatible with lower version firmware behavior

method 2

or simply just return the signature bit v only. like as following

--- a/firmware/ethereum.c
+++ b/firmware/ethereum.c
@@ -203,7 +203,7 @@ static void send_signature(void)

        msg_tx_request.has_signature_v = true;
        if (chain_id) {
-               msg_tx_request.signature_v = v + 2 * chain_id + 35;
+               msg_tx_request.signature_v = v;
        } else {
                msg_tx_request.signature_v = v + 27;
        }
  • additional fix need at js level
  • lower version incompatible in the firmware level

or even shortly

--- a/firmware/ethereum.c
+++ b/firmware/ethereum.c
@@ -203,7 +203,xx @@ static void send_signature(void)

        msg_tx_request.has_signature_v = true;
-        if (chain_id) {
-               msg_tx_request.signature_v = v + 2 * chain_id + 35;
+               msg_tx_request.signature_v = v;
-        } else {
-                msg_tx_request.signature_v = v + 27;
-        }
  • additional fix need at js level
  • lower version incompatible in the firmware level as well
  • but with firmware version info, we can make it work with no problem

@prusnak
Copy link
Member

prusnak commented Jul 25, 2018

Or even shorter - no firmware change, use 1 as chain_id, then increase returned v by (2 * chain_id - 1).

@hackmod
Copy link
Contributor Author

hackmod commented Jul 25, 2018

@prusnak // I guess you miss understood me.
described method 1,2,3 are tested shortly. (not a mere brain coding)

EthersocialNetwork#2 this is my new PR based on the 1st method.
(the 1st method is the lower version firmware compatible one)
(I can't rebase or push some changes onto this closed PR. will post PR later after my summer vacation ended)

@hackmod
Copy link
Contributor Author

hackmod commented Jul 30, 2018

See also Ledger case LedgerHQ/ledgerjs#168

@prusnak
Copy link
Member

prusnak commented Jul 30, 2018

See also Ledger case LedgerHQ/ledgerjs#168

Another reason why not use chain_id wider than 32 bits, so this proves my point

@hackmod
Copy link
Contributor Author

hackmod commented Jul 31, 2018

finally Ledger support full ranged 32bits chain_id.

Ledger even returns only 1-byte (truncated) signature v, but we know, v value contains signature v bit, so we can recalculate v at the client side, by v = chainId * 2 + 35 + signature v bit(0 or 1)
already mentioned at commit LedgerHQ/app-ethereum@8260268

Supports 32 bits chainId for signature and matching - the returned V will be wrong and has to be recomputed by the client

for example, MyEtherWallet/etherwallet#1979

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