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

Fix pending change after flushing RegisterView #3133

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Jan 14, 2025

Motivation

@afck ran into an issue where an assertion caught unexpected pending changes after voting for a block proposal. After investigating the issue, the cause seems to be that RegisterView::flush had an edge case where it wouldn't clear it's update field.

Proposal

Clear the update field after successfully flushing.

Test Plan

A unit test was added to reproduce the issue.

Release Plan

  • These changes should be backported to the latest devnet branch, then
    • be released in a validator hotfix.
  • These changes should be backported to the latest testnet branch, then
    • be released in a validator hotfix.

Links

jvff added 2 commits January 14, 2025 17:13
Ensure that it leaves no pending changes.
Remove all pending changes when flushing.
@jvff jvff added the bug Something isn't working label Jan 14, 2025
@jvff jvff added this to the Testnet #2 milestone Jan 14, 2025
@jvff jvff requested review from MathieuDutSik and afck January 14, 2025 17:21
@jvff jvff self-assigned this Jan 14, 2025
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.

Thank you! 🎉 🙏

@jvff jvff merged commit 3262d88 into linera-io:main Jan 14, 2025
23 checks passed
@jvff jvff deleted the fix-register-view-flush-not-clearing-update branch January 14, 2025 17:59
@@ -99,6 +99,7 @@ where
self.stored_value = value;
}
self.delete_storage_first = false;
self.update = None;
Copy link
Contributor

Choose a reason for hiding this comment

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

Crazy that this bug survived so long

afck added a commit that referenced this pull request Jan 15, 2025
## Motivation

Currently the chain manager is serialized as a whole and stored in a
`RegisterView` in the chain state view. Since it contains blobs it can
be very large, and the blobs are not needed every time the chain manager
is loaded.

## Proposal

Make the chain manager a `View`, and put the blobs in a `MapView`.

## Test Plan

This doesn't change any logic, so CI should catch regressions. (In fact,
it already did: #3133)

## Release Plan

- Nothing to do / These changes follow the usual release cycle.

## Links

- In preparation for:
#3048
- [reviewer
checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants