Closed holiman closed 1 year ago
Older version of besu, at commit 51956b7f3 does not have this error:
$ /home/martin/workspace/besu-vm --json state-test /tmp/00000936-mixed-1.json 2>&1 | grep COINBASE
{"pc":80,"op":65,"gas":"0x79bc24","gasCost":"0x2","memory":"0x","memSize":0,"stack":["0x0","0x1","0x1","0x2","0x2","0xffff","0x1f4","0x78859e5b97166c486532b1595a673e9f9073643f1b519c6f18511b9913","0x2","0x389","0x0","0x0","0x1","0x0","0x3e3d6d5ff042148d326c1898713a76759ca273","0x44852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d"],"returnData":"0x","depth":1,"refund":0,"opName":"COINBASE","error":""}
It's the test env, and I argue that Besu's interpretation, while differing from the past is correct, although I accept in the interim it may need to be rolled back for test conformance.
The test env in the reference test is as follows (reformatted for clarity)...
"env": {
"currentCoinbase": "b94f5374fce5edbc8e2a8697c15331677e6ebf0b",
"currentDifficulty": "0x20000",
"currentRandom": "0x0000000000000000000000000000000000000000000000000000000000020000",
"currentGasLimit": "0x26e1f476fe1e22",
"currentNumber": "0x1",
"currentTimestamp": "0x3e8",
"previousHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"currentBaseFee": "0x10"
},
The previous hash is Zero, the current block is 1, ergo the hash of block zero is zero, per the env setup. However, the reference test envs expect blockhash to be the the keccak of the uint256 representation of the block number in question.
If the "parentHash" field is removed, the test performs as expected.
Oh, wow, yeah the previousHash
, I hadn't thought of that. Good point. It also means I can work around this by supplying a previousHash of 0x44852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d
. Or, as you say, drop it.
So... bug or no bug? I have a patch ready to roll.
It is a bug, in my opinion (but my fuzzing can work around it so I don't personally care if you fix it or not)
The scope of this is strictly the state-test
subcommand of the evmtool
. All state tests run by the state test harness worked and all production networks use a different block-hash rule than the fuzzing frameworks.
Thinking some more about this: I would not say that besu is 'wrong' or has a 'bug'. The instructions are unclear: what is even the point of previousHash
if we just ignore it?
But apparently all other clients behave differently, and I guess that would be the only reason to change the behaviour. As I said, for my fuzzing, it doesn't matter, I just want to avoid tripping over false positives.
Feel free to close this regardless of whether you merge #5126 or not
A failing state-test. Here's the last line of agreement, and the differing line:
It's a
BLOCKHASH
with stack being[... , "0x0"]
. On the next step, geth has the top stack value0x44852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d
, besu has0x0
.We are executing on block
1
:This means that block
0
is eligible. The hash0x4485...
iscrypto.Keccak256([]byte{'0'})
(in gethcommon.BytesToHash(crypto.Keccak256([]byte(big.NewInt(int64(n)).String())))
). For any valid block number (valid as in within the range of allowed lookups), the state tests use that formula. (Forevm t8n
, the rules are stricter: it should error out if a blockhash op is invoked and the necessary blockhashes are not supplied by the caller).Nethermind:
Geth:
Nimbus-eth1:
Besu:
testcase: