8000 fix(cryptarchia): don't include not-yet-pruned blocks in the service state by youngjoon-lee · Pull Request #1398 · logos-co/nomos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(cryptarchia): don't include not-yet-pruned blocks in the service state #1398

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(cryptarchia): don't include not-yet-pruned blocks in the service state #1398

wants to merge 2 commits into from
+42 −104

Conversation

youngjoon-lee
Copy link
Contributor
@youngjoon-lee youngjoon-lee commented Jun 26, 2025 •
edited
Loading

1. What does this PR implement?

As described in many code comments, we defined the CryptarchiaConsensusState (which is for recovery) to store prunable_blocks as well as the tip and the LIB.
Here, the prunable_blocks means the blocks 8000 which were pruned from memory (cryptarchia engine) but not from storage.

It's good. But in the current implementation, the CryptarchiaConsensusState was also storing blocks that are not yet pruned in the cryptarchia engine.
Whenever we rebuild the CryptarchiaConsensusState, we fetch prunable blocks from the cryptarchia engine and include them into the CryptarchiaConsensusState.
However, those blocks don't need to be included because they are not yet pruned in the cryptarchia engine.

This PR simply removes fetching prunable blocks from the cryptarchia engine.
We should build CryptarchiaConsensusState only with the provided prunable_blocks, which were already pruned from memory but not from storage.

Even after this change, the consensus service should work as before, because we anyway prune blocks right before rebuliding the CryptarchiaConsensusState. In other words, the previous implementation has worked well, but had an unnecessary call of fetching prunable blocks, which may cause side effects in the future.

(FYI, this PR is one of preparations for implementing chain bootstrapping.)

2. Does the code have enough context to be clearly understood?

Yes

3. Who are the specification authors and who is accountable for this PR?

  • Spec: The state recovery is not directly related to the Cryptarchia spec.
  • Code owners: @zeegomo @youngjoon-lee
    but adding @ntn-x2 @strinnityk as reviewers who have heavily contributed to the state recovery and pruning.

4. Is the specification accurate and complete?

Yes

5. Does the implementation introduce changes in the specification?

No

Checklist

Warning

Do not merge the PR if any of the following is missing:

  • 1. The PR title follows the Conventional Commits specification.
  • 2. Description added.
  • 3. Context and links to Specification document(s) added.
  • 4. Main contact(s) (developers and specification authors) added
  • 5. Implementation and Specification are 100% in sync including changes. This is critical.
  • 6. Link PR to a specific milestone.

@youngjoon-lee youngjoon-lee added this to the Iteration 13 milestone Jun 26, 2025
@youngjoon-lee youngjoon-lee self-assigned this Jun 26, 2025
@youngjoon-lee youngjoon-lee requested a review from zeegomo as a code owner June 26, 2025 10:18
ntn-x2
ntn-x2 reviewed Jun 26, 2025
View reviewed changes
Copy link
Contributor
@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I understand the change, but I think the function comment could be improved a bit to indicate that prunable_blocks refers to previously processed blocks and that we are not looking into the engine state at this point.

@youngjoon-lee youngjoon-lee mentioned this pull request Jun 27, 2025
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntn-x2 ntn-x2 ntn-x2 approved these changes

@strinnityk strinnityk Awaiting requested review from strinnityk

@zeegomo zeegomo Awaiting requested review from zeegomo zeegomo is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@youngjoon-lee youngjoon-lee

Labels
cryptarchia fix
Projects
None yet
Milestone
Iteration 13
Development

Successfully merging this pull request may close these issues.
2 participants
@youngjoon-lee @ntn-x2
0