Conversation
29bcee8 to
728dd98
Compare
728dd98 to
6ecc876
Compare
| // using 4.6 TGas | ||
| bt.skipLoad(`.*randomStatetest94.json.*`) | ||
|
|
||
| // TODO(cey): We cannot run invalid blocks, since we don't currently have pre-insert checks. |
There was a problem hiding this comment.
Most of this file is a copy-pasta from libevm. Here we specifically skip some tests that's not currently supported.
There was a problem hiding this comment.
Most of this file is a copy-pasta from libevm
Why? The EEST documentation states that they are...
test fixtures (JSON) which can be consumed by any execution client to verify their implementation
...meaning that they're not even language-specific, let alone related to an implementation. I very strongly believe that we should be using these as inputs in an SAE-specific manner instead of trying to shoe-horn SAE into geth.
There was a problem hiding this comment.
These were not skipped due to a forced integration between geth and sae, but rather we do not have any means of invalidating a block insertion (due to lack of validations in SAE). Maybe we can now revisit this as we have implemented a few of them, but I rather to have that in a separate PR.
There was a problem hiding this comment.
from PR description:
2- We cannot run InvalidBlocks test because they're asserting either that block insertion is failing (which we don't have any verification yet), or they fail at execution (which we don't allow). We can revisit them once we add the verification.
|
|
||
| // TestExecutionSpecBlocktests runs the test fixtures from execution-spec-tests. | ||
| func TestExecutionSpecBlocktests(t *testing.T) { | ||
| if !common.FileExist(executionSpecBlockchainTestDir) { |
There was a problem hiding this comment.
this is actually an empty file, so we don't run this test.
There was a problem hiding this comment.
Then why is it included in this PR?
There was a problem hiding this comment.
this is an outdated comment I forgot to remove. We now download those spec fixtures in CI (fixtures_develop.tar.gz)
This reverts commit 9144414.
| return validBlocks, nil | ||
| } | ||
|
|
||
| func insertWithHeaderBaseFee(tb testing.TB, sut *SUT, bs types.Blocks) { |
There was a problem hiding this comment.
this is the hack to fake the gas clock in parent by setting excess to calculated header.BaseFee
There was a problem hiding this comment.
This is a useful primitive so please abstract blockstest.WithFakeBaseFee(testing.TB, *blocks.Block, [other necessary args]) *blocks.Block.
blocks/blockstest/blocks.go
Outdated
| gen := conf.genesisSpec | ||
| if gen == nil { | ||
| gen = &core.Genesis{ | ||
| Config: config, | ||
| Timestamp: conf.timestamp, | ||
| Alloc: alloc, | ||
| } |
There was a problem hiding this comment.
The default value should instead be set on conf and options.ApplyTo() might then override it. Placing defaults in the original allows options to use them; even if not something that's being done here, it's best practice.
| EXECUTION_SPEC_TESTS_FILE: fixtures_develop.tar.gz | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: |
There was a problem hiding this comment.
Please put all of these in a new job, perhaps go_spec_tests, and add it to the needs of go (to make CI require it). The rationale is that the other tests are more likely to be the ones failing and we want to give that feedback as quickly as possible—placing these in a new job makes it run in parallel.
blocks/blockstest/chain.go
Outdated
| } | ||
|
|
||
| // Insert adds a block to the chain. | ||
| func (cb *ChainBuilder) Insert(block *blocks.Block) { |
There was a problem hiding this comment.
NewChainBuilder() (for genesis) and ChainBuilder.NewBlock() should use Insert() so all paths to insert a block are equivalent.
blocks/blockstest/chain.go
Outdated
| func (cb *ChainBuilder) Insert(block *blocks.Block) { | ||
| cb.chain = append(cb.chain, block) | ||
| cb.blocksByHash.Store(block.Hash(), block) | ||
| } |
There was a problem hiding this comment.
| func (cb *ChainBuilder) Insert(block *blocks.Block) { | |
| cb.chain = append(cb.chain, block) | |
| cb.blocksByHash.Store(block.Hash(), block) | |
| } | |
| func (cb *ChainBuilder) Insert(tb testing.TB, b *blocks.Block) { | |
| tb.Helper() | |
| if len(cb.chain) > 0 { | |
| p := cb.Last() | |
| require.Equalf(tb, p.Height()+1, b.Height(), "%T.Insert() non-incrementing block height", cb) | |
| require.Equalf(tb, p.Hash(), b.ParentHash(), "%T.Insert() with mismatched parent hash", cb) | |
| } | |
| cb.chain = append(cb.chain, b) | |
| cb.blocksByHash.Store(b.Hash(), b) | |
| } |
Otherwise we risk breaking internal invariants.
There was a problem hiding this comment.
My initial thought was we should not fail any tests in those cases as it's intentional to insert invalid blocks (as stated here).
However it seems this should be fine. I checked that in upstream by putting a panic here and seems there is no such case https://github.com/ava-labs/libevm/blob/main/core/blockchain.go#L1532-L1533.
Anyway this change should fine for this PR as we do not enable InvalidBlocks tests yet.
There was a problem hiding this comment.
However it seems this should be fine. I checked that in upstream by putting a panic here and seems there is no such case https://github.com/ava-labs/libevm/blob/main/core/blockchain.go#L1532-L1533.
On the second thought: This only checks if the given chain is ordered and linked, but it does not really check anything if it's a chain of single block (in a case we're inserting a single block). So tests actually expect a returned error (not failed test) in such cases.
As I said it won't fail in this PR, but it seems we need to return errors when we actually insert blocks (either in this blockstest package or through a prod-level chain builder)
blocks/blockstest/chain.go
Outdated
|
|
||
| // GetHashAtHeight returns the hash of the block at the given height, and a flag indicating if it was found. | ||
| // If the height is greater than the number of blocks in the chain, it returns an empty hash and false. | ||
| func (cb *ChainBuilder) GetHashAtHeight(num uint64) (common.Hash, bool) { |
There was a problem hiding this comment.
This is only ever used to pass the hash into ChainBuilder.GetBlock() so could just be BlockByNumber(testing.TB, uint64) *blocks.Block and similarly GetNumberByHash() could just be BlockByHash(testing.TB, common.Hash) *blocks.Block.
Since they don't need to conform to any particular signature (unlike GetBlock(), which is a blocks.Source) they can do the test failures internally if the block doesn't exist.
There was a problem hiding this comment.
I changed them but omitted the TB param, since we're calling these from the readerAdapter from consensus.ChainHeaderReader , which we cannot change the signature and take TB. We can set a TB for the readerAdapter when we create them but not sure if it's a good idea to carry it around.
| // using 4.6 TGas | ||
| bt.skipLoad(`.*randomStatetest94.json.*`) | ||
|
|
||
| // TODO(cey): We cannot run invalid blocks, since we don't currently have pre-insert checks. |
There was a problem hiding this comment.
Most of this file is a copy-pasta from libevm
Why? The EEST documentation states that they are...
test fixtures (JSON) which can be consumed by any execution client to verify their implementation
...meaning that they're not even language-specific, let alone related to an implementation. I very strongly believe that we should be using these as inputs in an SAE-specific manner instead of trying to shoe-horn SAE into geth.
| opts = append(opts, withGenesisSpec(gspec)) | ||
|
|
||
| // Wrap the original engine within the beacon-engine | ||
| engine := beacon.New(ethash.NewFaker()) |
There was a problem hiding this comment.
Invoking the Beacon chain is IMO strongly suggestive of this not being an appropriate approach.
There was a problem hiding this comment.
indeed, this is why we require #72. Otherwise we will always need to comply with Ethereum rules and specs/fixtures for testing these.
There was a problem hiding this comment.
There was a problem hiding this comment.
While #72 will be crucial for coreth/subnet-evm, bu SAE is also not 1-1 compliant with those JSON fixtures (baseFee, missing blob handling, beacon etc). We're currently patching those through upstream engines and before/after hooks with upstream-like handlings (difficulty, coinbase, beacon etc). In fact we should generate those fixtures without running any of these (and skip some specs/fixtures like beacon/difficulty/blobs i.e stuff we don't really support/have)
I'm not sure what you mean by parsing JSON. We do parse them, create blocks from those JSONs and run blocks directly against the executor (here). If you mean modifying JSON that will be generating fixtures (as described in #72).
|
|
||
| // TestExecutionSpecBlocktests runs the test fixtures from execution-spec-tests. | ||
| func TestExecutionSpecBlocktests(t *testing.T) { | ||
| if !common.FileExist(executionSpecBlockchainTestDir) { |
There was a problem hiding this comment.
Then why is it included in this PR?
| return validBlocks, nil | ||
| } | ||
|
|
||
| func insertWithHeaderBaseFee(tb testing.TB, sut *SUT, bs types.Blocks) { |
There was a problem hiding this comment.
This is a useful primitive so please abstract blockstest.WithFakeBaseFee(testing.TB, *blocks.Block, [other necessary args]) *blocks.Block.
This reverts commit 1636b3d.
This PR adds the test execution (from https://github.com/ava-labs/libevm/tree/main/tests) and
testdataas a submodule from the repository https://github.com/ethereum/tests (atfa51c5c164as same commit in libevm@main). It only executesBlockchainTestsandLegacyTestswhich are inserting test blockchains (in json files) with SAE's own blockbuilder/chain (throughSUT) and then executes withsaexec. Tests are passing but with few limitations and hacks (seesaexec/ethtests/block_test.goand comments for that file) :1-
GeneralStateTestare too slow tried to run them but CI fails see comment2- We cannot run
InvalidBlockstest because they're asserting either that block insertion is failing (which we don't have any verification yet), or they fail at execution (which we don't allow). We can revisit them once we add the verification.3- There are some tests like
bcForkStressTestandbcMultiChainTesttesting multi-chains and forks with non-linear blocks and multiple possible chains. We currently don't support this in test chain builders. So these tests are skipped.4- We don't have any execution pre-checks yet (like MaxCodeSize and intrinsic gas checks) so some tests are failing with a
FAILlog in saexec. Once we have those checks we can execute those tests.5- SAE has it's own baseFee applied to the header just before the execution, which means header's baseFee won't directly applied to the state where EVM tests expect it to be applied as it's; thus failing some state tests like balances, rewards etc. This is currently handled by faking the gas clock in
insertWithHeaderBaseFee