feat(external-call): extension-service handler, config and command-execution wiring by angelol · Pull Request #553 · digital-asset/canton

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

Summary

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

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 connects the #537 extension-service client to command execution: it adds the handler that routes engine external-call questions to configured extension services, the participant configuration that keys services by extension id, and the node wiring.

Scope

This PR adds:

the ExtensionServiceExternalCallHandler implementation of the PR 3 handler SPI, dispatching engine external-call questions to the feat(external-call): add extension service client support #537 extension-service client (ExtensionServiceManager / HttpExtensionServiceClient / ExtensionServiceClient glue)
participant extension-service configuration keyed by extension id (CantonConfig, ConfigValidations, CantonEngineConfig)
handler wiring through command execution (ApiServiceOwner, ApiServices, StoreBackedCommandInterpreter) and node startup (LedgerApiServer, ParticipantNode)
configuration, HTTP-handler and command-interpreter tests

Out of scope

Recorded-result consistency checking and the extension-service validator implementation (PR 6), response routing (PR 7), and activation of validation in transaction processing (PR 8). The handler is dev-gated and off unless an extension service is configured, so the intermediate state is dormant.

Stacking & review

This PR is stacked on PR 4, so its diff against main is cumulative. The incremental change for this PR alone (it depends on PR 3's handler SPI, not PR 4):
zenith-network:angelol/external-call-06-split-pr4-damle-mcc...zenith-network:angelol/external-call-06-split-pr5-extension

Refs #513.

matthiasS-da · 2026-07-01T14:57:42Z

Start reviewing...

matthiasS-da · 2026-07-02T08:18:52Z

+      )
+      .map(_.left.map { extensionError =>
+        ResultNeedExternalCall.Error(sanitizedEngineError(extensionError))
+      })


I need to get back to this sanitization here: In #541, you pointed out that this is at a trust boundary, the HttpExtensionServiceClient will log extensionErrors anyway, and low-level error details should not be distributed via confirmation responses.
However, after re-reviewing this, I observe:

While I see that HttpExtensionServiceClient logs errors coming from the service responses, there are other ExtensionCallErrors that seem to not being logged anywhere. (E.g. the error that is directly produced by ExtensionServiceManager.handleExternalCall.) It is important that the full error gets logged prior to any sanitization.

I agree that ExtensionCallError.message potentially contains low-level error information and should therefore not be forwarded to other nodes.

By feeding extensionError into ResultNeedExternalCall.resume, it is potentially forwarded to ledger api clients.

As this is called only by StoreBackedCommandInterpreter, errors do not get distributed via confirmation responses.

Please let me know, if you agree with my observations. If so:

Please make sure that the full ExtensionCallError gets logged in any case. I recommend to log just before sanitization, instead of logging somewhere else.

To prevent information leakage, some sanitization is needed. I'm afraid the current approach is too conservative, as ledger api clients only get a status code. Shall we agree on a post-merge cleanup task to figure out the details?

Your observations are all correct. On 1, there were actually three unlogged branches: the manager's unconfigured-404 that you found, plus the client's invalid-header-value and invalid-auth-config errors, which also returned without logging.

Fixed in a4ea076. The full error — status, retryable, requestId, message — is now logged once in ExtensionServiceManager.handleExternalCall. That's one level below where you suggested, deliberately: #554 introduces a second consumer of the same manager method (the Phase-3 validator, which sanitizes separately), so the manager is the last point where one log statement covers every error-producing branch for both consumers. It also sits after the client's internal retries, so the full error is logged exactly once per failed call; the client's per-attempt logs remain for retry diagnostics.

One addition to observation 4 from the later PRs: nothing on this path reaches confirmation responses, as you say — but the #554 validator's sanitized reason (status code + requestId) does surface in LocalAbstain confirmation responses. That's the path the conservative sanitization was originally written for.

On the cleanup task: agreed. The ledger-api-client-facing message can likely carry more than a status code; the confirmation-response-facing side should stay minimal. Happy to work out the details post-merge.

Thanks for the additional logging. Let's revisit the sanitization post-merge.

matthiasS-da · 2026-07-02T12:59:11Z

I've concluded my first review pass:

Please address my detailed comments above.
Please rebase this PR as soon as feat(external-call): prepared-transaction codec for recorded results #550, feat(external-call): command-execution errors, rejection and handler SPI #551, and feat(external-call): engine execution, model conformance and validation SPI #552 have been merged to main.

matthiasS-da · 2026-07-06T08:54:44Z

@angelol please rebase this PR on HEAD of main so I can finalize the review!

…ll handler wiring

The three engine-extension validation constants each had one use site; inline them so the checks read without jumping around. Messages unchanged.

…tion Some ExtensionCallErrors were never logged (the manager's unconfigured-404 and the client's invalid-header and invalid-auth-config errors). Log the full error once in ExtensionServiceManager.handleExternalCall — the last common point before consumers sanitize the message away. The client's per-attempt logs stay for retry diagnostics, so the exception path now warns twice (per attempt + final outcome).

Extract vDevCreateTransaction and prepare helpers for the transaction-build and processPrepare boilerplate repeated across the four test cases.

The rejection is triggered by the stable protocol version, not the transaction, so name it accordingly.

…tionProcessorSpec Use shouldBe on the complete reason string instead of should-include, for both the VDev-with-V2 and stable-PV-with-V4 rejection cases.

angelol · 2026-07-06T09:25:41Z

Rebased onto main HEAD (e3c82a3, post-#552) — new tip b213120. The six content commits replayed byte-identically (range-diff all =), and the retired CI force-recompile commit is dropped per the PR 4 discussion, so the diff against main is now the clean PR 5 slice. Local gates green (whole-repo scalafix, ExternalTransactionProcessorSpec 4/4 at dev PV, CantonConfigTest 48/48).

matthiasS-da · 2026-07-06T09:36:05Z

Resuming review...

matthiasS-da · 2026-07-06T10:01:52Z

+    val errors = engineExtensionErrors(config) { case (name, extensionId, extensionConfig) =>
+      Option.when(extensionConfig.port == Port.Dynamic)(
+        s"For participant ${name.unwrap}, engine.extensions.$extensionId.port must be " +
+          s"between 1 and ${Port.maxValidPort}, but found '${extensionConfig.port}'"


MINOR: This error message breaks if the constant Port.Dynamic ever changes.
It's fine for me to handle this as a post-merge cleanup.

matthiasS-da · 2026-07-06T10:03:32Z

+        extensionConfig.retryInitialDelay.underlying > extensionConfig.retryMaxDelay.underlying
+      )(
+        s"For participant ${name.unwrap}, engine.extensions.$extensionId retry delays must satisfy retry-initial-delay <= retry-max-delay; " +
+          s"respective values are (${extensionConfig.retryInitialDelay.underlying}, ${extensionConfig.retryMaxDelay.underlying})"


MINOR: Need to fix the grammatical structure (currently: "respective values are (x, y)" to "respective values are x and y."
It's fine for me to handle this as a post-merge cleanup.

matthiasS-da · 2026-07-06T10:06:50Z

+    result.map(_.tapLeft { error =>
+      val requestId = error.requestId.fold("")(id => s", requestId=$id")
+      logger.warn(
+        s"External call to extension '$extensionId' (function '$functionId') failed: status=${error.statusCode}, retryable=${error.retryable}$requestId, message=${error.message}"


MINOR: Please change ExtensionCallError to implement PrettyPrinting instead of accessing the fields one by one. This minimizes the risk of forgetting to render newly added fields.

It's fine for me to address that as a post-merge cleanup task.

matthiasS-da · 2026-07-06T10:40:03Z

+      transaction: VersionedTransaction,
+      protocolVersion: ProtocolVersion,
+      submissionSeed: String,
+      nodeSeeds: ImmArray[(NodeId, LfHash)],


MINOR: To avoid unnecessary code the parameter list should be simplified:

protocolVersion: hardcode to testedProtocolVersion and proceed as outlined here: feat(external-call): engine execution, model conformance and validation SPI #552 (comment)

submissionSeed: choose a dummy value

nodeSeeds: only pass ids of nodes being included, generate the actual seeds here

It's fine for me if you do that as a post-merge cleanup.

matthiasS-da · 2026-07-06T10:40:53Z

+      submissionSeed: String,
+      hashingSchemeVersion: HashingSchemeVersion,
+      nodeSeeds: ImmArray[(NodeId, LfHash)] = ImmArray.Empty,
+  ) = {


MINOR: Please add type annotations for top-level declarations.

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

+        "prepared-vdev-requested-v2-test",
+        HashingSchemeVersion.V2,
+        ImmArray(nodeId -> LfHash.hashPrivateKey("prepared-vdev-requested-v2-node")),
+      ).value.failOnShutdown("prepare transaction").futureValue


MINOR: please move the .value.failOnShutdown("prepare transaction").futureValue into prepare.

matthiasS-da · 2026-07-06T10:48:36Z

For the record, after merging this PR, integration tests should be added that exercises failure cases of ExternalCallHandler and HttpExtensionServiceClient. These tests should check failures reported to ledger api clients as well as log messages.

matthiasS-da

Approving, but some clean-up tasks need to be done post merge:

Revisit of error sanitization by ExtensionServiceExternalCallHandler.
Add negative test cases: #553 (comment)
Minor changes requested in the detailed comments.

matthiasS-da · 2026-07-07T11:35:31Z

Approving, but some clean-up tasks need to be done post merge:

Revisit of error sanitization by ExtensionServiceExternalCallHandler.

Add negative test cases: #553 (comment)

Minor changes requested in the detailed comments.

Tracked in #564

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

angelol force-pushed the angelol/external-call-06-split-pr5-extension branch 2 times, most recently from 82a25ba to 3cc8b8d Compare June 30, 2026 16:31

matthiasS-da self-requested a review July 1, 2026 14:56

matthiasS-da self-assigned this Jul 1, 2026

angelol force-pushed the angelol/external-call-06-split-pr5-extension branch from b6e9db6 to 8d9554c Compare July 1, 2026 19:27