New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Ethereum support (GetEthereumAddress and SignEthereumTx) #11

Merged
merged 3 commits into from May 23, 2016

Conversation

Projects
None yet
4 participants
@axic
Contributor

axic commented May 16, 2016

This is purely an RFC to discuss what would be the preferred way to implement Ethereum support.

Quick intro into Ethereum:

0) It uses only uncompressed public keys

1) An address is formatted as:

  • Take the Keccak (pre-NIST SHA3) hash of the x and y points of the uncompressed key (i.e. remove the type field (0x04 for uncompressed) of the SEC1 format)
  • Take the 160 least significant bits
  • Use the hex string representation of the result

Albeit this is the format used internally and by all clients, there exist a backward compatible checksumming for the hex representation. Additionally several other address formats/proposals exist.

2) Ethereum is based on account state and not UTXOs. This results in a very simple transaction format, which includes the recipient, value and additional fields signed by the sender address. Technically it is the following fields, in this order, serialized using RLP:

  • nonce
  • gasPrice
  • gasLimit
  • to
  • value
  • data
  • v
  • r
  • s

Where nonce, gasPrice, gasLimit and value are 256 bit big endian numbers, data is an optional arbitrary length field, to is the recipient address (as a binary and not as a hex string) and v/r/s correspond to the signature fields.

The proposal: address retrieval

Based on how different the address format is, a new method GetEthereumAddress with its return value EthereumAddress is proposed.

I am not sure that this is the best way. Alternatively a field can be introduced in the CoinType to distinguish between Bitcoin-style and Ethereum-style addresses (sample implementation here: keepkey/device-protocol#3) and use GetAddress.

The Ethereum address format is subject to change hence I am bit worried which option to take.

The proposal: transaction signing

Again, as transactions have no similarity to Bitcoin, this proposes an entirely different format.

Request:

message EthereumSignTx {
        repeated uint32 address_n = 1;      // BIP-32 path to derive the key from master node
        required bytes nonce = 2;
        required string to = 3;
        required bytes value = 4;               // 256-bit number (big endian)
        optional bytes data = 5;
        required bytes gasPrice = 6;            // 256-bit number (big endian)
        optional bytes gasLimit = 7;            // 256-bit number (big endian)
}

Response:

message EthereumSignedTx {
    required bytes rlp = 1;
}

Remarks:

  • gasLimit is optional and defaults to 21000 (note: I should add it to the proto)
  • The user must be presented on screen with the recipient, total transaction cost (value + (gasPrice * gasLimit)) and some representation of the data field.
  • The data field behaves like OP_RETURN for transactions between external accounts, but is used as the ABI field (mostly method invocation) when transacting with contract accounts. A future improvement to this protocol could be moving the ABI encoding to the device instead in addition to the data field.

Some questions:

  1. What is the preferred way for address generation?
  2. How to handle non-negative 256 bit big number fields? As bytes or differently?
  3. The nonce must be incremental. One can publish future transactions to the mempool by leaving a gap. Should the firmware take care about nonces?
@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 16, 2016

Member

The proposal: address retrieval

I like your way much better than the other one in the KK pull request - i.e. using special messages for Ethereum.

  1. What is the preferred way for address generation?

Ether has a SLIP-44 code assigned, so I guess m/44'/60'/account'/change/index

  1. How to handle non-negative 256 bit big number fields? As bytes or differently?

Yes, bytes. Also you should use bytes for "to" field as well. String type is reserved for valid UTF-8 sequences.

  1. The nonce must be incremental. One can publish future transactions to the mempool by leaving a gap. Should the firmware take care about nonces?

If one wants to use multiple devices with one account, then it should be handled in the software, not hardware.

Member

prusnak commented May 16, 2016

The proposal: address retrieval

I like your way much better than the other one in the KK pull request - i.e. using special messages for Ethereum.

  1. What is the preferred way for address generation?

Ether has a SLIP-44 code assigned, so I guess m/44'/60'/account'/change/index

  1. How to handle non-negative 256 bit big number fields? As bytes or differently?

Yes, bytes. Also you should use bytes for "to" field as well. String type is reserved for valid UTF-8 sequences.

  1. The nonce must be incremental. One can publish future transactions to the mempool by leaving a gap. Should the firmware take care about nonces?

If one wants to use multiple devices with one account, then it should be handled in the software, not hardware.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 16, 2016

Member

Also don't use required fields, make them all optional. (Protobuf3 does not have required fields and even in v2 using of required is discouraged).

Member

prusnak commented May 16, 2016

Also don't use required fields, make them all optional. (Protobuf3 does not have required fields and even in v2 using of required is discouraged).

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 16, 2016

Member

Another question: Do we really need 256-bit numbers for all these numeric fields? Isn't uint64 enough? (max value = 18,446,744,073,709,551,615)

Member

prusnak commented May 16, 2016

Another question: Do we really need 256-bit numbers for all these numeric fields? Isn't uint64 enough? (max value = 18,446,744,073,709,551,615)

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 16, 2016

Contributor

I like your way better than the other one

I'm not entirely sure. There are pros and cons for both.

Ether has a SLIP-44 code assigned, so I guess m/44'/60'/account'/change/index

I've actually meant which GetAddress option would be preferred. Regarding derivation path there are lengthy discussions in the Ethereum space, but that is kind of the concern of the client library and not the firmware.

Yes, bytes. Also you should use bytes for "to" field as well. String type is reserved for valid UTF-8 sequences.

The reason I've used string because TxOutputType.address does the same. Using a string EthereumSignTx could indicate it supports multiple address formats (simple hex string, checksummed hex string, etc) and considering that it may be useful to only support bytes (the 160 bit Keccak hash) and let the client library handle conversions. Then again, wouldn't that then apply to GetAddress too?

Also don't use required fields, make them all optional.

Should I leave a comment which fields are mandatory or that is of concern of the firmware only?

Do we really need 256-bit numbers for all these fields? Isn't uint64 enough? (max value = 18,446,744,073,709,551,615)

Unfortunately yes. wei is the smallest unit and 1000000000000000000 wei equals to 1 eth. value and gasPrice are denoted in wei.

There is a discussion about limiting gasLimit to 63 bits, but not entirely sure if that will be passed and if when. Currently it is defined as 256 bits.

Contributor

axic commented May 16, 2016

I like your way better than the other one

I'm not entirely sure. There are pros and cons for both.

Ether has a SLIP-44 code assigned, so I guess m/44'/60'/account'/change/index

I've actually meant which GetAddress option would be preferred. Regarding derivation path there are lengthy discussions in the Ethereum space, but that is kind of the concern of the client library and not the firmware.

Yes, bytes. Also you should use bytes for "to" field as well. String type is reserved for valid UTF-8 sequences.

The reason I've used string because TxOutputType.address does the same. Using a string EthereumSignTx could indicate it supports multiple address formats (simple hex string, checksummed hex string, etc) and considering that it may be useful to only support bytes (the 160 bit Keccak hash) and let the client library handle conversions. Then again, wouldn't that then apply to GetAddress too?

Also don't use required fields, make them all optional.

Should I leave a comment which fields are mandatory or that is of concern of the firmware only?

Do we really need 256-bit numbers for all these fields? Isn't uint64 enough? (max value = 18,446,744,073,709,551,615)

Unfortunately yes. wei is the smallest unit and 1000000000000000000 wei equals to 1 eth. value and gasPrice are denoted in wei.

There is a discussion about limiting gasLimit to 63 bits, but not entirely sure if that will be passed and if when. Currently it is defined as 256 bits.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 16, 2016

Member

Thanks for explanation. I think your changes are good to go. Let's keep the required fields as they are. We'll fix them for every message in the future.

Member

prusnak commented May 16, 2016

Thanks for explanation. I think your changes are good to go. Let's keep the required fields as they are. We'll fix them for every message in the future.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 16, 2016

Contributor

Also there's another another question regarding the transaction messages.

Above I've listed the fields of the transaction which are serialised using RLP. The signing process:

  1. Serialise all fields except v/r/s using RLP
  2. Calculate the signature on the Keccak-hash of the serialised output
  3. Split the signature to its v/r/s components
  4. Now serialise all fields (including the signature) using RLP (note: with some effort the previous RLP output could be partially reused)

The protocol could return only the signature fields and let the client rebuild the final RLP:

message EthereumSignedTx {
    required uint v = 1; // this can only have a value of 27 or 28
    required bytes r = 2;
    required bytes s = 3;
}

(Note: v is the recovery parameter + 27 - similarly to Bitcoin)

Pro:

  • this skips an extra serialisation step (less CPU / memory usage and time)

Cons:

  • the client needs to include an RLP library (it is fairly simple, sample implementation in JS)
  • there must be an absolute agreement how the firmware and the client library handles default values for the optional fields

Actually all fields are mandatory for serialisation (and therefore the same could apply to EthereumSignTx), but you can assume the following:

  • nonce (mandatory, but starts from 0)
  • gasPrice (mandatory, 0 is valid too)
  • gasLimit (mandatory, 0 is valid, albeit useless)
  • to (mandatory, can be 0 as it is a valid address)
  • value (mandatory, can be 0)
  • data can be missing and is encoded as a special 0-byte type
Contributor

axic commented May 16, 2016

Also there's another another question regarding the transaction messages.

Above I've listed the fields of the transaction which are serialised using RLP. The signing process:

  1. Serialise all fields except v/r/s using RLP
  2. Calculate the signature on the Keccak-hash of the serialised output
  3. Split the signature to its v/r/s components
  4. Now serialise all fields (including the signature) using RLP (note: with some effort the previous RLP output could be partially reused)

The protocol could return only the signature fields and let the client rebuild the final RLP:

message EthereumSignedTx {
    required uint v = 1; // this can only have a value of 27 or 28
    required bytes r = 2;
    required bytes s = 3;
}

(Note: v is the recovery parameter + 27 - similarly to Bitcoin)

Pro:

  • this skips an extra serialisation step (less CPU / memory usage and time)

Cons:

  • the client needs to include an RLP library (it is fairly simple, sample implementation in JS)
  • there must be an absolute agreement how the firmware and the client library handles default values for the optional fields

Actually all fields are mandatory for serialisation (and therefore the same could apply to EthereumSignTx), but you can assume the following:

  • nonce (mandatory, but starts from 0)
  • gasPrice (mandatory, 0 is valid too)
  • gasLimit (mandatory, 0 is valid, albeit useless)
  • to (mandatory, can be 0 as it is a valid address)
  • value (mandatory, can be 0)
  • data can be missing and is encoded as a special 0-byte type
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

A note about differentiating between Ethereum networks (Homestead - mainnet, Morden - testnet, etc): there is no differentiation in place currently. The generated address doesn't have an indication of this as it is only based on the public key.

Given the real way to influence that is to have a different private key to start with, I think the best solution would be allocating a new SLIP-44 code for the Morden testnet (i.e. Ether testing).

Contributor

axic commented May 17, 2016

A note about differentiating between Ethereum networks (Homestead - mainnet, Morden - testnet, etc): there is no differentiation in place currently. The generated address doesn't have an indication of this as it is only based on the public key.

Given the real way to influence that is to have a different private key to start with, I think the best solution would be allocating a new SLIP-44 code for the Morden testnet (i.e. Ether testing).

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

This is a good summary of the requirements for Ethereum integration; thanks for writing it up, and I'm sorry I didn't see your work before I started on my own.

Regarding GetAddress/GetEthereumAddress, I think you raise a good point, but I'm slightly in favor of keeping the original RPC rather than adding a new one. My reasoning is this:

  1. Generic tools like trezorctl/keepkeyctl don't integrate with cryptocurrency clients, and so can't usefully interpret the field other than to show it to users; adding a key in a new format is unlikely to break anything, and allows them to immediately work for address retrieval with no changes.
  2. Specific tools, like cryptocurrency clients, already need to know what network and coin type they're talking to, and so know from external context how to interpret the address.

If there's a use-case where client software might need to be able to decode all "bitcoin-style" addresses without knowing the coin type ahead of time, then I think the split is warranted, but I'm not aware of one off-hand.

For transaction signing, I'd like to propose a slightly different method, modelled after the existing SignTx/TxRequest/TxAck flow:

/**
 * Request: Ask device to sign Ethereum transaction
 * @next PassphraseRequest
 * @next PinMatrixRequest
 * @next EthereumTxRequest
 * @next Failure
 */
message SignEthereumTx {
    repeated uint32 address_n = 1;      // BIP-32 path to derive the key from master node
    optional uint64 account_nonce = 2;
    optional bytes gas_price = 3;       // 256-bit big-endian
    optional bytes gas_limit = 4;       // 256-bit big-endian
    optional bytes recipient = 5;       // Recipient address
    optional bytes amount = 6;          // 256-bit number of wei to send
    optional uint32 payload_length = 7; // Length of transaction payload

    optional string coin_name = 8 [default='Ethereum']; // coin to use
}

/**
 * Response: Device asks for more data from transaction payload, or returns the signature.
 * If payload_length is set, device awaits that many more bytes of payload.
 * Otherwise, signature contains the computed transaction signature.
 * @prev SignEthereumTx
 * @next EthereumTxAck
 */
message EthereumTxRequest {
    optional uint32 payload_length = 1; // Number of bytes being requested
    optional bytes signature = 2;       // Computed signature
}

/**
 * Request: Transaction payload data.
 * @prev EthereumTxRequest
 * @next EthereumTxRequest
 */
message EthereumTxAck {
    optional bytes payload_chunk = 1;   // Bytes from transaction payload
}

Field naming aside, the main difference is that the device can ask for chunks of payload until it's received the whole thing, at which point it returns a signature. This allows use of a streaming hash, which permits larger transactions than a single RPC does. Since users are likely to want to be able to create contracts using hardware signing, this seems important.

Arachnid commented May 17, 2016

This is a good summary of the requirements for Ethereum integration; thanks for writing it up, and I'm sorry I didn't see your work before I started on my own.

Regarding GetAddress/GetEthereumAddress, I think you raise a good point, but I'm slightly in favor of keeping the original RPC rather than adding a new one. My reasoning is this:

  1. Generic tools like trezorctl/keepkeyctl don't integrate with cryptocurrency clients, and so can't usefully interpret the field other than to show it to users; adding a key in a new format is unlikely to break anything, and allows them to immediately work for address retrieval with no changes.
  2. Specific tools, like cryptocurrency clients, already need to know what network and coin type they're talking to, and so know from external context how to interpret the address.

If there's a use-case where client software might need to be able to decode all "bitcoin-style" addresses without knowing the coin type ahead of time, then I think the split is warranted, but I'm not aware of one off-hand.

For transaction signing, I'd like to propose a slightly different method, modelled after the existing SignTx/TxRequest/TxAck flow:

/**
 * Request: Ask device to sign Ethereum transaction
 * @next PassphraseRequest
 * @next PinMatrixRequest
 * @next EthereumTxRequest
 * @next Failure
 */
message SignEthereumTx {
    repeated uint32 address_n = 1;      // BIP-32 path to derive the key from master node
    optional uint64 account_nonce = 2;
    optional bytes gas_price = 3;       // 256-bit big-endian
    optional bytes gas_limit = 4;       // 256-bit big-endian
    optional bytes recipient = 5;       // Recipient address
    optional bytes amount = 6;          // 256-bit number of wei to send
    optional uint32 payload_length = 7; // Length of transaction payload

    optional string coin_name = 8 [default='Ethereum']; // coin to use
}

/**
 * Response: Device asks for more data from transaction payload, or returns the signature.
 * If payload_length is set, device awaits that many more bytes of payload.
 * Otherwise, signature contains the computed transaction signature.
 * @prev SignEthereumTx
 * @next EthereumTxAck
 */
message EthereumTxRequest {
    optional uint32 payload_length = 1; // Number of bytes being requested
    optional bytes signature = 2;       // Computed signature
}

/**
 * Request: Transaction payload data.
 * @prev EthereumTxRequest
 * @next EthereumTxRequest
 */
message EthereumTxAck {
    optional bytes payload_chunk = 1;   // Bytes from transaction payload
}

Field naming aside, the main difference is that the device can ask for chunks of payload until it's received the whole thing, at which point it returns a signature. This allows use of a streaming hash, which permits larger transactions than a single RPC does. Since users are likely to want to be able to create contracts using hardware signing, this seems important.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

Regarding the format of the return value, the same transaction size / memory concerns apply to returning the whole RLP, but I think it makes sense to return the serialized signature rather than its parts, since the client may not need to care about the individual components of the signature; see for example here. I'm happy to be persuaded otherwise, however. :)

Arachnid commented May 17, 2016

Regarding the format of the return value, the same transaction size / memory concerns apply to returning the whole RLP, but I think it makes sense to return the serialized signature rather than its parts, since the client may not need to care about the individual components of the signature; see for example here. I'm happy to be persuaded otherwise, however. :)

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

Regarding GetAddress/GetEthereumAddress, I think you raise a good point

If the response becomes bytes for Ethereum addresses (see above), that would mean a new optional field in GetAddress. Perhaps GetEthereumAddress makes more sense in that case.

Generic tools like trezorctl/keepkeyctl don't integrate with cryptocurrency clients, and so can't usefully interpret the field other than to show it to users

Good point, I've never used them.

This allows use of a streaming hash, which permits larger transactions than a single RPC does.

I like the idea for streaming the data field. I am not sure about the overhead. In many cases the data field is <= 36 bytes (a single parameter to a method). Would it make sense having a streaming and a non-streaming option in the protocol?

optional uint64 account_nonce = 2;

Note that the starting nonce is not necessarily 0 on every network. On Morden it is 2^20 and it is possible that other (private) deployments would go for a higher starting nonce to avoid replay attacks.

Taking practicality into consideration it makes sense. The trade off is that a protocol change is required if such a network comes into use.

optional string coin_name = 8 [default='Ethereum']; // coin to use

What is the point of this here?

Regarding the format of the return value, the same transaction size / memory concerns apply to returning the whole RLP

Not exactly. If you don't need to return the full RLP, you can stream the following: <input fields> -> RLP encoding -> SHA3. Only the hash is needed for signing.

If you want to return the full RLP, you can either:

a) keep the raw input or
b) keep the RLP and modify it at the end (In RLP the first bytes is the size field for the array following. You would need to memmove the buffer, then change the size in the front and lastly apply the signature fields at the end.)

but I think it makes sense to return the serialized signature rather than its parts

The link you have provided parses the serialised signature into the transaction structure. Which is then RLP serialised.

Contributor

axic commented May 17, 2016

Regarding GetAddress/GetEthereumAddress, I think you raise a good point

If the response becomes bytes for Ethereum addresses (see above), that would mean a new optional field in GetAddress. Perhaps GetEthereumAddress makes more sense in that case.

Generic tools like trezorctl/keepkeyctl don't integrate with cryptocurrency clients, and so can't usefully interpret the field other than to show it to users

Good point, I've never used them.

This allows use of a streaming hash, which permits larger transactions than a single RPC does.

I like the idea for streaming the data field. I am not sure about the overhead. In many cases the data field is <= 36 bytes (a single parameter to a method). Would it make sense having a streaming and a non-streaming option in the protocol?

optional uint64 account_nonce = 2;

Note that the starting nonce is not necessarily 0 on every network. On Morden it is 2^20 and it is possible that other (private) deployments would go for a higher starting nonce to avoid replay attacks.

Taking practicality into consideration it makes sense. The trade off is that a protocol change is required if such a network comes into use.

optional string coin_name = 8 [default='Ethereum']; // coin to use

What is the point of this here?

Regarding the format of the return value, the same transaction size / memory concerns apply to returning the whole RLP

