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

Block processing should use storage atomically #21

Open
Tracked by #1644
tzdybal opened this issue Jul 26, 2022 · 5 comments · Fixed by rollkit/rollkit#1782
Open
Tracked by #1644

Block processing should use storage atomically #21

tzdybal opened this issue Jul 26, 2022 · 5 comments · Fixed by rollkit/rollkit#1782
Assignees

Comments

@tzdybal
Copy link
Member

tzdybal commented Jul 26, 2022

Currently ApplyBlock, SaveBlock, Commit, SaveBlockResponses and SetHeight are executed as completely separate operations - this potentially leads to inconsistencies in database.
https://github.com/celestiaorg/optimint/blob/b1df4346508c124d59504555f34210773a4e5b50/block/manager.go#L216-L247
https://github.com/celestiaorg/optimint/blob/b1df4346508c124d59504555f34210773a4e5b50/block/manager.go#L392-L433

Ideally all those operations should use single KV batch to ensure atomicity.

Ref: rollkit/rollkit#457, rollkit/rollkit#474

@bd21
Copy link

bd21 commented Jul 30, 2022

Something like this?

Wanted to check if this was the right approach before refactoring everything

@Manav-Aggarwal
Copy link
Member

Talked to @tzdybal about this and he mentioned "this can only be visible when a validator process is killed brutally". Removed va tag.

@Manav-Aggarwal Manav-Aggarwal removed this from Rollkit Nov 30, 2023
@tzdybal
Copy link
Member Author

tzdybal commented Jun 5, 2024

It seems this can also be visible in certain circumstances (high load of node machine and very quick RPC queries) when for example BlockResults is called with height returned by Status.

@Manav-Aggarwal
Copy link
Member

A team ran into an issue when using status and block_results endpoint that noticed this issue.

From @tzdybal: We save block information and update height in the code just before saving block responses - and it's not atomic/transactional. because of that status may report a block for which block responses are not yet available.
As this is a race condition, high load and/or low-perf machine/VM makes this more visible.

@tuxcanfly tuxcanfly moved this from Todo to In Progress in Rollkit Jul 19, 2024
@Manav-Aggarwal Manav-Aggarwal moved this from In Progress to For Review in Rollkit Aug 12, 2024
github-merge-queue bot referenced this issue in rollkit/rollkit Aug 15, 2024
## Overview

Update height only after block responses are saved.

Partially fixes #477 

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit


- **New Features**
- Enhanced testing capabilities for the block publishing process,
including validation for successful executions and error handling.

- **Bug Fixes**
- Improved state management logic during block publication to enhance
consistency and error handling.

- **Tests**
- Added new test cases to ensure the correct behavior of the block
publishing method under various scenarios.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@github-project-automation github-project-automation bot moved this from For Review to Done in Rollkit Aug 15, 2024
@tzdybal
Copy link
Member Author

tzdybal commented Oct 8, 2024

This was not fixed by rollkit/rollkit#1782.

@tzdybal tzdybal reopened this Oct 8, 2024
@Manav-Aggarwal Manav-Aggarwal transferred this issue from rollkit/rollkit Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants