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

Don't mark block complete twice #3487

Closed

Conversation

alsrdn
Copy link
Contributor

@alsrdn alsrdn commented Dec 14, 2022

Don't mark the block complete from the Block Accumulator component.
This fixes: #3458

Also sligthly changed the name of the effect to be more suggestive.

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>
The block accumulator would mark a block completed in storage if it was
executed. Since the block is already marked as completed in the reactor
control logic after a block is executed and announced and also by the
block synchronizer when it's synchronizing historical blocks, there's
no need to mark it complete again in the block accumulator.
This fixes a bug where the block was marked completed two times in rapid
succession.

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>
Removed ShouldStore::SingleSignature variant for the block acceptor code
since it was never used there and refactored the code sligtly for better
readablility.

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>
@alsrdn alsrdn marked this pull request as ready for review December 14, 2022 14:05
@alsrdn alsrdn changed the title Mark complete bug Don't mark block complete twice Dec 14, 2022
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Makes sense to me.
Trying to remember why put_complete_block_to_storage was added in the first place. Was it just because the synchronizer didn't mark them as complete yet at that time? Maybe another pair of reviewer eyes would be good.

node/src/reactor/main_reactor.rs Show resolved Hide resolved
node/src/components/storage/tests.rs Outdated Show resolved Hide resolved
});
if response == true {
harness.send_request(storage, move |responder| {
BlockCompleteConfirmationRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that something specific to these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it really needs to mark them complete to be honest. This was changed in 503c4d0 so I wanted to preserve the same semantics due to that change.

Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rafal-ch
Copy link
Contributor

Applied the do-not-merge as we need to run some tests here.

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>
@alsrdn
Copy link
Contributor Author

alsrdn commented Jan 3, 2023

will be addressed by #3520

@alsrdn alsrdn closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants