-
Notifications
You must be signed in to change notification settings - Fork 44
Handle multiple beacon types in Certificate #1601
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
Conversation
Test Results 3 files ±0 42 suites ±0 8m 41s ⏱️ - 1m 8s Results for commit 92d877a. ± Comparison against base commit 46c7b79. This pull request removes 15 and adds 11 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
34b3906
to
e7e1fb5
Compare
e7e1fb5
to
52a31df
Compare
As one can always be deduced from the certificate since a genesis certificate is equivalent to a MithrilStakeDistribution SignedEntityType. This means that bleeding edge signers won't be compatible with our until we update our aggregators.
This type will be used for state machines states as its role is to encompass all beacon values.
Node: the tests are still red.
Allowing it to know the network on which the aggregator operate.
Since they don't use the structure content it can be fully eluded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good 👍
I left few comments and minor typos to fix
drop table certificate; | ||
alter table new_certificate rename to certificate; | ||
|
||
CREATE INDEX epoch_index on certificate(epoch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an index on signed_entity_type_id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, we did no indexes on any of our foreign key columns whatever the table. If we changes stance on that maybe we should do that on a dedicated ticket ?
mithril-common/src/messages/message_parts/certificate_metadata.rs
Outdated
Show resolved
Hide resolved
As we only need to do epoch comparison now, no need for a complex structure anymore.
We don't need a complicated way to compute a CardanoDbBeacon anymore
As the concerns expressed here have been handled.
b9a97ac
to
f3228ad
Compare
…te_genesis_certificate` As later we will remove the immutable_file_number this will make the removal commit lighter.
f3228ad
to
a87ff74
Compare
Plus use the updated version for the added deprecated attributes.
Content
Warning
The hash computation of certificates is changed by this PR, we must run the certificate hash migrator on our aggregators after deployment.
This PR do the groundwork to allow multiple beacon types in our nodes.
Certificate
- replace theCardanoDbBeacon
with a more genericSignedEntityType
:SignedEntityType
if the certificate sign a multi-signatureepoch
field since it's crucial data of the certificate and the signed entity type may not have onebeacon
fieldnetwork
andimmutable_file_number
fields to theCertificateMetadata
(the later is for retro-compatibility only, it's used to reconstruct aCardanoDbBeacon
for certificate messages).epoch
andsigned_entity_type
fields plus anetwork
field to their metadata.beacon
fields as deprecated.TimePoint
CardanoDbBeacon
for state machines pacing: cardano db beacon can only pace signed data by immutable file number and/or an epoch,TimePoint
will be allow to pace all signed entity types.BeaconProvider
is replaced by aTimePointProvider
Pre-submit checklist
Comments
Also fix a minor issue of the fake aggregator data import script when it was run on a really young network (such as a run only e2e right after it is ready).
Issue(s)
Relates to #1562