Multi-OCR3 - interface and sample base implementation #842

elatoskinas · 2024-05-13T11:17:26Z

Motivation

Implements an OCR3 interface that supports the latest OCR needs:

Multi-plugin support: Ability to configure & use multiple OCR plugins under one contract interface
OCR3 plugin compatibility: interface changes (pulled from liquiditymanager OCR3Abstract)
Simplified config digest handling: no longer re-compute config digest / no passing of on-chain config, since the source of truth data will live in the Capability Registry

Solution

Focus: the foucs of the PR is on the interface and structure rather on the exact logic

Implements a MultiOCR3Base abstract contract to match multi-plugin, Capability Registry and OCR3 plugin interface expectations
Converts _transmit to be internal - implementations are expected to implement separate transmit functions per-DON, since each report logic will be different, e.g. for CCIP:

function transmitExecution(
  // same params as _transmit, except the donId
) {
  // ... handle reporting logic
  _transmit(
    s_transmitDonId,
    // (pass in remaining params from function args)
  );
}

function transmitCommit(
  // same params as _transmit, except the donId
) {
  // ... handle reporting logic
  _transmit(
     s_commitDonId,
     // (pass in remaining params from function args)
  );
}

Sample implementation of MultiOCR3Base, with signature skipping support & multi-DON configs
Out of scope
- Config validations & oracle updating logic
- Transmit msg length handling for different transmit function cases
- Gas / contract size optimizations

github-actions · 2024-05-13T11:23:10Z

LCOV of commit `3f25cfd` during Solidity Foundry #4469

Summary coverage rate:
  lines......: 98.8% (1251 of 1266 lines)
  functions..: 96.4% (266 of 276 functions)
  branches...: 92.2% (507 of 550 branches)

Files changed coverage rate: n/a

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

elatoskinas · 2024-05-13T11:38:30Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

-  /// @param rs ith element is the R components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param ss ith element is the S components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param rawVs ith element is the the V component of the ith signature
-  function transmit(


note: this could rename to _auth later on, and skip the transmission event, if we were to move it to another Auth contract.

TBD based on feasibility of merging contracts

Ah transmission event is interesting, its basically used for this https://github.com/smartcontractkit/libocr/blob/73bda92fc450decece23011c007b253bd1be75fd/offchainreporting2plus/types/types.go#L324 offchain. Moving it outside to the caller of _auth is a bit strange because then caller has to parse out the sequenceNumber. Maybe that's worth it though for the separation of concerns this contract is purely a verification contract

I think we can still transmit with the donId/pluginId and we'd be able to differentiate b/w the two plugins and implement that interface correctly.

The higher level issue here is that this will be emitted on the destination chain and not all DON nodes can read the destination, so I wonder if we need to go back to that config tracking w/ voting idea Lorenz pointed out in our call last week, otherwise I'm not sure how you can get the whole committee to land on the same sequence number/config digest.

This is used in the pacemaker to restore the epoch: https://github.com/smartcontractkit/libocr/blob/fd3cab206b2ca3b7ff207996b95673b2d6303ec4/offchainreporting2plus/internal/ocr2/protocol/pacemaker.go#L302-L345 presumably after e.g restarting the node. So seems kinda important.

I don't think its an issue because LatestTransmissionDetails is part of the contract transmitter which will only be invoked on node which can read/write to the remote

I think we can still transmit with the donId/pluginId and we'd be able to differentiate b/w the two plugins and implement that interface correctly.

Yeah this makes sense - I agree with this. However, if we have a decoupled auth contract, it makes more sense emitting on the contract that is actually the OCR contract, rather than the Auth contract to decouple (otherwise the AuthContract emits multiple OCR events for different instances / capabilities)

contracts/src/v0.8/ccip/ocr/MultiOCR3Abstract.sol

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

contracts/src/v0.8/ccip/ocr/MultiOCR3Abstract.sol

connorwstein · 2024-05-13T23:53:05Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

+    /// @notice latest OCR config
+    ConfigInfo configInfo;
+    /// @notice makes it easier for offchain systems to extract config from logs.
+    uint32 latestConfigBlockNumber;


I think this latestConfigBlockNumber optimization not really necessary anymore with our log system (logpoller) offchain, can remove

Could you elaborate a bit on the log poller - does it mean we will have the block numbers next to the ConfigSet events which will be efficiently queryable, so we no longer need the field on-chain?

I don't think we need this because config won't be fetched from the dest chain anyway, only from home chain.

But in the blue/green model, the config digests on the remote chains might be set at different intervals than on the home chain - isn't it helpful to be able to query the history in case something goes wrong?

You're right, config digests on remote will be set after the green config is set on home chain. What kind of sanity check would you do with latestConfigBlockNumber though (offchain)? It has no relationship to the home chain config block number. I think OCR used it in the past so that we can do a granular log query just for ConfigSet logs in that particular block but since config log fetching logic is completely different now, not sure how you'd use this

It would not be used in sanity checks, but rather to retrieve the blocks when a staging config was set to the active config (by reading the latest block value and applying a query filter). The purpose would just be for debugging / inspection purposes if something goes wrong.

But I'd expect we already have this data available and efficiently queryable on the log poller?

Removed the field

connorwstein · 2024-05-13T23:59:24Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

-  /// @param rs ith element is the R components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param ss ith element is the S components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param rawVs ith element is the the V component of the ith signature
-  function transmit(


Ah transmission event is interesting, its basically used for this https://github.com/smartcontractkit/libocr/blob/73bda92fc450decece23011c007b253bd1be75fd/offchainreporting2plus/types/types.go#L324 offchain. Moving it outside to the caller of _auth is a bit strange because then caller has to parse out the sequenceNumber. Maybe that's worth it though for the separation of concerns this contract is purely a verification contract

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

contracts/src/v0.8/ccip/ocr/MultiOCR3Abstract.sol

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

makramkd · 2024-05-14T09:51:12Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

-  /// @param rs ith element is the R components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param ss ith element is the S components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param rawVs ith element is the the V component of the ith signature
-  function transmit(


I think we can still transmit with the donId/pluginId and we'd be able to differentiate b/w the two plugins and implement that interface correctly.

The higher level issue here is that this will be emitted on the destination chain and not all DON nodes can read the destination, so I wonder if we need to go back to that config tracking w/ voting idea Lorenz pointed out in our call last week, otherwise I'm not sure how you can get the whole committee to land on the same sequence number/config digest.

makramkd · 2024-05-14T09:53:35Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

-  /// @param rs ith element is the R components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param ss ith element is the S components of the ith signature on report. Must have at most MAX_NUM_ORACLES entries
-  /// @param rawVs ith element is the the V component of the ith signature
-  function transmit(


This is used in the pacemaker to restore the epoch: https://github.com/smartcontractkit/libocr/blob/fd3cab206b2ca3b7ff207996b95673b2d6303ec4/offchainreporting2plus/internal/ocr2/protocol/pacemaker.go#L302-L345 presumably after e.g restarting the node. So seems kinda important.

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

…nfigDigestAndEpoch

elatoskinas · 2024-05-17T16:42:56Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

+    }
+  }
+
+  // TODO: is this function required?


I recon we don't need this anymore? Or is there still a use-case for this? @connorwstein @makramkd

It would only be helpful if offchain we we're trying to make use of a shared transmitter which already implemented https://github.com/smartcontractkit/libocr/blob/73bda92fc450decece23011c007b253bd1be75fd/offchainreporting2plus/types/types.go#L324. I suspect we'll have a custom transmitter anyways to call different functions we can remove it for now.

elatoskinas · 2024-05-17T16:43:17Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

+  // The constant-length components of the msg.data sent to transmit.
+  // See the "If we wanted to call sam" example on for example reasoning
+  // https://solidity.readthedocs.io/en/v0.7.2/abi-spec.html
+  // TODO: assumes a constant function sig like in _transmit. Will need adjustment (either removing


note: will revisit this and validations in a follow-up PR focused on validations

connorwstein · 2024-05-20T21:11:53Z

contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol

+    }
+  }
+
+  // TODO: is this function required?


It would only be helpful if offchain we we're trying to make use of a shared transmitter which already implemented https://github.com/smartcontractkit/libocr/blob/73bda92fc450decece23011c007b253bd1be75fd/offchainreporting2plus/types/types.go#L324. I suspect we'll have a custom transmitter anyways to call different functions we can remove it for now.

elatoskinas · 2024-05-21T09:49:14Z

