cmd/evm: handle rlp errors in t9n

[holiman]

This PR closes #23770, by checking the iterator error when handling transactions.
The input to the tool is expected to be valid rlp, containing binary blobs of potentially invalid transactions. As opposed to invalid rlp. If invalid RLP is input, then the tool should exit with error code 11.

[winsvega]

still have:

tx.rlp content: 0xf855f851228001825208870b9331677e6ebf0a801ca098ff921201554726367d2be8c804a7ff89ccf285ebc57dff8ae4c44b9c19ac4aa03887321be575c8095f789dd4c743dfe42c1820f9231f98a962b210e3ac2452a3

Error: The command '/home/wins/.retesteth/default/start.sh --input.txs /dev/shm/20faf2e6-64ba-4116-8269-c078c0a96a25/tx.rlp --state.fork Frontier' exited with 2816 code.

ah, I see. thats because it is invalid RLP
that is actually what we want to test - a rejection of invalid RLP

I don't catch error exit codes from retesteth. any error exit code treated as a failure.

the invalid rlp handling needed only for t9n.

[winsvega]

hmm still failing with valid rlp but malicious field in structure:

rlp: non-canonical integer (leading zero bytes) for *big.Int, decoding into (types.Transactions)[0](types.LegacyTx).GasPrice
Error: The command '/home/wins/.retesteth/default/start.sh --input.alloc /dev/shm/d7bff45f-1cab-4f94-ae80-731c1478e120/alloc.json --input.txs /dev/shm/d7bff45f-1cab-4f94-ae80-731c1478e120/txs.rlp --input.env /dev/shm/d7bff45f-1cab-4f94-ae80-731c1478e120/env.json --state.fork Berlin --output.basedir /dev/shm/d7bff45f-1cab-4f94-ae80-731c1478e120 --output.result out.json --output.alloc outAlloc.json' exited with 256 code.

I put 00 prefix to transaction gasPrice when it is expected to be integer (no leading zeros)

[holiman]

hmm still failing with valid rlp but malicious field in structure:

Could you please provide sample rlp?

[holiman]

I put 00 prefix to transaction gasPrice when it is expected to be integer (no leading zeros)

I tested this now, here's what I get:

go run . t9n --input.txs ./testdata/18/invalid2.rlp --state.fork London
[
  {
    "error": "rlp: non-canonical integer (leading zero bytes) for uint64, decoding into (types.Transaction)(types.LegacyTx).Gas"
  }
]

It's as expected, no? One transaction, we can't derive sender / hash, because it fails to even marshal it as a transaction.
And if I place two of them in a row, I get two distinct errors:

[
  {
    "error": "rlp: non-canonical integer (leading zero bytes) for uint64, decoding into (types.Transaction)(types.LegacyTx).Gas"
  },
  {
    "error": "rlp: non-canonical integer (leading zero bytes) for uint64, decoding into (types.Transaction)(types.LegacyTx).Gas"
  }
]

rlp: 0xf8ecf8748094d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d08300520894d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d011801ba0c16787a8e25e941d67691954642876c08f00996163ae7dfadbbfd6cd436f549da06180e5626cae31590f40641fe8f63734316c4bfeb4cdfab6714198c1044d2e28f8748094d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d08300520894d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d011801ba0c16787a8e25e941d67691954642876c08f00996163ae7dfadbbfd6cd436f549da06180e5626cae31590f40641fe8f63734316c4bfeb4cdfab6714198c1044d2e28

[holiman]

I don't catch error exit codes from retesteth. any error exit code treated as a failure.

It is a failure. t9n is a tool to validate transactions, not block bodies. You are supposed to feed it valid rlp.
If you want to feed invalid rlp, and see how the engine behaves, then you need to use something else.

[winsvega]

rlp that crashes t8ntool

0xf867f8658082001083061a8094095e7baea6a6c7c4c2dfeb977efac326af552d87830186a0801ca0a0a90fee6f93f365431b335fa02c7a9716acf7f6c87dbb58921a16ab44e4df20a06a42fa777ab057cbfd856f3fe15e3c44ae00afb3f977b1241b5e67979ee045e1

t9n was doing it fine all the way.

[holiman]

Ah, right, so it's t8n you're talking about, not t9n. Got it

[holiman]

$ go run . t8n --input.txs=./testdata/18/invalid2.rlp --state.fork=London --input.env=./testdata/14/env.json
rlp: non-canonical integer (leading zero bytes) for *big.Int, decoding into (types.Transactions)[0](types.LegacyTx).GasPrice

So -- what would you expect t8n to do in this case?
You're telling it to "Apply these transactions to the given prestate". But one of the items in that list is not a valid transaction. Would you want t8n to just reject that one and continue with the others?

[winsvega]

Translate valid rlp (but invalid structure coded in rlp) trandactions into error message like you do with transactions that fail (oog/evm exception)

Will it check geth core logic since its in t8n?

[holiman]

Will it check geth core logic since its in t8n?

Well, that's kind of how it is now. Geth internally iterates over a slice of transactions, meaning that in order to even reach the point where it starts applying transactions, the payload must be parse:able into a slice of transactions. Which means that errors such as rlp: non-canonical integer (leading zero bytes)... will abort the procedure. It's simply an invalid block body, none of (other) transactions are ever validated/applied.

So if you want me to change that, then that change will make t8n behave less like "core" geth

[winsvega]

No. On contrary I want the logic to be the same as if it was a real block coming with such rlp.

Just need to catch an exception and report it somehow so the t8n doesn't crash.

An invalid tr we would have one at a time.

[holiman]

Just need to catch an exception and report it somehow so the t8n doesn't crash.

Well, t8n doesn't crash. It exits with with error, but that's not a "crash".

[winsvega]

Can this exit be supressed and instead write an error message in outalloc.json result section?

[winsvega]

Like, right now it returns:

ERROR(11): rlp: value size exceeds available input length
Error: The command '/home/marioevz/.retesteth/default/start.sh --input.txs /dev/shm/edbbfaba-6269-4682-a204-2a15b2e4f639/tx.rlp --state.fork Frontier' exited with 2816 code. (ttWrongRLP/RLPExtraRandomByteAtTheEnd, step: TransactionTestFiller)

Want:

Request: test_rawTransaction '0xf8608...80', Fork: `London
T9N Response:
[
  {
    "error": "rlp: value size exceeds available input length"
  }
]

[holiman]

The t8n output is very well defined and structured. See https://github.com/ethereum/go-ethereum/tree/master/cmd/evm#basic-usage .
You want it to all of a sudden output

[
  {
    "error": "rlp: value size exceeds available input length"
  }
]

?
Why not just handle the exit codes? I could make a special exit code for this particular case, if you want.

[winsvega]

No, I am talking about t9n output. It is the same format of output t9n use to report other errors.

In t8n case it would be good to place an error here in outAlloc

 "rejected": [
  {
   "index": 1,
   "error": "rlp: value size exceeds available input length"
  }

[holiman]

In t8n case it would be good to place an error here in outAlloc

Right, ok. Now I understand what you ask. And that's doable, yes, however,

Geth internally iterates over a slice of transactions, meaning that in order to even reach the point where it starts applying transactions, the payload must be parse:able into a slice of transactions. Which means that errors such as rlp: non-canonical integer (leading zero bytes)... will abort the procedure. It's simply an invalid block body, none of (other) transactions are ever validated/applied.

So if you want me to change that, then that change will make t8n behave less like "core" geth

I want the logic to be the same as if it was a real block coming with such rlp.

TLDR; you're both asking that we change it to not validate the transactions as if they were a block body (parse all of them at the same time, and reject them all if there's an error) - but instead parse/apply them one at a time, _and that we stick to what geth does internally. Those are two conflicting requirements.

[winsvega]

No. Don't change the core logic.

As I understand you construct a block from the input data and feed to evm then it raises the exception.

I just need the exception. If block has a malicious transaction I understand that it is invalid. No need to check transactions befor calling the evm. (This is for t8n)

So if possible to catch that abort of procedure in t8n, just return in outAlloc the same pre state and exception message in rejected.

[holiman]

So if possible to catch that abort of procedure in t8n, just return in outAlloc the same pre state and exception message in rejected.

I don't know, it feels like a hack. The rejected part currently maps 1:1 with the input transactions. If you pass 4 transactions, the rejected will contain the error for each tx that was rejected.

You're suggesting to simply handle invalid rlp as if it were an error on tx 1.

I'd rather either make t8n do like t9n, or leave it as it is.

[winsvega]

And having prefixed zeroes in transaction field is the same level of error as having unreadable rlp? Meaning t8n can't produce an error message per transaction if one transaction has leading zeroes kind of rlp error?

Another option perhaps I can catch error exit code, but that one doesn't tell me the exception string. I am not sure how I can get error log when running multiple instances of t8n simultaneously, I should have a look. Perhaps t8n could export stderr to logfile?

[winsvega]

Let's go one issue at a time.

Can you make t9n transaction tool catch and report invalid rlp exceptions the same way it reports prefixed 00 fields?

[winsvega]

Like, right now it returns:

ERROR(11): rlp: value size exceeds available input length
Error: The command '/home/marioevz/.retesteth/default/start.sh --input.txs /dev/shm/edbbfaba-6269-4682-a204-2a15b2e4f639/tx.rlp --state.fork Frontier' exited with 2816 code. (ttWrongRLP/RLPExtraRandomByteAtTheEnd, step: TransactionTestFiller)

Want:

Request: test_rawTransaction '0xf8608...80', Fork: `London
T9N Response:
[
  {
    "error": "rlp: value size exceeds available input length"
  }
]

Like this

[holiman]

Can you make t9n transaction tool catch and report invalid rlp exceptions the same way it reports prefixed 00 fields?

No. As I've said: the input to t9n must be an rlp list of things, where things are not necessarily transactions. If the tool cannot read it as an rlp list, then it's bad input and it exits with the corresponding error code.

It is a failure. t9n is a tool to validate transactions, not block bodies. You are supposed to feed it valid rlp.
If you want to feed invalid rlp, and see how the engine behaves, then you need to use something else.

We can change that, so e.g it can output some sort of json toplevel error, but it will not look the same way, structurally, as if one of the txs has bad rlp encoding.

[winsvega]

Ok then perhaps this would be a solution. Since I see the error message I need in error log.

If t9n/t8n return with error code of invalid rlp input. Using the option --errorlog /path/to/stderrfile
I can read the error log searching for a specific exception.

Can you make such an option?