-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Add CertificateChainSynchronizer
and make follower aggregators start their chain by synchronising with their leader
#2634
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
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.
Pull Request Overview
This PR implements a certificate chain synchronization mechanism so that follower aggregators can sync their chain from a leader rather than creating a genesis certificate.
- Introduces a
CertificateChainSynchronizer
service (and a no-op variant for leaders) - Refactors infrastructure startup to prepare and register leader/follower aggregators and serve them in sequence
- Updates the runtime state machine to validate and (if needed) synchronize the follower’s certificate chain before proceeding
Reviewed Changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
mithril-test-lab/mithril-end-to-end/src/run_only.rs | Simplify run flow to bootstrap the leader then serve followers |
mithril-test-lab/mithril-end-to-end/src/mithril/infrastructure.rs | Refactor aggregator startup to prepare leader/follower and register era |
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs | Add certificate chain synchronization logic |
mithril-aggregator/src/services/aggregator_client.rs | Extend HTTP client to fetch certificate lists and details |
mithril-aggregator/src/runtime/state_machine.rs | Validate and synchronize follower certificate chain in the runner |
Comments suppressed due to low confidence (1)
mithril-test-lab/mithril-end-to-end/src/mithril/infrastructure.rs:318
- The variable
leader_aggregator_endpoint
is not defined in this scope; you probably meant to useaggregator_endpoints[0]
or introduceleader_aggregator_endpoint
before this line.
aggregator_endpoint: &leader_aggregator_endpoint,
72b7d34
to
a46e42b
Compare
if the leader aggregator url was specified with a trailing slash, request to it would fails because joined url would have two slashes, i.e. `http://aggregator//route`. This align with the behavior of the aggregator client of the mithril-client lib.
… signer registration follower several benefits: - cleaner cut between signer registration business layer and its infrastructure - allow to use the concrete `AggregatorHTTPClient` type in dependency injection, allowing to implement multiple traits on it and still pass it to the multiple depenedencies that use that trait
…o a module In integration tests
…erver Instead of warp based `mithril-test-http-server`, as this allow to write simpler, more readable, code.
…in retrieval tests
Nothing is stored yet, this will come afterward
This allow implementor to use transactions if needed.
This is critical since rows are returned from the database by insertion order reversed.
…poch plus the genesis.
… entity only if not synchronized Has synchronised certificate are created without a signed entity, previouysly it was only the case for genesis certificates.
a46e42b
to
9bebed2
Compare
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
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.
LGTM 👍
I left some minor comments.
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
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.
LGTM 🚀
mithril-aggregator/src/services/certificate_chain_synchroniser/synchroniser_service.rs
Outdated
Show resolved
Hide resolved
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.
LGTM
- fix: synchronizer create the open messages based on the first certificate of the last epoch instead of the latest certificate - move `MockCertificateVerifier` definition to `tools::mocks` - fix spelling issues
… after sync This ensure that the synchronizer create an open message for MSD and that the state machine then proceed to the next signed entity type.
e05637f
to
d349f0b
Compare
* mithril-aggregator from `0.7.72` to `0.7.73` * mithril-common from `0.6.7` to `0.6.8` * mithril-end-to-end from `0.4.95` to `0.4.96`
d349f0b
to
11c8aec
Compare
Content
This PR add a certificate chain synchronization mechanism allowing follower Aggregators to avoid creating genesis certificate by, instead, synchronizing with their leader.
The synchronizer behaves as follows:
force
parameter (that allow the state machine to force a synchronization based on a condition, today it forces the sync if the follower chain is invalid), else the sync is done if either:CertificateVerifier
wired to the remote aggregator, it fetches the remote certificate chains and validate it at the same timeinsert or replace
to keep the data up to dateOpenMessage
forMithrilStakeDistribution
in the database so the aggregator won't create a certificate for this signed entityMain changes
CertificateChainSynchronizer
CertificateChainSynchronizer
: main trait, define the service apiRemoteCertificateRetriever
: define how a synchronizer get the latest certificate of the remote (to use it has the starting point of the synchronization) and the latest genesis of the remote (to check if it have changed, triggering a sync)SynchronizedCertificateStorer
: define how retrieved certificates are stored and how to retrieve the latest stored genesis certificate of the aggregatorOpenMessageStorer
: define how the open message created at the end of the process is storedMithrilCertificateChainSynchroniserNoop
: allow to construct a synchronizer in the leader aggregator dependency builderMithrilCertificateChainSynchronizer
: main implementationidle
toready
if the aggregator is a follower.InsertOrReplaceCertificateRecordQuery
andInsertOrReplaceOpenMessageQuery
AggregatorHTTPClient
: support three new queries,latest_certificates_list
,certificates_details
, andlatest_genesis_certificate
create_certificate_follower
integration tests: updated to check the synchronizer instead of spawning a genesiscertificate on the follower (this simplified the scenario since one less epoch is needed).
Additional changes
aggregator_url
with a trailing slash toAggregatorHTTPClient
would makes all its queries failsMithrilSignerRegistrationFollower
and theAggregatorHTTPClient
by moving and renaming theAggregatorClient
trait to it's api defined traits.AggregatorHTTPClient
directly as its concrete type in the dependencies builder, this storing it multiple times, one for each of the traits it implement.TestHttpServer
withaxum-test
as the backend for the leader aggregator http server.LeaderAggregatorHttpServer
to a dedicated moduleCertificateChainBuilder
): addlatest_certificate
to the fixturePre-submit checklist
Issue(s)
Relates to #2534