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

Include block header in leaf commitment #3647

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Conversation

ss-es
Copy link
Contributor

@ss-es ss-es commented Sep 4, 2024

This PR:

  • Adds a commitmethod to the inherent impl for Leaf. Note that this method is named intentionally to conflict with commit(), from the Committable implementation for Leaf defined in the same module. This means that the compiler will reject an attempt to write leaf.commit(), forcing you to write <Leaf<TYPES> as Committable>::commit() to refer to the old commitment. Additionally, the newer commit is an async function requiring an extra argument, which should make it virtually impossible to mix up.
  • Propagates upgrade_lock to every place the leaf commitment was used.
  • QuorumCertificate::genesis now requires a version parameter, since the leaf commitment it uses now depends on the version.

This PR does not:

  • No extra integration tests are added for this, because we expect test_success_marketplace to fail if we missed something in the leaf commitment calculation.

Key places to review:

  • The main change is in types/src/data.rs -- please triple-check the new commitment carefully. All other changes are a consequence of that change.

crates/types/src/data.rs Outdated Show resolved Hide resolved
@ss-es ss-es marked this pull request as ready for review September 5, 2024 17:00
@ss-es ss-es assigned bfish713 and unassigned jparr721 and lukeiannucci Sep 5, 2024
@ss-es ss-es merged commit a6f1177 into main Sep 10, 2024
33 of 36 checks passed
@ss-es ss-es deleted the ss/block-header-leaf-commit branch September 10, 2024 11:16
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.

4 participants