fixed rlp_length for chain_id > 255

[hackmod]

See also trezor/connect#137

[prusnak]

Can you provide test vectors for various values of chain_id? Your fix is wrong because it fixes the values of chain_id for <65536 but again fails for values higher than this. It even may fail for values >=0x80. Having the test vectors would find these bugs, that's why I encourage you to come up with some.

[hackmod]

Your fix is wrong because it fixes the values of chain_id for <65536 but again fails for values higher than this

Yes. you are right. It is a quick hack and it fix only for chainid < 65536 cases.

It even may fail for values >=0x80

nop. it works fine with chainId == 31102 (ESN) and for some cases like as chainid 256,820(callisto) blahblah

as i already mentioned for some cases, it is very tedious job.

• step 1 - obtain r,s,v for some chainId using modified connect/example/signtx.html
• step 2 - make some test.js using r,s,v

const ethTx = require('ethereumjs-tx');

const txParams =
{
// nonce: '0x04', // Replace by nonce for your account on geth nodedd
 nonce:"0x04",
 gasPrice:"0x0861c46800",
 gasLimit:"0x5208",
 to:"0x1B7eBfbF517F41d1C2Cc078511Af429d3798ee3A", // the same as from address
 value:"0x2386f26fc10000",
 data:"0x",
 chainId:31102, // FIXME
 v:"0xf320", // FIXME
 r:"0xb4efca01e2604461423305b11974553b52f0397209a6996e1520f0c8e854b20c", // FIXME
 s:"0x7ee5105e05811b2fd0c8947021c7f259738bcf02fe73d6477543b4dc9095345d" // FIXME
};

// Transaction is created
const tx = new ethTx(txParams);
const serializedTx = tx.serialize();
const rawTx = '0x' + serializedTx.toString('hex');
var ret = tx.verifySignature();
console.log('verifySignature() = ', ret);
console.log('SenderAddress = ' + tx.getSenderAddress().toString('hex'));

if (txParams.to.toLowerCase().slice(2) == tx.getSenderAddress().toString('hex')) {
  console.log("SUCCESS!");
} else {
  console.log("FAIL");
}

//console.log(rawTx)
console.log("eth.sendRawTransaction(\"" + rawTx + "\")");

[prusnak]

I know that's a tedious work, but it needs to be done. I suggest to create the test vectors for the following values of chain_ids: 0, 1, 255, 256, 65535, 65536, 16777215, 16777216 and 2147483630 (== MAX_CHAIN_ID).

With test vector I mean a pair of corresponding inputs and outputs - request message EthereumSignTx and response message EthereumTxRequest correctly filled in by provided and expected parameters.

Using these we can create a similar test to this one, but which will check all possible chain_id values: https://github.com/trezor/python-trezor/blob/2a5ca12924d33062db2e8c0163f49568083379ea/trezorlib/tests/device_tests/test_msg_ethereum_signtx.py#L97-L117

[hackmod]

OK.
I've just made a modified signtx.html script (before you reply it) to make a test.js for some selected chainId.
https://gist.github.com/hackmod/9da4963eeef8f7652691f847819b5c58

I will make some modified bip39 tool + above script to check all possible chainId cases more easily as soon as possible.

Using these we can create a similar test to this one, but which will check all possible chain_id values: https://github.com/trezor/python-trezor/blob/2a5ca12924d33062db2e8c0163f49568083379ea/trezorlib/tests/device_tests/test_msg_ethereum_signtx.py#L97-L117

thank you! will check it later.

[ghost]

@hackmod 이런 의미의 테스트 스크립트가 아니라 빌드 테스트 내 테스트 스크립트를 말하는것 같은데요;;

[hackmod]

this is a quick hack using bip39 tool to test trezor with chainId 0,1,255,256,65535, 65536, 16777215, 16777216,2147483630
https://github.com/EthersocialNetwork/bip39/tree/trezor-chainid-testsuite

screenshot

and result.

sooner or later, i will check trezor/python-trezor :)

[prusnak]

I guess you are mixing chain_id and BIP44 coin_type in your tests. It is not needed to change the coin_type in BIP32 path in order to perform sign tests with various chain_id-s.

[hackmod]

I guess you are mixing chain_id and BIP44 coin_type in your tests.
It is not needed to change the coin_type in BIP32 path in order to perform sign tests with various chain_id-s.

yes. you are right, but anyway this testsuit is quick and dirty but handy to invest chainid bug right now.

[hackmod]

updated to support chainid <= MAX_CHAIN_ID (2147483630)

[prusnak]

@axic @Arachnid Can you comment on this issue? The fix seems rather peculiar.

[axic]

Without having read the entire thread, shouldn't the length calculation for chain_id work like how rlp_encode_number works (serialising to big endian and finding the first non-zero byte to sense the length)?

[hackmod]

const assert = require('assert');
const RLP = require('rlp');

var ii = [0, 1, 127, 128, 255, 256, 65535, 65536, 16777215, 16777216,2147483630, 2147483631];

var kk = [];
for (var m = 1; m < 16777216; m++) {
  kk.push(m);
}
// for (var m = 1; m < 100000; m++) {
//  kk.push(parseInt(Math.random() * 2147483630 + 1));
// }

// ii.forEach(function(j) {
kk.forEach(function(j) {
  var x = RLP.encode(j);
  //console.log(j, x);
  l = j > 16777215 ? 4: j > 65535 ? 3: j > 255 ? 2: j > 127 ? 1 : 0;
  k = j > 0xffffff ? 4: j > 0xffff ? 3: j > 0xff ? 2: j > 0x7f ? 1 : 0;
  m = j < 0x80 ? 0: j < 0x100 ? 1: j < 0x10000 ? 2: j < 0x1000000 ? 3 : 4;
  //console.log('l=', l, 'k=', k, 'm=', m, x.length);
  if (l != k || k != m) {
    console.log('XXXXXXXX');
  }
});

[hackmod]

it looks ugly or weird but working and simple enough one liner hunk to fit proof-of-concept fix

[jhoenicke]

remove nested if, remove case for < 0x80.

[jhoenicke]

This is already in an if (chain_id). I think this nested if isn't necessary or even hurting.

[jhoenicke]

length should be 1 even for chain_id < 0x80. rlp_calculate_length will take care of the special case.

    length = chain_id < 0x100 ? 1 : chain_id < 0x10000 ? 2 : chain_id < 0x1000000 ? 3 : 4;

[jhoenicke]

Theoretically the second parameter should be the first byte of chain_id, i.e. chain_id >> (8*(length-1)). However practically, the first byte is only used if length is 1, so this works.

[hackmod]

yes. right :)