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

Remove beacon from certificate #2055

Merged
merged 12 commits into from
Oct 28, 2024
Merged

Conversation

Alenar
Copy link
Collaborator

@Alenar Alenar commented Oct 28, 2024

Content

This PR remove the beacon field from the CertificateMessage and CertificateListMessage as this field was supersede by the signed_entity_type field, field that is mandatory since distribution 2418.1.

This allow to remove the immutable_file_number plus the conversion to cardano db beacon on the CertificateMetadata entity and the CertificateRecord, those only existed to recover the beacon when building a message.

Warning

Nodes from distribution below 2430 are not compatible with this change.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Comments

This changes should not induce a recompute of the certificates hashes as the immutable file number was not used in the certificate hash computation (the epoch and the network are still used).

Issue(s)

Closes #1958

@Alenar Alenar self-assigned this Oct 28, 2024
Copy link

github-actions bot commented Oct 28, 2024

Test Results

    4 files  ±0     51 suites  ±0   10m 41s ⏱️ +15s
1 398 tests  - 2  1 398 ✅  - 2  0 💤 ±0  0 ❌ ±0 
1 609 runs   - 2  1 609 ✅  - 2  0 💤 ±0  0 ❌ ±0 

Results for commit 22dc14f. ± Comparison against base commit 650e77d.

This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.
mithril-common ‑ messages::certificate::tests::test_actual_json_deserialized_into_previous_message
mithril-common ‑ messages::certificate::tests::test_json_next_version_deserialized_into_actual_message
mithril-common ‑ messages::certificate_list::tests::test_actual_json_deserialized_into_previous_message
mithril-common ‑ messages::certificate_list::tests::test_json_next_version_deserialized_into_actual_message
mithril-common ‑ messages::certificate::tests::test_actual_json_deserialized_into_message_supported_until_open_api_0_1_32
mithril-common ‑ messages::certificate_list::tests::test_actual_json_deserialized_into_message_supported_until_open_api_0_1_32

♻️ This comment has been updated with latest results.

@Alenar Alenar force-pushed the djo/1958/remove_beacon_from_certificate branch from 9b841f4 to ef31341 Compare October 28, 2024 13:33
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

LGTM

@Alenar Alenar force-pushed the djo/1958/remove_beacon_from_certificate branch from e9a0655 to f2a48f4 Compare October 28, 2024 14:21
Copy link
Collaborator

@dlachaume dlachaume left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

mithril-common/src/messages/certificate_list.rs Outdated Show resolved Hide resolved
* mithril-aggregator from `0.5.91` to `0.5.92`
* mithril-client from `0.9.4` to `0.10.0`
* mithril-client-cli from `0.9.18` to `0.10.0`
* mithril-client-wasm from `0.5.3` to `0.6.0`
* mithril-common from `0.4.75` to `0.4.76`
* mithril-aggregator-fake from `0.3.10` to `0.3.11`
* openapi.yaml from `0.1.32` to `0.1.33`
@Alenar Alenar force-pushed the djo/1958/remove_beacon_from_certificate branch from f2a48f4 to 22dc14f Compare October 28, 2024 14:36
@Alenar Alenar merged commit 5d38bce into main Oct 28, 2024
50 checks passed
@Alenar Alenar deleted the djo/1958/remove_beacon_from_certificate branch October 28, 2024 14:49
@Alenar Alenar mentioned this pull request Oct 28, 2024
7 tasks
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.

Remove beacon field in certificate
4 participants