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

fix: reorgs with forks including invalid blocks #10320

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

fgimenez
Copy link
Member

These changes primarily address potential reorg-related failures raised by failing hive tests.

  • TreeState::on_new_head includes two fixes
    • when creating the new and old chains, insert blocks in the proper order
    • before extending new chain checks if we are not in a fork, so that we address deep forks
  • TreeState::is_fork: besides walking back from the given target hash and trying to find the canonical head (which would only address canonical chain extensions), it also checks walking back from the canonical head and tries to find the target hash.
  • tests:
    • new unit tests added to cover scenarios functionally equivalent to hive tests
    • fixes to properly exercise newPayload calls, both in test block creation and in the CancunPayloadFields passed to the call
    • new functionality in TestHarness and refactors

@fgimenez fgimenez requested a review from jenpaff August 15, 2024 17:02
@fgimenez fgimenez added C-bug An unexpected or incorrect behavior C-test A change that impacts how or what we test A-engine Related to the engine implementation C-hivetest Used for labelling automated issues relating to hive test failures labels Aug 15, 2024
@fgimenez fgimenez force-pushed the fgimenez/hive-tests-reorg-with-invalid-blocks branch from 755a57d to 2b7485b Compare August 16, 2024 23:09
fgimenez added a commit that referenced this pull request Aug 17, 2024
@fgimenez fgimenez force-pushed the fgimenez/hive-tests-reorg-with-invalid-blocks branch from 2b7485b to 04bd239 Compare August 18, 2024 00:33
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

awesome, I really like all of the new tests, and just have some nits

@@ -253,7 +264,7 @@ impl TreeState {

while old_hash != current_hash {
if let Some(block) = self.blocks_by_hash.get(&old_hash) {
old_chain.push(block.clone());
old_chain.insert(0, block.clone());
Copy link
Member

Choose a reason for hiding this comment

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

since we seem to be only inserting at the front, it might be worth doing push and then reverse like we do with new_chain

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, done here 3f8a5bc

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

very cool

pending @Rjected suggestions

@fgimenez fgimenez force-pushed the fgimenez/hive-tests-reorg-with-invalid-blocks branch from d879eda to 3f8a5bc Compare August 20, 2024 10:02
@fgimenez fgimenez enabled auto-merge August 20, 2024 10:03
@fgimenez fgimenez added this pull request to the merge queue Aug 20, 2024
Merged via the queue into main with commit 3373670 Aug 20, 2024
35 checks passed
@fgimenez fgimenez deleted the fgimenez/hive-tests-reorg-with-invalid-blocks branch August 20, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-engine Related to the engine implementation C-bug An unexpected or incorrect behavior C-hivetest Used for labelling automated issues relating to hive test failures C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants