feat(core,legacy): add support for Ethereum 64-bit chain_id

[arbitrarylink]

Adds support for Ethereum 64-bit chain_id This also makes sure that chain_id of 0 is rejected on core the same way it is rejected in legacy.

Test-Results.txt

[arbitrarygbg]

This supersedes #1741

[prusnak]

I think we could use this opportunity to make MAX_CHAIN_ID do what the name suggests. That is, we should throw an error when provided value in protobuf exceeds the maximum and simplify this:

trezor-firmware/core/src/apps/ethereum/sign_tx.py

Lines 153 to 156 in a398704

	if msg.chain_id > MAX_CHAIN_ID:
	req.signature_v -= 27
	elif msg.chain_id:
	req.signature_v += 2 * msg.chain_id + 8

into this:

if msg.chain_id: 
  req.signature_v += 2 * msg.chain_id + 8 

and this:

trezor-firmware/legacy/firmware/ethereum.c

Lines 211 to 217 in f93a851

	if (chain_id > MAX_CHAIN_ID) {
	msg_tx_request.signature_v = v;
	} else if (chain_id) {
	msg_tx_request.signature_v = v + 2 * chain_id + 35;
	} else {
	msg_tx_request.signature_v = v + 27;
	}

into this:

msg_tx_request.signature_v = v + 27;
if (chain_id) {
  msg_tx_request.signature_v = v + 2 * chain_id + 8; 
}

What do you think @matejcik?

[matejcik]

My thoughts were in the opposite direction: always require chain_id, and then just send a single bit for signature_v and let the host compute the correct value. (both Connect and trezorlib already do this).

I'm not sure if there are any Ethereum networks out there not using EIP-155, but we certainly don't support any, seeing as our Ethereum network database is keyed on chain_id.

At that point, perhaps we could go the extra mile and introduce chain_id_bytes field in case someone decides that 64 bits are not enough. (Ethereum seems to internally use 256-bit numbers, and all other numeric values in the message are bytes already.)

[prusnak]

My thoughts were in the opposite direction: always require chain_id

I let you decide on this. Either seems fine to me. I just want to decrease the number of source lines in our codebases.

At that point, perhaps we could go the extra mile and introduce chain_id_bytes field in case someone decides that 64 bits are not enough.

I prefer not to do this, because this would break all existing implementations. OTOH uint32 and uint64 look exactly the same on the wire.

[arbitrarylink]

Would it make sense for me to just remove the check_chain_id function, revert the tests to no longer set chain_id=1, add the two tests for MAX_CHAIN_ID and MAX_CHAIN_ID+1 and then leave all of the other suggestions for another future PR?

[matejcik]

@arbitrarylink yes, that makes sense. Please do that.

[arbitrarylink]

@arbitrarylink yes, that makes sense. Please do that.

The updates have been made.

[matejcik]

thanks for your work. I'm merging this to a separate Ethereum branch for now, I want to tweak it a little and make it a part of a larger change set (see also #1781)

[0xMarto]

@matejcik quick question, are you planning to merge ethereum-typing into master for the next firmware release?

[matejcik]

yes

[matejcik]

@arbitrarylink could you share more details about chain_id==0 ?
Reading the EIP-155 spec, it seems that the value is technically allowed, so I'm inclined to unify the behavior, instead of disallowing zero in core, allowing it in legacy.

Do you know of any reason why that would be a bad idea?

[arbitrarylink]

@arbitrarylink could you share more details about chain_id==0 ?
Reading the EIP-155 spec, it seems that the value is technically allowed, so I'm inclined to unify the behavior, instead of disallowing zero in core, allowing it in legacy.

Do you know of any reason why that would be a bad idea?

Currently Trezor One (Legacy) disallows chain_id of 0. It returns an error of "Error: DataError: ChainId out of bounds". This comes from the code ./legacy/firmware/ethereum.c: 444.

The code in this PR I had earlier was to disallow chain_id 0 in Trezor T (core) was to make the core behave like the legacy. This code has been reverted as requested.

If you want both devices to support a chain_id of 0 you will have to modify the Legacy code.

[matejcik]

i'm well aware.

I'm asking if you know something about one or the other behavior being "more correct" in some sense.

[arbitrarylink]

There is an EIP that is not yet accepted that will explicitly make chain_id of 0 invalid. https://eips.ethereum.org/EIPS/eip-3788

[matejcik]

Thanks for the info!

This means that currently transactions with chain_id 0 are valid and actually can be used.