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

[Consensus] Core to read last proposed round from DagState #20173

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

akichidis
Copy link
Contributor

@akichidis akichidis commented Nov 4, 2024

Description

Refactoring Core to read the last proposed block from DagState rather than local cached value. This change:

  • will allow for nodes that recover from amnesia (potentially) link their new proposed block the actual last proposed block instead of linking back to genesis.
  • because of the above point, we are also avoiding edge case error when an amnesia recovered node is attempting to propose for a round with exact quorum, although their last proposed block still pointing to genesis, practically making us (thankfully) hit the assert here
  • our last proposed block is cached in DagState anyways so no practical difference

Test plan

CI


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • REST API:

Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 5, 2024 5:33pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 5, 2024 5:33pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 5, 2024 5:33pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Nov 5, 2024 5:33pm

@akichidis akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 4, 2024 23:20 — with GitHub Actions Inactive
Copy link
Member

@mwtian mwtian left a comment

Choose a reason for hiding this comment

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

I guess we will need to rebase ancestor selection on this PR?

});
assert!(our_ancestor_included.is_some());
let our_ancestor_included = block
.ancestors()
Copy link
Member

Choose a reason for hiding this comment

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

Blocks proposed by Core should have the 1st ancestor as own ancestor.

@akichidis
Copy link
Contributor Author

I guess we will need to rebase ancestor selection on this PR?

Yes. If it's going to be a hassle @arun-koshy I can hold of until you merge, although the changes are quite small.

@arun-koshy
Copy link
Contributor

Yes. If it's going to be a hassle @arun-koshy I can hold of until you merge, although the changes are quite small.

Either way is fine with me! Feel free to merge and I can rebase as I am still waiting for approvals.

@akichidis akichidis merged commit 5c97430 into main Nov 6, 2024
52 checks passed
@akichidis akichidis deleted the akichidis/last-proposed-block branch November 6, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants