feat: extract reusable member-traffic reconciliation trigger (Scala) - #15
Conversation
39ac0d2 to
c2a3d42
Compare
timwu20
left a comment
There was a problem hiding this comment.
A mismatch between the configured target and the sequencer the connection actually serves is now
Status.INTERNALrather than a skip.
The code ships the better version of this — FAILED_PRECONDITION with retryable semantics, so a connection mid-switchover recovers on its own (the in-code comment makes that argument well). Worth updating this paragraph to match so the merged description doesn't contradict the implementation.
4d4b9af to
36e365c
Compare
946c4a9 to
ce0b45a
Compare
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Thanks! This overall goes in the right direction but some of the checks are a bit redundant or misplaced. Also left some comments wrt to BFT syncs. I don't think that needs any code changes so more of an FYI•
…igger [ci] Move the reconciliation logic into an abstract trigger in apps/common, parameterized by the target synchronizer and its sequencer admin connection, so the Sync Operator Node can reconcile a dedicated synchronizer with the same code. The SV subclass keeps its name and package so its canonical name, metrics and paused-trigger key are unchanged. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
- report a target/connection mismatch as FAILED_PRECONDITION so it is retryable instead of dropping the purchase with an ERROR - warn once when the configured target is not served by the sequencer, which a per-contract check cannot see because every contract skips first - carry the skip reason through trafficLimitOffset instead of overloading Option - pin the target to a stable value and drop the redundant synchronizer id from the store hooks - skip members before opening the sequencer connection Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
… the sequencer [ci] The one-off target check was sequenced into the skip, so an unreachable sequencer could make skipping a foreign contract retry or fail. It is now best-effort, guarded by a compare-and-set so only one check runs at a time and it logs at most once, and it reuses a single helper for resolving what the sequencer serves. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
…ape [ci] The mismatch branch claimed to be reported by warnOnceIfTargetNotServed but never called it, so a wrong connection produced retry noise with no diagnosis. It already knows what the sequencer serves, so it now warns directly through a shared at-most-once helper. Splits the single settled flag in two: confirming the wiring must not consume the one warning a later mismatch is entitled to. Also sketches the BFT extension on the class docstring, as canton-network#31 asks. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
ce0b45a to
6a63721
Compare
Drops targetSynchronizerId and everything that existed to police it: both warn helpers, both flags and the target-vs-served mismatch branch. The foreign-contract skip now compares the contract against what the sequencer reports it serves, so there is no configured value left to disagree with. This restores the pre-extraction behaviour, where the synchronizer id was read from the connection per contract. The SV subclass keeps its own id for its store queries. Also restores the unparseable-synchronizer-id warning from #14, which a merge commit carried and the rebase onto the merged base dropped. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
@moritzkiefer-da updated the code |
Rebased onto
feat/dedicated-syncnow that #14 has merged. The reconciliation logic is SV-shaped today: it asksSynchronizerNodeServicefor its own sequencer, so there is no way to point it at another synchronizer. This factors the logic out so the Sync Operator Node can run it against a dedicated synchronizer. No behavior change for the SV.What this does
ReconcileSequencerLimitWithMemberTrafficTriggerBase(apps/common/automation), following the existingPackageVettingTriggerpattern: shared logic in an abstract trigger, app-specific dependencies as abstract defs (targetSynchronizerId,sequencerAdminConnection,getTotalPurchasedMemberTraffic,trafficLimitOffset).SvDsoStore,SynchronizerNodeServiceand the synchronizer id already held bySvDsoAutomationService.getStatusround trip.The SV subclass deliberately keeps its name and package.
identifyTriggerClassByNamereturnsgetCanonicalNameand that string keysautomationConfig.pausedTriggers, while metrics usegetSimpleNameastrigger_name. Moving the concrete class would silently change both, including in dashboards and cluster configs outside this repo. Only the reusable logic moves out ofsinglesv.One deliberate behavior change
A mismatch between the configured target and the sequencer the connection actually serves is now
Status.INTERNALrather than a skip. After this change those are different conditions: a contract naming another synchronizer is normal and still skips, but the configured target disagreeing with the connection would credit the wrong sequencer, so it should be loud rather than silently granting nothing. This cannot fire from an LSU switchover, which preserves the logical synchronizer id.How it's verified
apps-sv/compile,scalafmtCheck,headerCheckandscalafix --checkpass locally on bothapps-commonandapps-sv.No new tests:
apps/svholds 79 triggers and no trigger unit tests. Equivalence rests on the target being the same value the old code compared against, which holds by type (PhysicalSynchronizerId.logical : SynchronizerId) and becauseSvDsoAutomationServicereceives the decentralized synchronizer id.Tracked in
Implements E2-1 (ChainSafe/canton-extending-mainnet#31); unblocks E2-2 (#32).
Note for E2-2:
getTotalPurchasedMemberTrafficfilters onmigration_id, andMemberTrafficingestion filters onpayload.migrationId == domainMigrationId, so an operator node built on this path would inherit #60 and grant nothing on a network past migration 0. #16 avoids that by not matching on the node's own migration id, since a registered synchronizer is pinned to migration id 0.