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

pd: use commit_in_place to write app safeguard #4954

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Dec 9, 2024

Describe your changes

This PR:

  • fixes a bug in the app version safeguard logic that would cause discrepancies between JMT state versions and block height
  • refactors some of the inner logic to avoid writing any state unless it is necessary (i.e, no safeguard was found)

Testing this:

To observe the state discrepancy and test remediation:

  1. Checkout the commit that only contains the extra tracing statement: git checkout a8c51f0cacb64304a6b1781226d6f74376800af7
  2. Create a local devnet
  3. Restart the devnet N times
  4. Observe that the state version and block height have offset=N

To test that this fixes it:

  1. Run this patch
  2. Restart the node
  3. Observe that the state version stays stable.

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes.

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Not consensus breaking.

@erwanor erwanor added the A-node Area: System design and implementation for node software label Dec 9, 2024
@erwanor erwanor requested a review from conorsch December 9, 2024 23:02
@erwanor erwanor self-assigned this Dec 9, 2024
@conorsch
Copy link
Contributor

conorsch commented Dec 9, 2024

Refs #4919.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Thanks for the test plan, made it simple to verify the fix. 🙏

@conorsch conorsch merged commit 22602d7 into main Dec 9, 2024
14 checks passed
@conorsch conorsch deleted the erwan/fix_commit_in_place_uip6 branch December 9, 2024 23:32
zbuc pushed a commit that referenced this pull request Dec 16, 2024
## Describe your changes

This adds a `pcli query chain detect-desync` command which detect if a
full node is affected by state desync (#4954).

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Client change
conorsch pushed a commit that referenced this pull request Dec 16, 2024
## Describe your changes

This adds a `pcli query chain detect-desync` command which detect if a
full node is affected by state desync (#4954).

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Client change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-node Area: System design and implementation for node software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants