CRE Don2Don accept OCR attestation of a response#21607
CRE Don2Don accept OCR attestation of a response#21607dhaidashenko wants to merge 8 commits intodevelopfrom
Conversation
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
4a361c5 to
8c09813
Compare
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
2 times, most recently
from
2bbab2b to
ec482f1
Compare
| rpt := resp.Metadata.Metering[0] | ||
| rpt.Peer2PeerID = sender.String() | ||
| var payload []byte | ||
| payload, err = c.encodePayloadWithMetadata(msg, commoncap.ResponseMetadata{Metering: []commoncap.MeteringNodeDetail{rpt}}) |
There was a problem hiding this comment.
could benefit from a library on the client side that crafts resp.Metadata.Metering[0] in the way we expect here
There was a problem hiding this comment.
I've consolidated logic for interacting with metering details in the client_request.
If you meant a method to craft resp.Metadata.Metering[0] on the capabilities side, I agree that it's beneficial. However, it's out of scope for this PR.
There was a problem hiding this comment.
If you meant a method to craft resp.Metadata.Metering[0] on the capabilities side, I agree that it's beneficial.
I do yes.
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
2 times, most recently
from
ffd661b to
0d17383
Compare
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH (changes remote executable request aggregation/validation logic and adds a new OCR-attestation fast-path)
This PR updates the remote executable capability client to accept and verify OCR-attested responses (rather than requiring a quorum of identical responses), and bumps chainlink-common / keystore dependencies across multiple modules to pick up the needed functionality.
Changes:
- Add OCR attestation verification for capability responses in the remote executable client request path.
- Plumb OCR3 configs through launcher → executable client → request validation.
- Update tests to cover OCR-attested responses and bump
chainlink-common/keystoreversions across modules.
Reviewed changes
Copilot reviewed 14 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
core/capabilities/remote/executable/request/client_request.go |
Adds OCR-attestation verification path, metering extraction helper, and new error-handling behavior. |
core/capabilities/remote/executable/request/client_request_test.go |
Extends request tests to cover OCR-attested responses and new constructor signature. |
core/capabilities/remote/executable/request/client_request_internal_test.go |
Adds focused unit tests for attestation verification logic. |
core/capabilities/remote/executable/client.go |
Extends dynamic config + SetConfig to carry OCR3 configs into requests. |
core/capabilities/launcher.go |
Passes OCR3 configs into v2 remote executable client config. |
core/capabilities/remote/executable/client_test.go |
Updates tests for SetConfig signature change. |
core/capabilities/remote/executable/endtoend_test.go |
Updates workflow-node config call signature. |
go.mod |
Bumps github.com/smartcontractkit/chainlink-common and .../keystore versions. |
go.sum |
Checksum updates for bumped dependencies. |
core/scripts/go.mod |
Bumps chainlink-common / keystore versions in scripts module. |
core/scripts/go.sum |
Checksum updates for scripts module. |
deployment/go.mod |
Bumps chainlink-common / keystore versions in deployment module. |
deployment/go.sum |
Checksum updates for deployment module. |
integration-tests/go.mod |
Bumps chainlink-common / keystore versions for integration tests. |
integration-tests/go.sum |
Checksum updates for integration tests. |
integration-tests/load/go.mod |
Bumps chainlink-common / keystore versions for load tests. |
integration-tests/load/go.sum |
Checksum updates for load tests. |
system-tests/lib/go.mod |
Bumps chainlink-common / keystore versions for system-test lib. |
system-tests/lib/go.sum |
Checksum updates for system-test lib. |
system-tests/tests/go.mod |
Bumps chainlink-common / keystore versions for system tests. |
system-tests/tests/go.sum |
Checksum updates for system tests. |
Scrupulous human review focus:
ClientRequest.verifyAttestation(signature bounds checks, config-digest checks, and overall correctness of the “1 response with F+1 sigs” acceptance criteria).ClientRequest.OnMessageerror-handling path forErrResponsePayloadNotAvailableand the interaction with response quorum/timeout behavior.- Operational/logging impact of adding
ocr3ConfigstoSetConfiglogs.
Suggested reviewers (per CODEOWNERS):
- For
/core/capabilities/**:@smartcontractkit/keystone,@smartcontractkit/capabilities-team - For root
go.mod/go.sum:@smartcontractkit/core,@smartcontractkit/foundations - For
/integration-tests/**:@smartcontractkit/devex-tooling,@smartcontractkit/core - For
/deployment/**:@smartcontractkit/ccip-tooling,@smartcontractkit/operations-platform,@smartcontractkit/keystone,@smartcontractkit/core
Comments suppressed due to low confidence (1)
core/capabilities/remote/executable/request/client_request.go:350
OnMessageunmarshals the capability response (pb.UnmarshalCapabilityResponse) to check for OCR attestation, and then for the non-attestation path callsgetMessageHashAndMetadata, which unmarshals the same payload again. Consider refactoring to unmarshal once and reuse the parsed response/metadata when computing the response hash to avoid redundant work on the hot path.
resp, err := pb.UnmarshalCapabilityResponse(msg.Payload)
if err != nil {
return fmt.Errorf("failed to unmarshal capability response: %w", err)
}
if resp.Metadata.OCRAttestation != nil {
rpt, err := extractMeteringFromMetadata(sender, resp.Metadata)
if err != nil {
return fmt.Errorf("failed to extract metering detail from metadata: %w", err)
}
// Since signatures are provided switch to OCR based validation. It's enough to get 1 response with F+1 signatures
// to be confident that the response is honest.
err = c.verifyAttestation(resp, rpt)
if err != nil {
c.lggr.Errorw("failed to verify capability response OCR attestation", "peer", sender, "err", err, "requestID", c.id, "msgPayload", hex.EncodeToString(msg.Payload))
return fmt.Errorf("failed to verify capability response OCR attestation: %w", err)
}
var payload []byte
payload, err = c.encodePayloadWithMetadata(msg, commoncap.ResponseMetadata{Metering: []commoncap.MeteringNodeDetail{rpt}})
if err != nil {
return fmt.Errorf("failed to encode payload with metadata: %w", err)
}
c.sendResponse(clientResponse{Result: payload})
return nil
}
// metering reports per node are aggregated into a single array of values. for any single node message, the
// metering values are extracted from the CapabilityResponse, added to an array, and the CapabilityResponse
// is marshalled without the metering value to get the hash. each node could have a different metering value
// which would result in different hashes. removing the metering detail allows for direct comparison of results.
responseID, metadata, err := c.getMessageHashAndMetadata(msg)
if err != nil {
return fmt.Errorf("failed to get message hash: %w", err)
}
| cfg, ok := c.ocr3Configs[pb.OCR3ConfigDefaultKey] | ||
| if !ok { | ||
| return fmt.Errorf("OCR3 config with key %s not found", pb.OCR3ConfigDefaultKey) | ||
| } | ||
|
|
||
| attestation := resp.Metadata.OCRAttestation | ||
| if len(attestation.Sigs) < int(cfg.F)+1 { | ||
| return fmt.Errorf("not enough signatures: got %d, need at least %d", len(attestation.Sigs), cfg.F+1) | ||
| } | ||
|
|
||
| reportData := commoncap.ResponseToReportData(c.workflowExecutionID, c.referenceID, resp.Payload.Value, metering.SpendUnit, metering.SpendValue) | ||
| sigData := ocr2key.ReportToSigData3(attestation.ConfigDigest, attestation.SequenceNumber, reportData[:]) | ||
| signed := make([]bool, len(cfg.Signers)) |
There was a problem hiding this comment.
Attestation verification currently does not validate that resp.Metadata.OCRAttestation.ConfigDigest matches the configured cfg.ConfigDigest. Without this, a response signed under an unexpected/stale config digest could still be accepted as long as the signer set overlaps. Add an explicit config-digest equality check (and consider also guarding against resp.Metadata.OCRAttestation == nil inside this helper to avoid nil deref if it’s ever called directly).
There was a problem hiding this comment.
There is no issue with accepting reports with old ConfigDigest as long as we have F+1 valid signatures.
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
b62fc5a to
9f8971a
Compare
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
9f8971a to
50e5be8
Compare
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
50e5be8 to
b778e19
Compare
|
|
||
| lggr = logger.With(lggr, "requestId", requestID) // cap ID and method name included in the parent logger | ||
| return newClientRequest(ctx, lggr, requestID, remoteCapabilityInfo, localDonInfo, dispatcher, requestTimeout, tc, types.MethodExecute, rawRequest, workflowExecutionID, req.Metadata.ReferenceID, capMethodName) | ||
| return newClientRequest(ctx, lggr, requestID, remoteCapabilityInfo, localDonInfo, dispatcher, requestTimeout, tc, types.MethodExecute, rawRequest, workflowExecutionID, req.Metadata.ReferenceID, capMethodName, ocr3Configs) |
There was a problem hiding this comment.
We shouldn't pass ocrconfig here. Launcher can extract "signer" keys from Nodes. Check out this: https://github.com/smartcontractkit/chainlink/blob/develop/core/capabilities/launcher.go#L514
Those signer keys correspond to "onchainPublicKey" values from EVM OCR key bundles (which is what all OCRs in CRE should be signing with, by default).
There was a problem hiding this comment.
Switched to signers
| return fmt.Errorf("not enough signatures: got %d, need at least %d", len(attestation.Sigs), cfg.F+1) | ||
| } | ||
|
|
||
| reportData := commoncap.ResponseToReportData(c.workflowExecutionID, c.referenceID, resp.Payload.Value, metering.SpendUnit, metering.SpendValue) |
There was a problem hiding this comment.
This feels awkward. We are putting two metering fields explicitly. ClientRequest shouldn't handle those details. Could we proto-marshal the whole metadata struct and sign that together with payload? That would mean you have to put signatures outside of metadata - maybe as a third field at the same level as payload and metadata?
There was a problem hiding this comment.
Including full metadata feels awkward, too. It contains fields CapDON_N and Peer2PeerID that are not needed in the report, and that must be set to 0 to ensure signatures are valid.
Does not passing the fields explicitly make it more obvious that special handling is needed to ensure that OCR and Don2Don are aligned?
There was a problem hiding this comment.
Could we still pass the whole metadata struct here and let the helper ResponseToReportData extract what's needed? I'd rather have that logic encapsulated in the helper than here.
| return fmt.Errorf("failed to unmarshal capability response: %w", err) | ||
| } | ||
|
|
||
| if resp.Metadata.OCRAttestation != nil { |
There was a problem hiding this comment.
Why is this logic here and not lower where we check "c.responseIDCount[responseID] == c.requiredIdenticalResponses " ?
It seems that it fits there more naturally - we either need F+1 copies or attestation.
There was a problem hiding this comment.
I've tried to separate the two flows to:
- Avoid redundant computation to calculate responseID and store metering details.
- Have the ability to return an error in case of invalid metering details to avoid a hard-to-debug "invalid signature error."
- I've tried to minimize the changes to the existing flow to ensure the previous behaviour is the same.
Let me know if you'd prefer I refactor the logic to be more unified.
There was a problem hiding this comment.
- I'm not too worried.
- You can still do that if the logic is later on, no?
- Can you extract this logic into a helper function? That way it won't obscure existing logic too much.
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
ee49a77 to
db96bf2
Compare
…attestatin # Conflicts: # core/capabilities/remote/executable/request/client_request.go # deployment/go.mod
…attestatin # Conflicts: # core/scripts/go.mod # core/scripts/go.sum # deployment/go.mod # deployment/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum # integration-tests/load/go.mod # integration-tests/load/go.sum # system-tests/lib/go.mod # system-tests/lib/go.sum # system-tests/tests/go.mod # system-tests/tests/go.sum
dhaidashenko
force-pushed
the
feature/PLEX-2611-cre-don2don-accept-ocr-attestatin
branch
from
f1e42c2 to
8d9bd0c
Compare
|
Allow Capabilities Nodes to provide OCR attestation of the response.
Motivation
To reduce the bandwidth used by chain capabilities OCR, for some request types, nodes will exchange hashes of observed RPC responses instead of actual responses. While this greatly reduces the required bandwidth, there is now a high probability that only F+1 chain cap nodes will have the RPC response. Thus, to ensure reliability, we should allow capability nodes to provide OCR attestation so that receiving a response from a single node is sufficient.
All chain capability nodes will still attempt to send the response. If the node has only a report and no RPC payload, it will return a special error indicating that the node must wait for a response from another node.
Changes to Don2Don to ensure that only one capability node returns the report with the payload are out of scope for this PR. And the benefits of that approach are debatable.
Ticket: https://smartcontract-it.atlassian.net/browse/PLEX-2611
Depends on: