-
Notifications
You must be signed in to change notification settings - Fork 20.3k
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
tests: update tests #26314
Conversation
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 |
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. |
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 ... |
I have now applied a fix, please take a look |
f29104f
to
af5af02
Compare
eaa6e3a
to
6d43e16
Compare
…fficulty into account
When I was fixing the failing tests, I also got around to fix the broken test which is also touched in #26272 . |
66e4f91
to
0c5243f
Compare
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 |
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
@karalabe @MariusVanDerWijden Please also check the change in |
Tried this on hive and the following tests break:
|
Which simulator is that? |
Ah, it's these ones: https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/README.md#engine-api-negative-test-cases
|
It's still running, but so far (115 tests) all tests pass for me
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
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:
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
: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:
This is something we need to investigate, I'm not sure exactly what is The Correct Fix.