Note: to be iterated in next PRs - the interface is not yet final

elatoskinas added 2 commits May 13, 2024 11:55

chore: create copies for OCR3

236cf9f

feat: implement multi-ocr3 interfaces and sample solutions

801bd1b

elatoskinas temporarily deployed to sdlc May 13, 2024 11:17 — with GitHub Actions Inactive

elatoskinas commented May 13, 2024

View reviewed changes

elatoskinas requested a review from RayXpub May 13, 2024 11:39

connorwstein reviewed May 13, 2024

View reviewed changes

cds95 reviewed May 14, 2024

View reviewed changes

makramkd reviewed May 14, 2024

View reviewed changes

elatoskinas added 3 commits May 14, 2024 17:18

docs: F value naming and additional docs for config

14a9425

feat: convert donId to ocrPluginType

f5b06df

feat: allow setting multiple configs and fetching a full config

bfc3ecd

elatoskinas temporarily deployed to sdlc May 14, 2024 16:57 — with GitHub Actions Inactive

elatoskinas added 3 commits May 14, 2024 19:02

style: rename to isSignatureVerificationEnabled

034fdff

perf: remove latestConfigBlockNumber optimisation and re-add latestCo…

3313ed3

…nfigDigestAndEpoch

refactor: remove MultiOCR3Abstract

00ff3b7

elatoskinas temporarily deployed to sdlc May 15, 2024 16:54 — with GitHub Actions Inactive

elatoskinas added 6 commits May 16, 2024 12:57

test: setup functions and stack pressure fixes

e38d420

test: unit tests for MultiOCR3Base

e9f8d44

Merge branch 'ccip-develop' into feat/multi-ocr3-base

3327464

test: unit tests for MultiOCR3Base

1f148ed

chore: remove stale TOODs

984e1dc

chore: add snapshots and changeset

6d76798

elatoskinas temporarily deployed to sdlc May 17, 2024 16:31 — with GitHub Actions Inactive

chore: fix lint errors

b9eb404

elatoskinas changed the base branch from temp/multi-ocr-copies to ccip-develop May 17, 2024 16:33

elatoskinas temporarily deployed to sdlc May 17, 2024 16:35 — with GitHub Actions Inactive

elatoskinas marked this pull request as ready for review May 17, 2024 16:39

elatoskinas requested review from RensR and matYang as code owners May 17, 2024 16:39

elatoskinas requested a review from connorwstein May 17, 2024 16:41

elatoskinas commented May 17, 2024

View reviewed changes

chore: update snapshot

3f25cfd

elatoskinas temporarily deployed to sdlc May 17, 2024 16:44 — with GitHub Actions Inactive

connorwstein approved these changes May 20, 2024

View reviewed changes

RensR approved these changes May 21, 2024

View reviewed changes

elatoskinas merged commit a0d4251 into ccip-develop May 21, 2024
79 checks passed

elatoskinas deleted the feat/multi-ocr3-base branch May 21, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-OCR3 - interface and sample base implementation #842

Multi-OCR3 - interface and sample base implementation #842

elatoskinas commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

elatoskinas May 13, 2024

connorwstein May 13, 2024

makramkd May 14, 2024

makramkd May 14, 2024

connorwstein May 16, 2024

elatoskinas May 17, 2024 •

edited

connorwstein May 13, 2024

elatoskinas May 14, 2024

makramkd May 14, 2024

elatoskinas May 14, 2024

makramkd May 14, 2024

elatoskinas May 14, 2024

elatoskinas May 15, 2024

connorwstein May 13, 2024

makramkd May 14, 2024

makramkd May 14, 2024

elatoskinas May 17, 2024

connorwstein May 20, 2024

elatoskinas May 17, 2024

connorwstein May 20, 2024

elatoskinas commented May 21, 2024

Multi-OCR3 - interface and sample base implementation #842

Multi-OCR3 - interface and sample base implementation #842

Conversation

elatoskinas commented May 13, 2024 • edited

Motivation

Solution

github-actions bot commented May 13, 2024 • edited

LCOV of commit 3f25cfd during Solidity Foundry #4469

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elatoskinas May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elatoskinas commented May 21, 2024

elatoskinas commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

LCOV of commit `3f25cfd` during Solidity Foundry #4469

elatoskinas May 17, 2024 •

edited