Not exactly. If you don't need to return the full RLP, you can stream the following: <input fields> -> RLP encoding -> SHA3. Only the hash is needed for signing.

If you want to return the full RLP, you can either:

a) keep the raw input or
b) keep the RLP and modify it at the end (In RLP the first bytes is the size field for the array following. You would need to memmove the buffer, then change the size in the front and lastly apply the signature fields at the end.)

but I think it makes sense to return the serialized signature rather than its parts

The link you have provided parses the serialised signature into the transaction structure. Which is then RLP serialised.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

If the response becomes bytes for Ethereum addresses (see above), that would mean a new optional field in GetAddress. Perhaps GetEthereumAddress makes more sense in that case.

I don't think it makes sense to suddenly start transmitting Ethereum addresses in binary when other addresses are human-readable, personally.

I like the idea for streaming the data field. I am not sure about the overhead. In many cases the data field is <= 36 bytes (a single parameter to a method). Would it make sense having a streaming and a non-streaming option in the protocol?

My concern there is that there's no way for the device to communicate to the client what the maximum size is for its data field, without some sort of multi-part request in any case. This multiple request-response sequence is used for regular Bitcoin signing - the 'simple' API isn't implemented in the code - and doesn't seem to impose undue overhead. With a sensible limit on the chunk size (I see 1k used for other string fields), I don't think this imposes undue overhead, as most signing requests will then only require two RPCs.

Note that the starting nonce is not necessarily 0 on every network. On Morden it is 2^20 and it is possible that other (private) deployments would go for a higher starting nonce to avoid replay attacks.

I'm assuming here that nonces will be managed entirely by the client; looking at geth, this seems justified, as it doesn't store nonce values in the keystore at all, but rather relies on fetching the previous nonce from the state.

What is the point of this here?

Just like there are multiple bitcoin-derived networks, there may be multiple Ethereum-derived networks, and it would be useful to distinguish them. In the absence of any protocol differences, though, this isn't actually used at present - it probably makes sense to drop it and add the field if it ever becomes necessary.

Not exactly. If you don't need to return the full RLP, you can stream the following: -> RLP encoding -> SHA3. Only the hash is needed for signing.

If you want to return the full RLP, you can either:

a) keep the raw input or
b) keep the RLP and modify it at the end (In RLP the first bytes is the size field for the array following. You would need to memmove the buffer, then change the size in the front and lastly apply the signature fields at the end.)

Right. Both of these require storing the entire transaction in memory, however. Given that that's not a requirement for signing, it seems worth avoiding.

The link you have provided parses the serialised signature into the transaction structure. Which is then RLP serialised.

Quite right. I should have said that since there's a canonical serialized format for signatures, it may make sense to use that rather than to encode v, r, and s separately. I'm ambivalent, though.

If the response becomes bytes for Ethereum addresses (see above), that would mean a new optional field in GetAddress. Perhaps GetEthereumAddress makes more sense in that case.

I don't think it makes sense to suddenly start transmitting Ethereum addresses in binary when other addresses are human-readable, personally.

I like the idea for streaming the data field. I am not sure about the overhead. In many cases the data field is <= 36 bytes (a single parameter to a method). Would it make sense having a streaming and a non-streaming option in the protocol?

My concern there is that there's no way for the device to communicate to the client what the maximum size is for its data field, without some sort of multi-part request in any case. This multiple request-response sequence is used for regular Bitcoin signing - the 'simple' API isn't implemented in the code - and doesn't seem to impose undue overhead. With a sensible limit on the chunk size (I see 1k used for other string fields), I don't think this imposes undue overhead, as most signing requests will then only require two RPCs.

Note that the starting nonce is not necessarily 0 on every network. On Morden it is 2^20 and it is possible that other (private) deployments would go for a higher starting nonce to avoid replay attacks.

I'm assuming here that nonces will be managed entirely by the client; looking at geth, this seems justified, as it doesn't store nonce values in the keystore at all, but rather relies on fetching the previous nonce from the state.

What is the point of this here?

Just like there are multiple bitcoin-derived networks, there may be multiple Ethereum-derived networks, and it would be useful to distinguish them. In the absence of any protocol differences, though, this isn't actually used at present - it probably makes sense to drop it and add the field if it ever becomes necessary.

Not exactly. If you don't need to return the full RLP, you can stream the following: -> RLP encoding -> SHA3. Only the hash is needed for signing.

If you want to return the full RLP, you can either:

a) keep the raw input or
b) keep the RLP and modify it at the end (In RLP the first bytes is the size field for the array following. You would need to memmove the buffer, then change the size in the front and lastly apply the signature fields at the end.)

Right. Both of these require storing the entire transaction in memory, however. Given that that's not a requirement for signing, it seems worth avoiding.

The link you have provided parses the serialised signature into the transaction structure. Which is then RLP serialised.

Quite right. I should have said that since there's a canonical serialized format for signatures, it may make sense to use that rather than to encode v, r, and s separately. I'm ambivalent, though.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

What is the maximum size for data field? Is there any limit? Other bytes
field are size limited right?

Member

prusnak commented May 17, 2016

What is the maximum size for data field? Is there any limit? Other bytes
field are size limited right?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

@Arachnid I don't have time for commenting everything, but:

I should have said that since there's a canonical serialized format for signatures

It is not the canonical format. The only semi-canonical format is the format of eth_sign, which was changed recently.

I only see two options: either return a full RLP or the signature components. If you return a serialised signature, that needs to be broken up to its components and put into the RLP structure on the client side.

@prusnak:

What is the maximum size for data field?

The only limit is the encoding of the size field in RLP. Practically it is usually around the 6 to 16k mark for contract deployment.

Other bytes field are size limited right?

