fix: skip unparseable and foreign sync ids in traffic triggers (Scala) - #14
Merged
Merged
Conversation
sadiq1971
force-pushed
the
fix/sync-id-parse-hardening
branch
2 times, most recently
from
August 10, 2026 17:20
c49a6da to
55ae2f4
Compare
sadiq1971
changed the title
sadiq1971
changed the title
sadiq1971
added a commit
that referenced
this pull request
Aug 13, 2026
Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
sadiq1971
added a commit
that referenced
this pull request
Aug 14, 2026
Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
timwu20
approved these changes
Aug 19, 2026
timwu20
left a comment
timwu20
left a comment
Collaborator
There was a problem hiding this comment.
One pre-merge mechanical: the branch predates the #8/#12 squashes, so it lacks the Flyway ignore-pattern fix that rode in with #12 — the two red checks are the Aug-13 known flakes (simtime clog gate with sbt green underneath), but any fresh run on this tree will fail ~7 jobs on the Postgres-18.6 pattern. Suggest rebasing onto current feat/dedicated-sync (5186e84) and letting [ci] rerun; expect green, then this is merge-ready. (#15 stacks on this branch, so it rebases in the same pass.)
…ers [ci] Traffic can be purchased for any registered synchronizer, so an SV sequencer observes MemberTraffic contracts for synchronizers it does not serve. Parse failures and foreign synchronizer ids now skip the contract instead of failing the trigger. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
A foreign synchronizer id is routine once dedicated synchronizers exist, but an unparseable one means corrupt data, so it is logged as a warning as well as skipped. Matches the acceptance criterion in canton-extending-mainnet#33. Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
sadiq1971
force-pushed
the
fix/sync-id-parse-hardening
branch
from
August 19, 2026 23:00
4d4b9af to
36e365c
Compare
moritzkiefer-da
approved these changes
Aug 21, 2026
moritzkiefer-da
left a comment
moritzkiefer-da
left a comment
There was a problem hiding this comment.
thanks, can we add a test for this? I would just bare create a MemberTraffic contract for that.
| err => { | ||
| // Unlike a foreign synchronizer id, an unparseable one means corrupt data and | ||
| // should never be routine, so it is worth an alarm as well as a skip. | ||
| logger.warn(s"Skipping MemberTraffic with unparseable synchronizerId: ${err}") |
There was a problem hiding this comment.
Can we just avoid parsing the synchronizer id in the first place? I don't really see why we shouldn't merge for an unmergeable sync id.
1 task
sadiq1971
added a commit
that referenced
this pull request
Aug 24, 2026
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>
sadiq1971
added a commit
that referenced
this pull request
Aug 25, 2026
…15) * refactor(scala-sv): extract reusable member-traffic reconciliation trigger [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> * refactor(scala-sv): address review on the reusable traffic trigger [ci] - 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> * refactor(scala-sv): keep the traffic trigger skip path independent of 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> * address review: warn on the miswired-connection path, note the BFT shape [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> * address review: take the synchronizer id from the sequencer [ci] 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> --------- Signed-off-by: sadiq1971 <sadiqurr8@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two SV traffic triggers treat a
MemberTrafficcontract they cannot act on as a fault. Traffic can be purchased for any registered synchronizer, so an SV's sequencer legitimately observes contracts naming synchronizers it does not serve. Scala side only, no Daml or config changes.What this does
SynchronizerId.tryFromString->fromString+foldin both triggers. The throwing variant fails the trigger and leaves it retrying the same contract; theEitherform skips it with aTaskSuccess, matching howmemberIdis already handled directly above in both files.ReconcileSequencerLimitWithMemberTrafficTriggerno longer raisesStatus.INTERNALwhen the contract names a different synchronizer. Those grants are applied by that synchronizer's own operator on its own sequencer, so this node skips them rather than failing.MergeMemberTrafficContractsTriggerparses ahead of theforcomprehension, dropping agetDsoRules()round trip on ids that are skipped anyway.How it's verified
apps-sv/compile,scalafmtCheck,headerCheckandscalafix --checkall pass locally.No new tests:
apps/svholds 79 triggers and no trigger unit tests, so this follows the existing pattern.Both skip paths land in the Appendix B acceptance e2e (register, buy, then reconcile across two synchronizers), which needs the two-synchronizer topology the foreign path requires anyway. Naming it here so the debt is tracked rather than implicit.
One forward-looking note for whoever writes that test: the case that feeds an unparseable synchronizer id will trip the CI log gate on the
logger.warnadded in 4d4b9af, and needs an ignore-pattern entry alongside the test.Tracked in
Implements E2-3 (ChainSafe/canton-extending-mainnet#33).