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

[#6301] Track bad block cause #6622

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

mbaxter
Copy link
Contributor

@mbaxter mbaxter commented Feb 27, 2024

PR description

Track bad block causes in BadBlockManager in preparation for exposing plugin events. This data is useful because a consumer may want to take a different action depending on the cause. For example, for #6301, we probably want to trace blocks with an actual validation error and ignore blocks that just descend from a bad block.

Summary of changes:

  • Require a BadBlockCause when pushing blocks to BadBlockManager
  • Simplify bad block checks by adding utility method BadBlockManager.isBadBlock
  • Add some basic tests for BadBlockManager
  • In event handler MergeCoordinator.onBadChain, only push descendants of the bad block to theBadBlockManager. The bad block itself will already have been pushed into the BadBlockManager when processed in MainnetBlockValidator and the MergeCoordinator does not have the context on why the bad block was marked as bad.
  • Remove unused API MergeMiningCoordinator.addBadBlock
  • Fix / add tests to MainnetBlockValidatorTest
    • Add tests for fastBlockValidation()
    • Set all mocks upfront to ensure every test executes from a baseline of success. Previously several tests were passing artificially. The mocks were not set up correctly so some tests were not hitting the cases that were intended.
  • Create some factory methods on InvalidBlockException to handle cases where (previously required) block data is not available
  • Disambiguate a few different types of errors in DownloadHeaderSequenceTask and add tests

Fixed Issue(s)

Part of #6301

Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
@mbaxter mbaxter marked this pull request as ready for review February 28, 2024 20:27
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
@mbaxter
Copy link
Contributor Author

mbaxter commented Feb 28, 2024

@siladu - I have already pinged @garyschulte and @jflo for a review if you are busy

Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Copy link
Contributor

@jflo jflo left a comment

Choose a reason for hiding this comment

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

Overall big improvement in observability and readability! I think we need to think through the treatment of any RuntimeException as a bad block, that is a lot broader than known application exceptions like StorageException and MerkleTrieException. If that gets thrown unexpectedly and a protocol-compliant block is marked as bad, there will be a chain halt.

@mbaxter mbaxter requested a review from jflo March 4, 2024 15:46
@mbaxter mbaxter enabled auto-merge (squash) March 4, 2024 21:29
@mbaxter
Copy link
Contributor Author

mbaxter commented Mar 4, 2024

I think we need to think through the treatment of any RuntimeException as a bad block, that is a lot broader than known application exceptions like StorageException and MerkleTrieException. If that gets thrown unexpectedly and a protocol-compliant block is marked as bad, there will be a chain halt.

See inline response here.

@mbaxter mbaxter merged commit 240edd4 into hyperledger:main Mar 4, 2024
88 of 93 checks passed
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
Signed-off-by: mbaxter <mbaxter.dev@gmail.com>
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.

2 participants