Skip to content
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 tests #26314

Merged
merged 14 commits into from
Dec 20, 2022
Merged

tests: update tests #26314

merged 14 commits into from
Dec 20, 2022

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 6, 2022

This PR builds on #26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge.

This update contains an interesting error:

--- FAIL: TestBlockchain (0.01s)
    --- FAIL: TestBlockchain/TransitionTests/bcArrowGlacierToMerge/powToPosBlockRejection.json (0.01s)
        block_test.go:51: test without snapshotter failed: block (index 11) insertion should have failed due to: 3675PoWBlockRejected
        block_test.go:54: test with snapshotter failed: block (index 11) insertion should have failed due to: 3675PoWBlockRejected

Here, we try to import some blocks, and one of the blocks goes over the TTD. The test
expects us to reject the last one, however, we have this little thing in the
beacon/consensus.go:

func (beacon *Beacon) VerifyHeaders(chain consensus.ChainHeaderReader, headers []*types.Header, seals []bool) (chan<- struct{}, <-chan error) {
	if !beacon.IsPoSHeader(headers[len(headers)-1]) {
		return beacon.ethone.VerifyHeaders(chain, headers, seals)
	}
	var (
		preHeaders  []*types.Header
		postHeaders []*types.Header

That is, we check the very last header -- if it looks like a PoW-header, we ship all
headers to the ethone engine for verification, and done.

In the other path, where we go through the work of separating them up in pre- and post- headers,
we're much more careful about ensuring that the PoW headers do not exceed the TTD:

		// Verify that pre-merge headers don't overflow the TTD
		if index, err := verifyTerminalPoWBlock(chain, preHeaders); err != nil {

This is something we need to investigate, I'm not sure exactly what is The Correct Fix.

@MariusVanDerWijden
Copy link
Member

Hmm you're right I think. If we get a long pow chain, we only verify it with the inner engine and in there we don't verify that the block isn't post ttd afaict

@holiman holiman changed the title Test update merge tests: update tests Dec 6, 2022
@holiman holiman marked this pull request as ready for review December 6, 2022 12:09
@fjl fjl removed the status:triage label Dec 6, 2022
@rjl493456442
Copy link
Member

If the header batch contains all PoW style headers and some of them are above the TTD, it means they come from the legacy PoW network which should be rejected in the first place.

There are some logics in the fetcher https://github.com/ethereum/go-ethereum/blob/master/eth/handler.go#L279

I mean these logics are not embedded in the consensus engine but we hold the assumption that they should be rejected.

@holiman
Copy link
Contributor Author

holiman commented Dec 6, 2022

I mean these logics are not embedded in the consensus engine but we hold the assumption that they should be rejected.

IMO these checks should be embedded in the (beacon) consensus engine.. So I'll make a stab at that. Also, we could merge this PR as is, with the test commented-out, and fix that test in a follow-up PR. If someone approves this ...

@holiman
Copy link
Contributor Author

holiman commented Dec 6, 2022

I have now applied a fix, please take a look

@holiman
Copy link
Contributor Author

holiman commented Dec 7, 2022

When I was fixing the failing tests, I also got around to fix the broken test which is also touched in #26272 .

@holiman holiman requested a review from gballet as a code owner December 7, 2022 19:03
@holiman
Copy link
Contributor Author

holiman commented Dec 9, 2022

Note for reviewers: the changes look large, but most of it is in tests. The core change is here: https://github.com/ethereum/go-ethereum/pull/26314/files#diff-89272f61e115723833d498a0acbe59fa2286e3dc7276a676a7f7816f21e248b7

consensus/beacon/consensus.go Outdated Show resolved Hide resolved
consensus/beacon/consensus.go Outdated Show resolved Hide resolved
@@ -2006,7 +2011,10 @@ func testSideImport(t *testing.T, numCanonBlocksInSidechain, blocksBetweenCommon
merger.ReachTTD()
merger.FinalizePoS()
// Set the terminal total difficulty in the config
gspec.Config.TerminalTotalDifficulty = big.NewInt(int64(len(blocks)))
ttd := big.NewInt(int64(len(blocks)))
ttd.Mul(ttd, params.GenesisDifficulty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we sum up the difficulty of past blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, they are all using the same difficulty of params.GenesisDifficulty. So n*GenesisDifficulty menas that n-1 reaches ttd, since genesis also has a difficulty.
If there's an off by one here, it seems that the test ignore it.
... It was humongously erroneous before this change...

@rjl493456442
Copy link
Member

@karalabe @MariusVanDerWijden Please also check the change in consensus/beacon package.

@MariusVanDerWijden
Copy link
Member

Tried this on hive and the following tests break:

INFO[12-19|16:19:10] API: test started                        suite=0 test=1 name="Invalid Terminal Block in ForkchoiceUpdated (go-ethereum)"
INFO[12-19|16:19:12] API: client go-ethereum started          suite=0 test=1 container=1c54ed85
INFO[12-19|16:19:12] API: test ended                          suite=0 test=1 pass=false
INFO[12-19|16:19:12] API: test started                        suite=0 test=2 name="Invalid GetPayload Under PoW (go-ethereum)"
INFO[12-19|16:19:14] API: client go-ethereum started          suite=0 test=2 container=6c74bb76
INFO[12-19|16:19:14] API: test ended                          suite=0 test=2 pass=false
INFO[12-19|16:19:14] API: test started                        suite=0 test=3 name="Invalid Terminal Block in NewPayload (go-ethereum)"
INFO[12-19|16:19:16] API: client go-ethereum started          suite=0 test=3 container=8978a659
INFO[12-19|16:19:16] API: test ended                          suite=0 test=3 pass=false

@holiman
Copy link
Contributor Author

holiman commented Dec 19, 2022

Tried this on hive

Which simulator is that?

@holiman
Copy link
Contributor Author

holiman commented Dec 19, 2022

Ah, it's these ones: https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/README.md#engine-api-negative-test-cases

Invalid Terminal Block in ForkchoiceUpdated:
Client should reject ForkchoiceUpdated directives if the referenced HeadBlockHash does not meet the TTD requirement.

@holiman
Copy link
Contributor Author

holiman commented Dec 20, 2022

It's still running, but so far (115 tests) all tests pass for me

INFO[12-20|09:19:09] API: test started                        suite=0 test=1 name="Invalid Terminal Block in ForkchoiceUpdated (go-ethereum_latest)"
INFO[12-20|09:19:13] API: client go-ethereum_latest started   suite=0 test=1 container=a9d15988
INFO[12-20|09:19:16] API: test ended                          suite=0 test=1 pass=true

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@holiman holiman added this to the 1.11.0 milestone Dec 20, 2022
@holiman holiman merged commit b818e73 into ethereum:master Dec 20, 2022
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
This PR builds on ethereum#26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge.

This change adds checks to the beacon consensus engine, making it more strict in validating the pre- and post-headers, and not relying on the caller to have already correctly sanitized the headers/blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants