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

ethereum: support full range 32bit chain_id#399

Merged
prusnak merged 1 commit intotrezor:masterfrom
hackmod:32bit_chainid_compatfix
Aug 14, 2018
Merged

ethereum: support full range 32bit chain_id#399
prusnak merged 1 commit intotrezor:masterfrom
hackmod:32bit_chainid_compatfix

Conversation

@hackmod
Copy link
Copy Markdown
Contributor

@hackmod hackmod commented Aug 14, 2018

  • remove chain_id restriction to support full 32bit chain_id.
  • for chain_id > MAX_CHAIN_ID(2147483630) case, simply return v signature parity.

with this fix Trezor can support full 32bit chain_id.

  • no backward compatible issue.
  • for chain_id > MAX_CHAIN_ID case, returned v is 0 or 1 and it can be easily detectable.
  • for chain_id > MAX_CHAIN_ID case, one can easily recalculate signature_v at client side.
chain_id signature_v firmware v
no EIP155 case 27+ v (>= 27) 1.6.2 1, 0
1 <= chain_id <= MAX_CHAIN_ID 2 * chain_id + 35 + v (>= 37) 1.6.2[*] [**] 1, 0
chain_id > MAX_CHAIN_ID v ( <= 1) detectable 1, 0

[*]: currently, official 1.6.2 firmware only support 0 < chain_id < 256 correctly.
[**]: trezor:master repo support 0 < chain_id <= MAX_CHAIN_ID.

 * remove chain_id restriction to support full 32bit chain_id
 * for chain_id > MAX_CHAIN_ID(2147483630) case, simply return signature_v
@prusnak prusnak closed this Aug 14, 2018
@mkrufky
Copy link
Copy Markdown
Contributor

mkrufky commented Aug 14, 2018

@prusnak please reconsider, or offer an alternative. This small, two line change is non-invasive, gets the job done in a sane way without any adverse effects.

Changing chain ID is not an option. Ledger is supporting Pirl's full 32bit chain ID already. We could tell our users to buy only Ledger, as Trezor won't work for us, but we'd much rather have proper support and offer our users their own choice in hardware wallets.

Please help us to make that possible.

@prusnak
Copy link
Copy Markdown
Member

prusnak commented Aug 14, 2018

@mkrufky This two line change gets the job done for Trezor One, but not for Trezor Model T and other hardware wallets out there.

If you want proper support, you'll pick a different chain_id. You fail to explain why this is not an option.

There are over 2 billion possible options and yet you somehow managed to pick a number outside of this range. Tomorrow there will be a chain that picked an id that uses full 256-bits and will use your chain as an example why we should support their chain.

If you insist on this very bad choice, you'll be stumbling upon problems with every possible implementation out there - all hardware and all software wallets.

@mkrufky
Copy link
Copy Markdown
Contributor

mkrufky commented Aug 14, 2018

I don't know why exactly Pirl has chosen this specific chain id, but that choice was made long ago. Pirl is not a new network, and it is not trivial to change chain id without causing significant breakage. If we come up with a similar patch for the model T, would that help move along Trezer support? I understand your concern with this large chain id, but (to my knowledge) this is the only issue Pirl has encountered so far that couldn't be remedied by sharing a patch. We'd really like to work with Trezor as well.

@prusnak
Copy link
Copy Markdown
Member

prusnak commented Aug 14, 2018

If someone creates a pull request adding new test cases to the unit test here: https://github.com/trezor/python-trezor/blob/master/trezorlib/tests/device_tests/test_msg_ethereum_signtx_eip155.py

and fix the same issue here: https://github.com/trezor/trezor-core/blob/master/src/apps/ethereum/sign_tx.py#L141-L142 using

if msg.chain_id > MAX_CHAIN_ID:
    req.signature_v -= 27

I am willing to merge these changes.

@prusnak prusnak reopened this Aug 14, 2018
hackmod added a commit to hackmod/trezor-core that referenced this pull request Aug 14, 2018
 * remove chain_id restriction to support full 32bit chain_id.
 * for chain_id > MAX_CHAIN_ID(2147483630) case, simply return v signature parity.
 * see also trezor/trezor-mcu#399
@prusnak prusnak merged commit d114665 into trezor:master Aug 14, 2018
@prusnak
Copy link
Copy Markdown
Member

prusnak commented Aug 14, 2018

Thanks! Please send PR to python-trezor with extended checks to https://github.com/trezor/python-trezor/blob/master/trezorlib/tests/device_tests/test_msg_ethereum_signtx_eip155.py

prusnak pushed a commit to trezor/trezor-core that referenced this pull request Aug 14, 2018
 * remove chain_id restriction to support full 32bit chain_id.
 * for chain_id > MAX_CHAIN_ID(2147483630) case, simply return v signature parity.
 * see also trezor/trezor-mcu#399
This was referenced Aug 14, 2018
hackmod added a commit to hackmod/python-trezor that referenced this pull request Aug 14, 2018
@hackmod
Copy link
Copy Markdown
Contributor Author

hackmod commented Aug 14, 2018

@prusnak // all done!

and thank you again!!

Trezor is a great project!
I've never seen it before like as the Trezor :) (some HW design, circuit are opensourced.)
personally, I just wanted to contribute your great project and thanks again! 😄

@prusnak
Copy link
Copy Markdown
Member

prusnak commented Aug 15, 2018

Thanks, merged all in!

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.

3 participants