feat(external-call): engine execution, model conformance and validation SPI by angelol · Pull Request #552 · digital-asset/canton

angelol · 2026-06-30T12:22:56Z

Summary

This is PR 4 of 8 in the external-call runtime-integration series tracked in #513, stacked on PR 3.

The external-call stack (#513): #506 added the transaction-side representation for recording external-call results; #514 added the LF EXTERNAL_CALL builtin surface; #518 wired it into Speedy and the LF engine; #522 added transaction protobuf encoding/decoding; #526 added Canton protocol serialization for recorded results; #537 added the participant-side extension-service client. #541 implemented the remaining runtime integration as one PR; this series splits #541 into 8 independently reviewable PRs (each < 1000 lines) and supersedes it.

This PR adds the engine-side replay of recorded external-call results and the validation SPI used during confirmation.

Scope

This PR adds:

DAMLe reinterpretation support for replaying recorded external-call results, with ExternalCallPayloadDescription
the ExternalCallValidator SPI trait and ExternalCallValidationError
ModelConformanceChecker integration that replays recorded results during conformance checking
engine, model-conformance and DAMLe tests

Out of scope

The concrete validator implementation (PR 6), the extension-service runtime (PR 5), consistency checking and routing (PR 6–7), and activation of validation in transaction processing (PR 8).

Stacking & review

PR 1–3 of the series have merged, and this branch is rebased onto main — the "Files changed" diff of this PR is now exactly its own incremental slice (no cumulative content from other PRs).

Refs #513.

paulbrauner-da

First batch of (harmless) comments. I have possibly more consequential comments queued, but I need to discuss them internally first.

matthiasS-da · 2026-07-01T12:41:16Z

Starting reviewing...

matthiasS-da · 2026-07-01T12:55:53Z

+      key: DAMLe.ExternalCallKey,
+      recordedOutput: Bytes,
+  )(implicit traceContext: TraceContext): FutureUnlessShutdown[ExternalCallValidator.Result]
+}


What's the reason of adding this code?

It's the SPI for validating a recorded external-call result against the extension service — same situation as the handler SPI in #551: the implementation arrives with the extension service (#553) and the consumers with the consistency checker and response router (#554/#555). It rides in this PR with the other external-call validation types.

matthiasS-da · 2026-07-01T13:53:42Z

+      outputsByKey.get(key) match {
+        case None => Right(None)
+        case Some(outputs) if outputs.sizeCompare(1) == 0 => Right(outputs.headOption)
+        case Some(outputs) => Left(ExternalCallRecordedResultDisagreement(key, outputs))


This is a very important validation step, but from my point of view the implementation is unnecessarily complex, as it is spread over ViewParticipantData, ModelConformanceChecker and DAMLe.
Have you considered putting this into TransactionView?

Fail TransactionView.validated if there are differing outputs for the same key.

Add a method to TransactionView that computes the ExternalCallReplayData.

Change ExternalCallReplayDate.outputsByKey to have the type Map[ExternalCallKey, LfBytes].

The question never posed itself, because this code was written against the full feature rather than this slice — and in the full series the validation you're pointing at deliberately lives elsewhere. ExternalCallConsistencyChecker (#554) detects conflicting recorded outputs structurally across all visible occurrences — including the occurrences and their checking parties — and ExternalCallResponseRouter (#555) turns exactly that into per-checking-party consistency rejections plus an alarm; #556 suppresses the replay-side duplicate so it isn't double-reported. Failing TransactionView.validated instead would reject the whole tree as malformed, dropping the per-party confirmation semantics and the occurrence granularity a key-level check can't carry. The check here is intentionally narrow: a self-contained guard that reinterpretation never proceeds on ambiguous inputs — which is also why outputsByKey keeps the Set; it's what lets replay detect the conflict without depending on the checker having run. So I'd keep this as is — happy to revisit after #554/#555 if you still see a better cut.

Ok, thanks for the explanation. We can defer the discussion until we reach the reviews for #554 and #555. Please note that I am uneasy with this approach; relying on logic in future unmerged PRs to justify current complexity is highly prone to causing architectural friction and unexpected delays, especially under such a tight timeline.

Understood — though one clarification that may ease this: the later PRs aren't future design work. The full feature exists as one implementation (this series is #541 sliced for reviewability), so the checker and router I'm referring to are finished code in #554/#555, open for inspection today.

Thanks again for the detailed breakdown, @angelol. Having now looked ahead and reviewed the entire trajectory of this design across PRs 5–8, I am still strongly in favor of centralizing this in TransactionView rather than scattering it across ModelConformanceChecker and DAMLe.

However, to avoid deadlocking your current review train, I am willing to defer this structural change to an explicit, post-merge architectural cleanup task.

For the record, and to frame that upcoming cleanup, here are the core technical reasons why the current layout introduces structural regressions:

Performance Bottleneck: It does not make sense to spend expensive cycles running transaction reinterpretation on a view that is guaranteed to be discarded anyway.

Fail-Last Complexity: Carrying an invalid state forward that is destined to fail introduces significant accidental complexity downstream. The need for PR feat(external-call): confirmation-responses factory and validation activation #556 to explicitly suppress duplicate rejections is a direct symptom of this "fail-last" design pattern, and reviewing this extra code to handle the split responsibility inherently consumes more review cycles as we go down the chain.

Regarding the specific downstream trade-offs you mentioned:

Failing TransactionView.validated instead would reject the whole tree as malformed, ...

Note that the protocol will ultimately reject the whole tree (recte: whole view) anyway, even with the downstream machinery introduced in PRs 5–8.

...dropping the per-party confirmation semantics and the occurrence granularity a key-level check can't carry.

I don't think the per-party confirmation semantics provide any practical advantage in this scenario. Every hosted confirming party can observe the disagreement; therefore, it is entirely sufficient to reject on behalf of every hosted confirming party rather than building a complex routing mechanism to isolate them.

Let's proceed with this slice as-is for now so you can move forward, on the strict condition that we open a tracked technical debt issue to refactor this validation logic back to a centralized, fail-fast model post-milestone.

matthiasS-da · 2026-07-01T14:13:38Z

+  /** Deterministic external-call identity. Config and input are engine-emitted canonical hex
+    * strings.
+    */
+  final case class ExternalCallKey(


This is an optional design improvement for code cleanliness and is not required for approval. Feel free to track this as a post-merge refactoring chore.

From an architectural standpoint, it looks like ExternalCallKey was introduced primarily to avoid typing out Map[(String, String, String, String), Set[LfBytes]] inside ExternalCallReplayData.

If that's the case, we could improve encapsulation in one of two ways:

Move ExternalCallKey directly into object ExternalCallReplayData as a private class (which would require a quick adjustment to the signature of ExternalCallReplayData.outputFor).

Alternatively, update ResultNeedExternalCall to take a parameter of type ExternalCallKey.

The key stays public because the consistency checker and response router later in the series (#554/#555) group and order by it, which rules out option 1; option 2 would change the engine's ResultNeedExternalCall API, which lives on the Daml side. Happy to track a post-merge tidy-up once the full series is in.

Please move this class to a separate file in com.digitalasset.canton.participant.protocol.validation.

Happy to track that as post-merge cleanup task.

matthiasS-da · 2026-07-01T14:18:35Z

@angelol to make the compilation pass, please force full recompilation as in #549.

matthiasS-da · 2026-07-01T14:39:02Z

@angelol from my point of view the following tests are needed:

End-to-end test with external calls
Test with disagreeing external call outputs (different views have different outputs for the same key)
Test with missing external call outputs (The engine request the output for a key. The key is not contained by any external call result in the request.)

For 1, I understand this is blocked as long as you don't have external calls in Daml.
What is your plan for 2 and 3?

matthiasS-da · 2026-07-01T14:56:08Z

I've concluded my first review pass:

Please address my detailed comments above.
If the compile job keeps failing, please apply the same trick as in feat(external-call): protocol and transaction-hash foundation #549.
Please rebase this PR as soon as feat(external-call): prepared-transaction codec for recorded results #550 and feat(external-call): command-execution errors, rejection and handler SPI #551 have been merged to main.

angelol · 2026-07-02T07:35:41Z

Applied the same full-recompilation trick as #549 in b4a740b, and addressed the detailed comments above (one commit each).

angelol · 2026-07-02T08:15:28Z

On 2 and 3:

3 (missing output) is covered in this PR: DAMLeExternalCallTest "reject external calls without recorded output" has the engine request a key with no recorded result and asserts the ExternalCallReplayMissing rejection; "not leak external-call payloads for replay errors" covers the same path's error hygiene.

2 (disagreeing outputs) has its replay-level half in this PR — "reject conflicting semantic outputs without leaking their payloads" (two recorded outputs for one key fail reinterpretation) — but the cross-view semantics you're describing are deliberately Phase-3 validation and are tested where they land later in the series: the consistency checker in #554 covers detection directly (e.g. "report only hosted parties that check conflicting outputs", "record visible disagreements without reporting non-hosted checking parties", "not report identical outputs"), and #555/#556 cover how those disagreements become confirmation responses (e.g. "route recorded external-call result disagreements by checking party", "split external-call disagreements from the general verdict by party", "alarm on visible recorded external-call disagreements without hosted confirmers").

For 1, the plan is an end-to-end prepare/execute test as soon as daml#23099 gives external calls a Daml-language surface — there's nothing to hang it on before that.

matthiasS-da · 2026-07-02T10:10:06Z

On 2 and 3:

3 (missing output) is covered in this PR: DAMLeExternalCallTest "reject external calls without recorded output" has the engine request a key with no recorded result and asserts the ExternalCallReplayMissing rejection; "not leak external-call payloads for replay errors" covers the same path's error hygiene.

2 (disagreeing outputs) has its replay-level half in this PR — "reject conflicting semantic outputs without leaking their payloads" (two recorded outputs for one key fail reinterpretation) — but the cross-view semantics you're describing are deliberately Phase-3 validation and are tested where they land later in the series: the consistency checker in #554 covers detection directly (e.g. "report only hosted parties that check conflicting outputs", "record visible disagreements without reporting non-hosted checking parties", "not report identical outputs"), and #555/#556 cover how those disagreements become confirmation responses (e.g. "route recorded external-call result disagreements by checking party", "split external-call disagreements from the general verdict by party", "alarm on visible recorded external-call disagreements without hosted confirmers").

For 1, the plan is an end-to-end prepare/execute test as soon as daml#23099 gives external calls a Daml-language surface — there's nothing to hang it on before that.

Thanks for explaining. I'm aware of the unit tests in DAMLeExternalCallTest. I intended to refer to end-to-end integration tests, where a malicious submitting participant produces these situations. What's the plan for that?

angelol · 2026-07-02T12:09:49Z

For 2 and 3: both need the confirming participant to reinterpret a transaction that genuinely contains external-call nodes — a malicious submitter doesn't change that. Without a real external-call node behind the recorded results, the honest side rejects on model conformance rather than the external-call path, so the disagreement / missing-output logic never runs. So these need an external-call choice in a Daml package, same as the honest e2e (1).

Two options:

Land them with the external-call Daml surface (daml#23099), against a compiled package.
Now, without it: the LF builtin exists in dev-LF, so I could hand-author a raw-LF package with an external-call choice (in-memory via the LF test parser) and exercise it through the ledger API. Caveats: I found no precedent for exercising a hand-authored package at the integration level (execution tests all use compiled DARs), it'd be pinned to dev-LF, and presumably replaced once (1) lands.

The harness is otherwise ready, so either way it's a small delta. Interim coverage is at the component level: ExternalCallConsistencyCheckerTest, TransactionConfirmationResponsesFactoryExternalCallTest, DAMLeExternalCallTest.

matthiasS-da · 2026-07-03T07:47:11Z

For 2 and 3: both need the confirming participant to reinterpret a transaction that genuinely contains external-call nodes — a malicious submitter doesn't change that. Without a real external-call node behind the recorded results, the honest side rejects on model conformance rather than the external-call path, so the disagreement / missing-output logic never runs. So these need an external-call choice in a Daml package, same as the honest e2e (1).

Two options:

Land them with the external-call Daml surface (daml#23099), against a compiled package.

Now, without it: the LF builtin exists in dev-LF, so I could hand-author a raw-LF package with an external-call choice (in-memory via the LF test parser) and exercise it through the ledger API. Caveats: I found no precedent for exercising a hand-authored package at the integration level (execution tests all use compiled DARs), it'd be pinned to dev-LF, and presumably replaced once (1) lands.

The harness is otherwise ready, so either way it's a small delta. Interim coverage is at the component level: ExternalCallConsistencyCheckerTest, TransactionConfirmationResponsesFactoryExternalCallTest, DAMLeExternalCallTest.

Ok, let's defer tests 1-3 to a later PR.

matthiasS-da · 2026-07-03T08:58:20Z

@angelol #551 has been merged. Please rebase this one!

paulbrauner-da · 2026-07-03T14:23:38Z

Caveats: I found no precedent for exercising a hand-authored package at the integration level (execution tests all use compiled DARs), it'd be pinned to dev-LF, and presumably replaced once (1) lands.

We actually have such an integration test. The dars are produced here from this LF code. So it's not unheard of.

…validation SPI

…ining The inlining was incidental and unrelated to the external-call feature; reverting it keeps the PR diff focused.

The test feeds two different outputs for one input identity, so "conflicting" (not "duplicate") is accurate; the new name also matches its non-leakage assertions rather than "without choosing one".

…e results type ExternalCallReplayData was a redundant single-field wrapper around StoredExternalCallResults, which held all the logic. Fold that logic into ExternalCallReplayData and delete the separate results type; behavior unchanged.

…ay-data thunk The reinterpret SPI's externalCallReplayData thunk returned FutureUnlessShutdown[ExternalCallReplayData], but every producer wrapped a synchronous in-memory value in FutureUnlessShutdown.pure -- there is no async source. Make the thunk () => ExternalCallReplayData and unwrap the future in handleExternalCall accordingly; behavior unchanged.

… aggregation ViewParticipantData.validated already rejects non-empty externalCallResults below protocol version dev on every construction path, so the supports-guard was dead; fromResults(empty) equals empty, so the isEmpty short-circuit was too. Drop both and the now-unused viewParticipantData parameter.

ExternalCallKey implements PrettyPrinting (payload sizes only, never values), so both replay-error prettys collapse to param("key", _.key). Also closes the payload-leak channel through the key's default case-class toString.

resumeWithExternalCallOutput and replayStoredExternalCallOutput each had one caller and formed a single chain; fold both into handleExternalCall.

The replayDataO parameter cached the externalCallReplayData thunk across external calls within one reinterpretation, but the only production caller already passes a lazy-val-backed thunk, so the internal memoization was redundant. handleExternalCall now calls the thunk directly.

Replace the hardcoded dev-PV factory with the suite factory and testedProtocolVersion, gated onlyRunWithOrGreaterThan(dev) like the sibling tampered-metadata test, so the test no longer re-runs identically under every protocol version. Add a counterpart below dev asserting the aggregated replay data stays empty. Fold buildUnderTestWithFactory into its now-single caller.

Left behind by the earlier type-shortening rename; caught by Test/scalafmtCheck, which the Compile-scoped check used before misses.

Follow-up to the earlier helper inlining: with those gone, this two-use wrapper no longer pays for itself either.

angelol · 2026-07-03T15:50:03Z

Rebased onto latest main (post-#551), along with the rest of the stack — the diff is now the clean PR-4 slice. Local gates are green (whole-repo test-compile + the dev-PV suites for this PR's surface).

angelol · 2026-07-03T15:50:10Z

Thanks — I'd missed UpgradesMatrixIT, that removes the no-precedent caveat. When we pick up the deferred tests, producing the DAR from hand-authored LF per that pattern looks like the right route.

No longer needed per review feedback on digital-asset#551.

angelol · 2026-07-03T18:31:32Z

Following up on #551 (comment) (#551 is locked): the force-recompile file is removed here in 8d61347 — this PR's diff is now the 8 content files only. The equivalent files on PR 5–8 will be dropped as each is rebased.

The full-recompilation mechanism is retired (see digital-asset#551 review discussion r3518217233 and its removal on digital-asset#552); mainline no longer carries the file.

matthiasS-da · 2026-07-06T07:11:40Z

Resuming review...

matthiasS-da

Approving with the caveats outlined in the detailed comments.

Most importantly:

The additional complexity of processing views with result disagreements should be removed.
Missing end-to-end tests need to be implement.

matthiasS-da · 2026-07-07T11:27:04Z

@angelol from my point of view the following tests are needed:

End-to-end test with external calls

Test with disagreeing external call outputs (different views have different outputs for the same key)

Test with missing external call outputs (The engine request the output for a key. The key is not contained by any external call result in the request.)

For 1, I understand this is blocked as long as you don't have external calls in Daml. What is your plan for 2 and 3?

Missing end-to-end tests are tracked in #564.

angelol requested review from rgugliel-da and soren-da as code owners June 30, 2026 12:22

angelol force-pushed the angelol/external-call-06-split-pr4-damle-mcc branch from bcd36d4 to 1c261f2 Compare June 30, 2026 14:33

paulbrauner-da reviewed Jun 30, 2026

View reviewed changes

angelol force-pushed the angelol/external-call-06-split-pr4-damle-mcc branch from 1c261f2 to 7dc5bcb Compare June 30, 2026 16:31

paulbrauner-da reviewed Jun 30, 2026

View reviewed changes

Comment thread community/participant/src/main/scala/com/digitalasset/canton/participant/util/DAMLe.scala Outdated

matthiasS-da self-requested a review July 1, 2026 12:40

matthiasS-da self-assigned this Jul 1, 2026