Skip to content

Conversation

@jflo
Copy link
Contributor

@jflo jflo commented Jan 30, 2025

fixes first part of #8204 by using BadBlockManager from protocol context

@jflo jflo requested a review from matkt January 30, 2025 20:43
macfarla
macfarla previously approved these changes Jan 31, 2025
@matkt
Copy link
Contributor

matkt commented Jan 31, 2025

did you had time to test it with a stateroot mismatch issue ? I saw on my last test that with a stateroot mismatch we are passing here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50R215 and in this case we are not adding the bad block

@github-actions
Copy link

github-actions bot commented Mar 3, 2025

This pr is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the Stale label Mar 3, 2025
@macfarla macfarla dismissed their stale review March 3, 2025 02:20

I'm dismissing my review pending q from matkt

@github-actions github-actions bot removed the Stale label Mar 4, 2025
@garyschulte garyschulte force-pushed the isolateBadBlockManager branch 2 times, most recently from 69fad3c to d9b0564 Compare March 11, 2025 15:31
@garyschulte
Copy link
Contributor

did you had time to test it with a stateroot mismatch issue ? I saw on my last test that with a stateroot mismatch we are passing here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50R215 and in this case we are not adding the bad block

@matkt when you have a moment, pls check this commit for context: d9b0564

@matkt
Copy link
Contributor

matkt commented Mar 18, 2025

did you had time to test it with a stateroot mismatch issue ? I saw on my last test that with a stateroot mismatch we are passing here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50R215 and in this case we are not adding the bad block

@matkt when you have a moment, pls check this commit for context: d9b0564

I see that you are catching a new execption for the stateroot mismatch but I don't see how it could fix the issue

In my case the I had an invalid stateroot and we passed here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50L221. With your commit it seems to be the same or maybe I missing something 🤔

@garyschulte
Copy link
Contributor

did you had time to test it with a stateroot mismatch issue ? I saw on my last test that with a stateroot mismatch we are passing here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50R215 and in this case we are not adding the bad block

can you give a test case? previously bonsai stateroot mismatches were never being added as bad blocks, but were being treated as a database layer problem. This PR should address that. LMK if there is a reproducible case where a stateroot mismatch does not result in adding to the bad block manager.

@matkt
Copy link
Contributor

matkt commented Mar 25, 2025

did you had time to test it with a stateroot mismatch issue ? I saw on my last test that with a stateroot mismatch we are passing here https://github.com/hyperledger/besu/pull/8207/files#diff-e390f1feab110aff3f324e7e36e681b676198067f3b8932ff3abe97f0793df50R215 and in this case we are not adding the bad block

can you give a test case? previously bonsai stateroot mismatches were never being added as bad blocks, but were being treated as a database layer problem. This PR should address that. LMK if there is a reproducible case where a stateroot mismatch does not result in adding to the bad block manager.

Don't have a unit test but it was pretty easy to reproduce the issue. I think if you force a stateroot mismatch by changing the gas cost of an operation and try to sync on mainnet you will be able to have it

@matkt
Copy link
Contributor

matkt commented Apr 2, 2025

Checked and it's working, I saw that you are using

public BlockProcessingResult(
      final Optional<BlockProcessingOutputs> yield, final String errorMessage) {
    super(errorMessage);
    this.yield = yield;
    this.isPartial = false;
  }

in case of a stateroot mismatch and not

 public BlockProcessingResult(
    final Optional<BlockProcessingOutputs> yield, final Throwable cause) {
  super(cause.getLocalizedMessage(), cause);
  this.yield = yield;
  this.isPartial = false;
}

so it will work

Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

LGTM

@garyschulte garyschulte force-pushed the isolateBadBlockManager branch from d9b0564 to e7dc076 Compare April 2, 2025 19:07
@garyschulte garyschulte enabled auto-merge (squash) April 2, 2025 19:07
Signed-off-by: garyschulte <[email protected]>
@garyschulte garyschulte merged commit d6228ca into hyperledger:main Apr 2, 2025
43 checks passed
@garyschulte garyschulte deleted the isolateBadBlockManager branch April 2, 2025 20:46
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