feat: extract reusable member-traffic reconciliation trigger (Scala) - #15
feat: extract reusable member-traffic reconciliation trigger (Scala)#15sadiq1971 wants to merge 4 commits into
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.
| // Granting here would credit a sequencer of a different synchronizer. Reported as | ||
| // retryable so that a connection that is still switching over recovers on its own | ||
| // instead of dropping the purchase. A target that is permanently wrong therefore | ||
| // retries rather than failing once, and is reported by warnOnceIfTargetNotServed. |
There was a problem hiding this comment.
This comment says the permanently-wrong case "is reported by warnOnceIfTargetNotServed", but this branch never calls it — the warn-once only fires from the foreign-contract skip path. The two miswiring shapes therefore diverge: a wrong target makes every contract look foreign and the warn fires; a wrong connection (contracts match the target) lands here every task and produces only retry noise, no one-line diagnosis. Since warnOnceIfTargetNotServed is already CAS-guarded and best-effort, invoking it from this branch too is a one-liner that makes the comment true. (Same point promised from #14's tripwire thread.)
There was a problem hiding this comment.
Fixed in 946c4a9. You were right that the comment was false. Rather than call warnOnceIfTargetNotServed() here, which would re-fetch the status this branch already has, I split out a shared at-most-once warnOnceTargetNotServed(served) and call it directly before failing.
One thing that fell out while doing it: the single targetCheckSettled flag was conflating "already warned" with "confirmed correctly wired", so a successful early check would have consumed the one warning a later mismatch is entitled to. Now two flags, targetMismatchWarned and targetConfirmed.
There was a problem hiding this comment.
Your fix is better than what I suggested, for two reasons. Reusing the sync id the code already has in hand beats asking the sequencer again. And splitting the flag in two fixes a bug my version would have had: once the wiring check passed, the flag would already be used up, so a later mismatch would never get its warning.
| * naming synchronizers it does not serve. Each instance of this trigger owns exactly one | ||
| * synchronizer, given by [[targetSynchronizerId]], and skips everything else; the operator of | ||
| * another synchronizer grants those purchases on its own sequencer. | ||
| * |
There was a problem hiding this comment.
canton-network#31 asks that the shape "not hard-wire a single sequencer (keep BFT open)". The one-admin-connection-per-reconcile shape is right for MVP, but a BFT operator will need to reconcile the limit on every sequencer of the synchronizer. One sentence here sketching the intended extension (the connection hook returns a set, reconcile loops) keeps that door visibly open rather than implicitly closed.
There was a problem hiding this comment.
Added to the class docstring in 946c4a9: one instance reconciles one sequencer, a BFT operator runs one instance per sequencer each with its own sequencerAdminConnection, and nothing here assumes the single-sequencer case beyond that hook returning one connection.
Worth recording why it has to be per-sequencer rather than one instance looping: a traffic grant is submitted with an AggregationRule over the active sequencers and only commits once the group threshold is met (TrafficPurchasedSubmissionHandler), so the grant genuinely needs threshold-many operators each submitting through their own sequencer.
There was a problem hiding this comment.
One trigger per sequencer is simpler than what I had in mind (one trigger looping over all of them) — each instance gets its own connection and nothing is shared. The docstring now covers what canton-network#31 asked for.
4d4b9af to
36e365c
Compare
…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 <[email protected]>
- 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 <[email protected]>
… 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 <[email protected]>
…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 <[email protected]>
946c4a9 to
ce0b45a
Compare
Stacked on #14. The reconciliation logic is SV-shaped today: it asks
SynchronizerNodeServicefor 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.