-
Notifications
You must be signed in to change notification settings - Fork 20k
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
tests: update for cancun #27721
tests: update for cancun #27721
Conversation
Looks to me like geth does it right, placing a |
The provided test are calculated on nimbus lets investigate. This errors should not happen. |
This is the state I get for "state": {
"root": "ef7ccce1de9cba62c324f81c6d3c29c2c33bd82c5e02ad2fe4aaa627a70f3033",
"accounts": {
"0x095e7baea6a6c7c4c2dfeb977efac326af552d87": {
"balance": "1000000000000100000",
"nonce": 0,
"root": "0x4289ec81d647020d33f2cf78a124c5dcf163b473188a125d4df6335eaa6e9515",
"codeHash": "0xb66aacbafdc7cb3e298496dd55427edfe775d6f703066bb071e2d2fb2cbfeed2",
"code": "0x600049600055600a4960015500",
"storage": {
"0x0000000000000000000000000000000000000000000000000000000000000000": "01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8"
},
"key": "0x0fbc62ba90dec43ec1d6016f9dd39dc324e967f2a3459a78281d1f4b2ba962a6"
},
"0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba": {
"balance": "56444",
"nonce": 0,
"root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
"codeHash": "0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470",
"key": "0x9d860e7bb7e6b09b87ab7406933ef2980c19d7d0192d8939cf6dc6908a03305f"
},
"0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b": {
"balance": "999999999999646002",
"nonce": 1,
"root": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
"codeHash": "0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470",
"key": "0x03601462093b5945d1676df093446790fd31b20e7b12a2e8e5e09d068109616b"
}
}
I think it should, but I think it's lacking blobhash-parsing into transactions. (EDIT: I'm not sure. Maybe it works already) |
Re the other failures, where an error is expected. for _, tx := range block.Transactions() {
// Count the number of blobs to validate against the header's dataGasUsed
blobs += len(tx.BlobHashes())
// Validate the data blobs individually too
if tx.Type() == types.BlobTxType {
if len(tx.BlobHashes()) == 0 {
return errors.New("no-blob blob transaction present in block body")
}
for _, hash := range tx.BlobHashes() {
if hash[0] != params.BlobTxHashVersion {
return fmt.Errorf("blob hash version mismatch (have %d, supported %d)", hash[0], params.BlobTxHashVersion)
}
}
}
} This ^ is in the block validator, but seems to me that maybe we ought to do it somewhere else too, e.g wherever we to transaction validity checks in general @MariusVanDerWijden ? |
here is nimbus post state on that test
|
We don't have
It looks like nimbus is giving 0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba an extra 0x80000/524288 ether balance. |
So |
@MariusVanDerWijden please check f2f7d20#diff-cbeb8282854297a2c8ef02293455cb0258ae75b1d2a1a398b2eb60e59a591ba5R92 and see if you agree |
yes the nimbus is likely to be incorrect here, we need a vmtrace from nimbus.
also it is a question about option for t8n to allow generation of invalid cases like in here (if possible) |
These tests are using a basefee of |
I think its okay. We need to duplicate that check in the txpool anyway to make sure we don't accept any transactions with the wrong version byte in the txpool. |
Well, what the txpool does nor does not do doesn't matter for the core, since the txpool has no bearing on consensus. For the consensus-checks, I think it's better to have the validation done in one place and one place only. I'm not sure what "I think its okay." meant, if you meant we could remove the ones from block validator or if you meant that it was ok to have it double. |
Ah, geth is missing some gas-account that currently is implemented in e.g. #27511, hence geth doesn't calculate the transaction gas usage for blob txs correctly. |
With the latest updates, geth agrees with besu :
|
I think we can merge this now, if we mark the failing tests as failing. |
I've also added |
Tests do have data gas used |
That was a minimum allowed basefee as far as I remember Yes ori did a test and it goes to 7 according to the formula. But there was no strict rule to reject baseffe below 7, just that it will never go lower than 7 |
This updates the reference tests to the latest version and also adds logic to process EIP-4844 blob transactions into the state transition. We are now passing most Cancun fork tests. Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Felix Lange <[email protected]>
This PR pulls the latest tests, and also the latest "eip tests" -- tests not for general consumption yet, but which are slated for a future fork. It also contains some necessary changes to statetests, state processing and errors, to handle fields related to 4844.
It passes the
MCOPY
andTSTORE/TLOAD
tests, but fails a couple ofBLOBHASH
tests. These tests seem to be erroneously generated, as the geth stateroot matches besu.