-
Notifications
You must be signed in to change notification settings - Fork 185
feat: Implement a support for the new BN response BehindPublisher
#22540
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Petar Tonev <[email protected]>
Signed-off-by: Petar Tonev <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Signed-off-by: Petar Tonev <[email protected]>
| * | ||
| * @param nodeBehind the BehindPublisher response received from the block node | ||
| */ | ||
| private void handleBlockNodeBehind(@NonNull final BehindPublisher nodeBehind) { |
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.
BehindPublisher is not an EndStream condition and thus the connection should not be closed and started again with the new block. Additionally, there needs to be a condition to respond with an ERROR if the block requested is above the latest block produced by the CN. Basically the handling of BehindPublisher should be the exact same as handling a ResendBlock message - just logging/metrics changed.
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.
The implementation is inspired from these changes in the protocol document in BN repo.
It says that the block node no longer closes the stream when behind, but
waits for the Publisher to either stream an earlier block or
send "too far behind". By sending too far behind, we are essentially closing the stream and rescheduling
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.
if (blockBufferService.getBlockState(restartBlockNumber) != null) {
logger.info("{} Block node reported it is behind. Will restart stream at block {}.", this, restartBlockNumber);
closeAndRestart(restartBlockNumber);
} else { ... }
The if block above calls #closeAndRestart(long). That is the part I am talking about. The document was updated to indicate this:
BlockNode -->> Publisher: Respond with BehindPublisher and specify the latest completed block (L)
Publisher-->>BlockNode: Send from block L+1 or send EndStream(Too Far Behind) and retry with exponential backoff
Only if the block isn't available should the connection be closed, otherwise the connection should not be closed and we should just turn around and start sending the requested block, like we do for ResendBlock
Description:
This PR adds support for the new Block Node response
BehindPublisher.BlockNodeConnection#onNext()method to handle the new responseBehindPublisherreceivedblock-stream.jsondashboard file accordinglyRelated issue(s):
Fixes #
Notes for reviewer:
! ---DO NOT MERGE BEFORE RELEASE
0.25.0of the Block Node --- !Checklist