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.

Sign up for GitHub

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

Jump to bottom

retry block processing #4600

Merged
merged 18 commits into from
Nov 18, 2022
Merged

retry block processing #4600

merged 18 commits into from
Nov 18, 2022

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Nov 4, 2022 •
edited
Loading

Signed-off-by: Justin Florentine justin+github@florentine.us

  • if MerkleTrieException, don't mark block as INVALID - and mark for retry
  • do not use yield.isPresent to indicate whether block processing was successful

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@jflo
aggresively seeking out other places to defend from this
2f089fd 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@macfarla macfarla changed the title aggresively seeking out other places to defend from this retry block processing Nov 4, 2022
@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2022

This pull request introduces 1 alert when merging e8f8efc into 0287ec5 - view on LGTM.com

new alerts:

  • 1 for Unused format argument

@jflo
adds test to throw merkletrie during block iteration
6239984 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2022

This pull request introduces 1 alert when merging 6239984 into 0287ec5 - view on LGTM.com

new alerts:

  • 1 for Unused format argument

@jflo
confirms that bad block manager isn't added to outside of mergeCoordi…
78dea2e 
…nator

Signed-off-by: Justin Florentine <justin+github@florentine.us>
@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2022

This pull request introduces 1 alert when merging 78dea2e into fcf640a - view on LGTM.com

new alerts:

  • 1 for Unused format argument

@jflo
more testing, spotless
654e4c2 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 1 alert when merging 654e4c2 into fcf640a - view on LGTM.com

new alerts:

  • 1 for Unused format argument

jflo added 5 commits November 11, 2022 15:22
@jflo
pr cleanup
fe58be1 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
Merge branch 'main' into 4512-merkle-handle
f93d018
@jflo
javadoc fix
388b5e9 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
removed unused param
8325876 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
Merge branch 'main' into 4512-merkle-handle
bf4aaf3
@garyschulte garyschulte mentioned this pull request Nov 12, 2022
Merged
2 tasks
@jflo jflo enabled auto-merge (squash) November 17, 2022 17:40
fab-10
fab-10 reviewed Nov 18, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, but before shipping I have some questions and suggestions

...merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java Show resolved Hide resolved
...e/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java Show resolved Hide resolved
.../org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadTest.java Outdated Show resolved Hide resolved
.../org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadTest.java Outdated
Comment on lines 269 to 288
@Test
public void shouldNotReturnInvalidOnThrownMerkleTrieException() {
BlockHeader mockHeader = createBlockHeader();
when(blockchain.getBlockByHash(mockHeader.getHash())).thenReturn(Optional.empty());
when(blockchain.getBlockHeader(mockHeader.getParentHash()))
.thenReturn(Optional.of(mock(BlockHeader.class)));
when(mergeCoordinator.getLatestValidAncestor(any(BlockHeader.class)))
.thenReturn(Optional.of(mockHash));
when(mergeCoordinator.latestValidAncestorDescendsFromTerminal(any(BlockHeader.class)))
.thenReturn(true);
when(mergeCoordinator.rememberBlock(any())).thenThrow(new MerkleTrieException("missing leaf"));

var resp = resp(mockPayload(mockHeader, Collections.emptyList()));

verify(engineCallListener, times(1)).executionEngineCalled();
verify(mergeCoordinator, never()).addBadBlock(any(), any());
// verify mainnetBlockValidator does not add to bad block manager

fromErrorResp(resp);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it's me or this seems equals to the previous test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Previous test was covering that the exception had been caught, and wrapped into a Result. This is the case where it is not handled, and the exception is thrown upward of rememberBlock().

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/MainnetBlockValidator.java
}
badBlockManager.addBadBlock(invalidBlock, result.causedBy());
Copy link
Contributor

Choose a reason for hiding this comment

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

before an internal error did not cause the block to be added to the bad list, now it is always added, is this wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

The filtering out of badBlocks with a causedBy that is internal is now handled inside the badBlockManager

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/BadBlockManager.java Outdated
@@ -78,4 +83,12 @@ public void addLatestValidHash(final Hash blockHash, final Hash latestValidHash)
public Optional<Hash> getLatestValidHash(final Hash blockHash) {
return Optional.ofNullable(latestValidHashes.getIfPresent(blockHash));
}

public boolean isInternalError(final Throwable causedBy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private?

@jflo
adopts pr suggestions
ea0f570 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo jflo requested a review from fab-10 November 18, 2022 16:45
jflo added 2 commits November 18, 2022 13:42
@jflo
adopts pr suggestions
75cd230 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
Merge branch 'main' into 4512-merkle-handle
6f5058a
@jflo jflo merged commit e5d73e0 into hyperledger:main Nov 18, 2022
macfarla added a commit to macfarla/besu that referenced this pull request Jan 10, 2023
@macfarla @jflo
retry block processing (hyperledger#4600)
8d25e63 
* aggresively seeking out other places to defend from this

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Co-authored-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@macfarla @jflo @eum602
retry block processing (hyperledger#4600)
9932fca 
* aggresively seeking out other places to defend from this

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Co-authored-by: Justin Florentine <justin+github@florentine.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jflo jflo jflo left review comments

@fab-10 fab-10 fab-10 approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@macfarla @jflo @fab-10