Yes, they are 256 bit numbers.

Contributor

axic commented May 17, 2016

@Arachnid I don't have time for commenting everything, but:

I should have said that since there's a canonical serialized format for signatures

It is not the canonical format. The only semi-canonical format is the format of eth_sign, which was changed recently.

I only see two options: either return a full RLP or the signature components. If you return a serialised signature, that needs to be broken up to its components and put into the RLP structure on the client side.

@prusnak:

What is the maximum size for data field?

The only limit is the encoding of the size field in RLP. Practically it is usually around the 6 to 16k mark for contract deployment.

Other bytes field are size limited right?

Yes, they are 256 bit numbers.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

On 17/05/16 10:45, Alex Beregszaszi wrote:

The only limit is the encoding of the size field in RLP. Practically it is usually around the 6 to 16k mark for contract deployment.

Then we would need to go for a streaming option. Currently, maximum size
of a single message is somewhere around 7-9 kilobytes.

Best Regards / S pozdravom,

Pavol Rusnak stick@gk2.sk

Member

prusnak commented May 17, 2016

On 17/05/16 10:45, Alex Beregszaszi wrote:

The only limit is the encoding of the size field in RLP. Practically it is usually around the 6 to 16k mark for contract deployment.

Then we would need to go for a streaming option. Currently, maximum size
of a single message is somewhere around 7-9 kilobytes.

Best Regards / S pozdravom,

Pavol Rusnak stick@gk2.sk

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

It is not the canonical format. The only semi-canonical format is the format of eth_sign, which was changed recently.

I only see two options: either return a full RLP or the signature components. If you return a serialised signature, that needs to be broken up to its components and put into the RLP structure on the client side.

Fair enough; I'd vote for the latter, then. Returning the RLP is possible in a streaming fashion - we can return one chunk for each RPC - but mostly means just repeating the data back to the caller, and then callers like Go would have to deserialize the whole thing to extract the signature, in order to work with current implementations. It doesn't seem unreasonable to expect clients to know RLP, so I'm in favor of sending back the components.

It is not the canonical format. The only semi-canonical format is the format of eth_sign, which was changed recently.

I only see two options: either return a full RLP or the signature components. If you return a serialised signature, that needs to be broken up to its components and put into the RLP structure on the client side.

Fair enough; I'd vote for the latter, then. Returning the RLP is possible in a streaming fashion - we can return one chunk for each RPC - but mostly means just repeating the data back to the caller, and then callers like Go would have to deserialize the whole thing to extract the signature, in order to work with current implementations. It doesn't seem unreasonable to expect clients to know RLP, so I'm in favor of sending back the components.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

Regarding the presence of a coin_type field in SignEthereumTx, it turns out this is useful: it allows the device to identify transactions with transaction fees that require user approval, as well as providing access to the unit name for displaying amounts.

Regarding the presence of a coin_type field in SignEthereumTx, it turns out this is useful: it allows the device to identify transactions with transaction fees that require user approval, as well as providing access to the unit name for displaying amounts.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

I just had a discussion with slush and we think it would be beneficial to have separate messages for Ethereum with Ethereum prefix, so:

EthereumGetAddress
EthereumAddress
EthereumSignTx
EthereumTxRequest
EthereumTxAck

This would allow us to fulfil the vision of separate "apps" running on TREZOR and we'll rename the Bitcoin-related messages to the ones with Bitcoin prefix. (Renaming is not a breaking change, because clients only do care about message IDs).

Member

prusnak commented May 17, 2016

I just had a discussion with slush and we think it would be beneficial to have separate messages for Ethereum with Ethereum prefix, so:

EthereumGetAddress
EthereumAddress
EthereumSignTx
EthereumTxRequest
EthereumTxAck

This would allow us to fulfil the vision of separate "apps" running on TREZOR and we'll rename the Bitcoin-related messages to the ones with Bitcoin prefix. (Renaming is not a breaking change, because clients only do care about message IDs).

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

Okay, fair enough. I'll amend my existing work to use those. Any preference for message IDs for the new messages? Any thoughts on how tools like trezorctl should handle 'get_address' - rename it to 'get_bitcoin_address'? add a 'get_ethereum_address' but keep the existing one? Determine which API to call based on the coin name specified?

One thing we haven't discussed is adding a 'NetworkType' field to the coin info struct, which I've done in the PR @axic linked to; exporting this to the client would make that last option a possibility.

Any decision on the binary vs human-readable decision for the GetAddress? Personally I think it would be a bad idea to introduce a new convention when the existing address field is encoded in human-readable form.

Finally, any firm views on the details of the sign/request/ack messages?

Okay, fair enough. I'll amend my existing work to use those. Any preference for message IDs for the new messages? Any thoughts on how tools like trezorctl should handle 'get_address' - rename it to 'get_bitcoin_address'? add a 'get_ethereum_address' but keep the existing one? Determine which API to call based on the coin name specified?

One thing we haven't discussed is adding a 'NetworkType' field to the coin info struct, which I've done in the PR @axic linked to; exporting this to the client would make that last option a possibility.

Any decision on the binary vs human-readable decision for the GetAddress? Personally I think it would be a bad idea to introduce a new convention when the existing address field is encoded in human-readable form.

Finally, any firm views on the details of the sign/request/ack messages?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

Please don't change get_address call for now. Let's add ethereum_get_address option.

Let's use hex-encoded string for Ethereum addresses.

Let's not use NetworkType field. It does not make sense to mix messages which belong to separate "applications".

I don't see any problems with Sign/Request/Ack messages, but I am no Ethereum guy. :-)

Member

prusnak commented May 17, 2016

Please don't change get_address call for now. Let's add ethereum_get_address option.

Let's use hex-encoded string for Ethereum addresses.

Let's not use NetworkType field. It does not make sense to mix messages which belong to separate "applications".

I don't see any problems with Sign/Request/Ack messages, but I am no Ethereum guy. :-)

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

Let's not use NetworkType field. It does not make sense to mix messages which belong to separate "applications".

In that event, how will different Ethereum-derived coins be distinguished? Should we define an "EthereumCoinType" struct, or punt this until there's >1 network to support (besides testnet)?

Let's not use NetworkType field. It does not make sense to mix messages which belong to separate "applications".

In that event, how will different Ethereum-derived coins be distinguished? Should we define an "EthereumCoinType" struct, or punt this until there's >1 network to support (besides testnet)?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

Aha, I did not know there are multiple Ethereum coin types. :-) Are they? But like you write above, we can always add that field later.

Member

prusnak commented May 17, 2016

Aha, I did not know there are multiple Ethereum coin types. :-) Are they? But like you write above, we can always add that field later.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

There aren't yet - there's testnet and mainnet, but they only really need distinguishing by HD path. But I fully expect we'll see Ethereum derivatives in future, just like there's Bitcoin derivatives. I'll leave out the field for now in my own code, and hardcode the limit for transaction fee confirmation, then.

There aren't yet - there's testnet and mainnet, but they only really need distinguishing by HD path. But I fully expect we'll see Ethereum derivatives in future, just like there's Bitcoin derivatives. I'll leave out the field for now in my own code, and hardcode the limit for transaction fee confirmation, then.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

@prusnak:

Then we would need to go for a streaming option. Currently, maximum size
of a single message is somewhere around 7-9 kilobytes.

There are three common use cases:

  1. Value transfer between accounts - data is empty
  2. Simple method invocation on a contract - data is usually between 4 and 164 bytes (it can be a lot more of course)
  3. Contract deployment - today, data is usually between 6 and 16k (it can be a lot more for more complex contracts)

For case 1) and 2) there no need to stream. I am wondering what kind of speed penalty streaming causes? If it is negligible there is no point complicating the protocol.

@Arachnid:

Regarding the presence of a coin_type field in SignEthereumTx, it turns out this is useful: it allows the
device to identify transactions with transaction fees that require user approval,

Can you elaborate? What is this transaction fee about? The transaction fee (and/or total consideration) should be shown to the user in every case.

as well as providing access to the unit name for displaying amounts.

That is a good reason, if/once Ethereum derivates come around.

There aren't yet - there's testnet and mainnet, but they only really need distinguishing by HD path. But I fully expect we'll see Ethereum derivatives in future, just like there's Bitcoin derivatives.

There is an important distinction here. Bitcoin-derived coins have their coin type encoded in the base58 address. This is not the case with Ethereum (or future derivates?).

And the derivation path is not controlled by the coin type as far as I know.

Contributor

axic commented May 17, 2016

@prusnak:

Then we would need to go for a streaming option. Currently, maximum size
of a single message is somewhere around 7-9 kilobytes.

There are three common use cases:

  1. Value transfer between accounts - data is empty
  2. Simple method invocation on a contract - data is usually between 4 and 164 bytes (it can be a lot more of course)
  3. Contract deployment - today, data is usually between 6 and 16k (it can be a lot more for more complex contracts)

For case 1) and 2) there no need to stream. I am wondering what kind of speed penalty streaming causes? If it is negligible there is no point complicating the protocol.

@Arachnid:

Regarding the presence of a coin_type field in SignEthereumTx, it turns out this is useful: it allows the
device to identify transactions with transaction fees that require user approval,

Can you elaborate? What is this transaction fee about? The transaction fee (and/or total consideration) should be shown to the user in every case.

as well as providing access to the unit name for displaying amounts.

That is a good reason, if/once Ethereum derivates come around.

There aren't yet - there's testnet and mainnet, but they only really need distinguishing by HD path. But I fully expect we'll see Ethereum derivatives in future, just like there's Bitcoin derivatives.

There is an important distinction here. Bitcoin-derived coins have their coin type encoded in the base58 address. This is not the case with Ethereum (or future derivates?).

And the derivation path is not controlled by the coin type as far as I know.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

Streaming causes almost no penalty. So I think it's OK to stream also 1) and 2).

Member

prusnak commented May 17, 2016

Streaming causes almost no penalty. So I think it's OK to stream also 1) and 2).

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

More so that case 1) can be solved with setting the length to 0 and effectively skipping the payload query.

Contributor

axic commented May 17, 2016

More so that case 1) can be solved with setting the length to 0 and effectively skipping the payload query.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

Correct. There will be just one answer in EthereumTxRequest (with signature and payload_length==0).

Member

prusnak commented May 17, 2016

Correct. There will be just one answer in EthereumTxRequest (with signature and payload_length==0).

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

Thinking a bit more about address representation I think bytes is the appropriate format:

  • To my eyes, the Trezor protocol tries to match how the Bitcoin protocol works. It includes the Bitcoin address as a string, because it has the network/coin type included and it is useful to have it verified.
  • The transaction RLP expects the address to be a raw 20 byte hash. Making that a string feels a bit inconsistent as on the client side, when rebuilding the RLP, the raw version needs to be used.
  • There are multiple address formats being discussed and I don't expect to support more than one. Having it as a string suggests it supports more/all.
  • The hex representation of the address format has 4 variants:
    • regular hex string without 0x prefix
    • regular hex string with 0x prefix
    • mixed-case hex string without 0x prefix
    • mixed-case hex string with 0x prefix
  • With bytes, the only validation needed is checking the length, everything else is given.
  • Conversion is only needed in one case to hex string: when displaying it during GetAddress and SignTx on the screen.
Contributor

axic commented May 17, 2016

Thinking a bit more about address representation I think bytes is the appropriate format:

  • To my eyes, the Trezor protocol tries to match how the Bitcoin protocol works. It includes the Bitcoin address as a string, because it has the network/coin type included and it is useful to have it verified.
  • The transaction RLP expects the address to be a raw 20 byte hash. Making that a string feels a bit inconsistent as on the client side, when rebuilding the RLP, the raw version needs to be used.
  • There are multiple address formats being discussed and I don't expect to support more than one. Having it as a string suggests it supports more/all.
  • The hex representation of the address format has 4 variants:
    • regular hex string without 0x prefix
    • regular hex string with 0x prefix
    • mixed-case hex string without 0x prefix
    • mixed-case hex string with 0x prefix
  • With bytes, the only validation needed is checking the length, everything else is given.
  • Conversion is only needed in one case to hex string: when displaying it during GetAddress and SignTx on the screen.
@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

I have no strong opinion on this. Both string (but we'd need to agree on exact type of string e.g mixed-case with 0x prefix) and bytes are OK for me.

Member

prusnak commented May 17, 2016

I have no strong opinion on this. Both string (but we'd need to agree on exact type of string e.g mixed-case with 0x prefix) and bytes are OK for me.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

@axic I think we may be talking at cross-purposes. I agree that the address field inside the 'EthereumSignTx' message should definitely be expressed as bytes, as it's a representation of the protocol internals of the signing process.

I'm only saying that the address in 'EthereumAddress' should be a hex encoded string, in keeping with the current 'Address' message's use of base58 for Bitcoin. The message serves a different purpose there, providing an address which the user can provide to others by the usual mechanism; were it sent as bytes it would, 100% of the time, immediately be converted to a hex encoded string.

@axic I think we may be talking at cross-purposes. I agree that the address field inside the 'EthereumSignTx' message should definitely be expressed as bytes, as it's a representation of the protocol internals of the signing process.

I'm only saying that the address in 'EthereumAddress' should be a hex encoded string, in keeping with the current 'Address' message's use of base58 for Bitcoin. The message serves a different purpose there, providing an address which the user can provide to others by the usual mechanism; were it sent as bytes it would, 100% of the time, immediately be converted to a hex encoded string.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

Pushed an updated protocol based on the discussion so far.

I have used the field order and naming as they are defined in the Yellow Paper. Please have a look.

Contributor

axic commented May 17, 2016

Pushed an updated protocol based on the discussion so far.

I have used the field order and naming as they are defined in the Yellow Paper. Please have a look.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 17, 2016

LGTM except my comment about use of bytes for the EthereumAddress response.

It's probably worth clarifying if the various 256 bit bigints have to be exactly 32 bytes long; given common bigint implementations, protocol overhead, and the fact that RLP encoding truncates them anyway, I think it makes the most sense to require they be exactly as many bytes as required, instead - either that, or allow them to be short, and expect the device to truncate them if they've got leading 0 bytes.

Arachnid commented May 17, 2016

LGTM except my comment about use of bytes for the EthereumAddress response.

It's probably worth clarifying if the various 256 bit bigints have to be exactly 32 bytes long; given common bigint implementations, protocol overhead, and the fact that RLP encoding truncates them anyway, I think it makes the most sense to require they be exactly as many bytes as required, instead - either that, or allow them to be short, and expect the device to truncate them if they've got leading 0 bytes.

@axic axic referenced this pull request May 17, 2016

Merged

Support Ethereum pubkeyhash #59

@jhoenicke

This comment has been minimized.

Show comment
Hide comment
@jhoenicke

jhoenicke May 17, 2016

Contributor

I think we will need some new field in GetAddress for segwit to distinguish between P2PKH, P2WPKH and maybe even P2WPKHoverP2SH addresses. Under this light, it may be simpler to add a new enumeration field to GetAddress that can also be used for ethereum specific formats.

I think GetAddress is only needed for the "Show On Trezor" feature; usually the client computes the address from the public node directly.

The signing is so different that it makes sense to use different messages for these though.

Contributor

jhoenicke commented May 17, 2016

I think we will need some new field in GetAddress for segwit to distinguish between P2PKH, P2WPKH and maybe even P2WPKHoverP2SH addresses. Under this light, it may be simpler to add a new enumeration field to GetAddress that can also be used for ethereum specific formats.

I think GetAddress is only needed for the "Show On Trezor" feature; usually the client computes the address from the public node directly.

The signing is so different that it makes sense to use different messages for these though.

Show outdated Hide outdated protob/messages.proto
*/
message EthereumTxRequest {
optional uint32 data_length = 1; // Number of bytes being requested
optional uint32 signature_v = 2; // Computed signature (recovery parameter, limited to 27 or 28)

This comment has been minimized.

@jhoenicke

jhoenicke May 17, 2016

Contributor

Is this the same byte as used by bitcoin message signatures? Shouldn't this be 27-30? Or how do signatures look if r overflows (probability for this is < 2^-128, but still)?

@jhoenicke

jhoenicke May 17, 2016

Contributor

Is this the same byte as used by bitcoin message signatures? Shouldn't this be 27-30? Or how do signatures look if r overflows (probability for this is < 2^-128, but still)?

This comment has been minimized.

@axic

axic May 17, 2016

Contributor

It is limited to 27/28 by the specification (it deems other values invalid). There are other limitations regarding the r/s components too.

@axic

axic May 17, 2016

Contributor

It is limited to 27/28 by the specification (it deems other values invalid). There are other limitations regarding the r/s components too.

This comment has been minimized.

@jhoenicke

jhoenicke May 20, 2016

Contributor

Interesting, so strictly speaking Ethereum would need a slightly different variant of rfc6979 that also rejects k with overflowing r values.

@jhoenicke

jhoenicke May 20, 2016

Contributor

Interesting, so strictly speaking Ethereum would need a slightly different variant of rfc6979 that also rejects k with overflowing r values.

This comment has been minimized.

@axic

axic May 23, 2016

Contributor

For Homestead (the hard fork at block 1150000) there's an additional check for s <= n/2.

@axic

axic May 23, 2016

Contributor

For Homestead (the hard fork at block 1150000) there's an additional check for s <= n/2.

This comment has been minimized.

@jhoenicke

jhoenicke May 23, 2016

Contributor

We already fix the signature to get s <= n/2. There is also a restriction in Bitcoin; otherwise the transaction is non-standard and not relayed by all nodes (if I remember correctly).

@jhoenicke

jhoenicke May 23, 2016

Contributor

We already fix the signature to get s <= n/2. There is also a restriction in Bitcoin; otherwise the transaction is non-standard and not relayed by all nodes (if I remember correctly).

This comment has been minimized.

@axic

axic May 23, 2016

Contributor

It was added to Bitcoin a long while ago IIRC, one still need to disable that check to validate the early blockchain.

Even though we are not at that step yet, where would you include the code to handle the infinite r/s (v = 29/30)?

@axic

axic May 23, 2016

Contributor

It was added to Bitcoin a long while ago IIRC, one still need to disable that check to validate the early blockchain.

Even though we are not at that step yet, where would you include the code to handle the infinite r/s (v = 29/30)?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 17, 2016

Member

I'd rather not mix messages for various "apps" together. Suppose there is some kind of "router" in the future that would need to route the message to the correct app without looking into its content.

And clearly Bitcoin + Bitcoin-related altcoins and Ethereum + alts will be handled by separate apps.

Member

prusnak commented May 17, 2016

I'd rather not mix messages for various "apps" together. Suppose there is some kind of "router" in the future that would need to route the message to the correct app without looking into its content.

And clearly Bitcoin + Bitcoin-related altcoins and Ethereum + alts will be handled by separate apps.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 17, 2016

Contributor

@jhoenicke:

I think GetAddress is only needed for the "Show On Trezor" feature; usually the client computes the address from the public node directly.

Apart from privacy reasons, GetPublicKey is also less useful for Ethereum, because first the key needs to be uncompressed and then transformed into an address. This requires ECC on the client side, which might not be necessary (though useful) when using trezor.

Contributor

axic commented May 17, 2016

@jhoenicke:

I think GetAddress is only needed for the "Show On Trezor" feature; usually the client computes the address from the public node directly.

Apart from privacy reasons, GetPublicKey is also less useful for Ethereum, because first the key needs to be uncompressed and then transformed into an address. This requires ECC on the client side, which might not be necessary (though useful) when using trezor.

@ming08108 ming08108 referenced this pull request May 18, 2016

Closed

Ethereum Support #6

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 18, 2016

On discussion offline with @axic, we've discovered an edge case with this API: Due to the nature of RLP encoding, if the payload is exactly 1 byte long, it's not possible to determine the first bytes of the encoded data without first knowing the value of that byte. As a result, it would be necessary to buffer the entire EthereumSignTx message until the first byte has been received, which would add a significant amount of heap memory allocation.

I'd like to suggest (re-)adding a payload field to EthereumSignTx, and documenting that the first n bytes of the transaction data must be supplied with the initial message (maybe somewhere in the region of 256-1024?).

On discussion offline with @axic, we've discovered an edge case with this API: Due to the nature of RLP encoding, if the payload is exactly 1 byte long, it's not possible to determine the first bytes of the encoded data without first knowing the value of that byte. As a result, it would be necessary to buffer the entire EthereumSignTx message until the first byte has been received, which would add a significant amount of heap memory allocation.

I'd like to suggest (re-)adding a payload field to EthereumSignTx, and documenting that the first n bytes of the transaction data must be supplied with the initial message (maybe somewhere in the region of 256-1024?).

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 18, 2016

Member

Ack. 1024 bytes are fine for initial payload. (should cover cases 1 and
2 from above without any problems).

Member

prusnak commented May 18, 2016

Ack. 1024 bytes are fine for initial payload. (should cover cases 1 and
2 from above without any problems).

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 19, 2016

Member

Can we have this updated so EthereumSignTx contains first chunk of data as discussed per two comments above?

Member

prusnak commented May 19, 2016

Can we have this updated so EthereumSignTx contains first chunk of data as discussed per two comments above?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 20, 2016

Contributor

Yes it can be added. Note: not having it and buffering the contents of the EthereumSignTx in RLP means buffering at most ~150 bytes.

Contributor

axic commented May 20, 2016

Yes it can be added. Note: not having it and buffering the contents of the EthereumSignTx in RLP means buffering at most ~150 bytes.

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid May 20, 2016

That's 150 bytes permanently consumed from the heap, in addition to a ~200 byte SHA3 state that's required either way, and the additional code complexity of retaining it and handling it on the first data callback (or immediately, if the data length is 0). Given the edge-case, it seems simpler to require at least the first few bytes be included with the initial request.

That's 150 bytes permanently consumed from the heap, in addition to a ~200 byte SHA3 state that's required either way, and the additional code complexity of retaining it and handling it on the first data callback (or immediately, if the data length is 0). Given the edge-case, it seems simpler to require at least the first few bytes be included with the initial request.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 23, 2016

Contributor

Updated the protocol and included a description of the actual field limits. (Note that the number fields are not fixed 256 bit wide.)

I think it really depends on the implementation and I don't see it as a permanently wasted space, but not here to argue about that.

I'm actually glad to have the original proposal of having both options in the initial request 😃

Contributor

axic commented May 23, 2016

Updated the protocol and included a description of the actual field limits. (Note that the number fields are not fixed 256 bit wide.)

I think it really depends on the implementation and I don't see it as a permanently wasted space, but not here to argue about that.

I'm actually glad to have the original proposal of having both options in the initial request 😃

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 23, 2016

Contributor

Considering the field limits must be defined in trezor-mcu/firmware/protob/messages.options, I've updated the protocol with the same:

EthereumGetAddress.address_n            max_count:8
EthereumAddress.address                 max_size:20

EthereumSignTx.address_n                max_count:8
EthereumSignTx.nonce                    max_size:32
EthereumSignTx.gas_price                max_size:32
EthereumSignTx.gas_limit                max_size:32
EthereumSignTx.to                       max_size:20
EthereumSignTx.value                    max_size:32
EthereumSignTx.data_initial_chunk       max_size:1024

EthereumTxRequest.signature_r           max_size:32
EthereumTxRequest.signature_s           max_size:32

EthereumTxAck.data_chunk                max_size:1024

In short: all data fields are limited to 1024 bytes and that is the max chunk size for each Tx/Ack roundtrip. Does that sound reasonable based on past Bitcoin experience?

Contributor

axic commented May 23, 2016

Considering the field limits must be defined in trezor-mcu/firmware/protob/messages.options, I've updated the protocol with the same:

EthereumGetAddress.address_n            max_count:8
EthereumAddress.address                 max_size:20

EthereumSignTx.address_n                max_count:8
EthereumSignTx.nonce                    max_size:32
EthereumSignTx.gas_price                max_size:32
EthereumSignTx.gas_limit                max_size:32
EthereumSignTx.to                       max_size:20
EthereumSignTx.value                    max_size:32
EthereumSignTx.data_initial_chunk       max_size:1024

EthereumTxRequest.signature_r           max_size:32
EthereumTxRequest.signature_s           max_size:32

EthereumTxAck.data_chunk                max_size:1024

In short: all data fields are limited to 1024 bytes and that is the max chunk size for each Tx/Ack roundtrip. Does that sound reasonable based on past Bitcoin experience?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic May 23, 2016

Contributor

Uploaded some basic code to https://github.com/axic/trezor-mcu/tree/ethereum and added @Arachnid with write access. It isn't tested at all.

I'm more interested in comments about the layout - is this the preferred one?

Contributor

axic commented May 23, 2016

Uploaded some basic code to https://github.com/axic/trezor-mcu/tree/ethereum and added @Arachnid with write access. It isn't tested at all.

I'm more interested in comments about the layout - is this the preferred one?

@prusnak prusnak merged commit 0b5c08a into trezor:master May 23, 2016

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak May 23, 2016

Member

Looks good! Let's get this in.

Member

prusnak commented May 23, 2016

Looks good! Let's get this in.

@axic axic deleted the axic:ethereum-protocol branch Aug 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment