Implement CommitPlugin with RMN OffChain Blessing #17

rstout · 2024-07-02T21:50:20Z

Implements a new version of the Commit Plugin in /commit_rmn_ocb (ocb = off chain blessing) based on the spec. This is basically only the OCR3 function implementations (e.g. Query, Observation, Outcome, etc). No tests have been written yet. I recommend starting with the README.

plugintypes/commit.go

makramkd

Still combing through the PR, this is just my first pass

makramkd · 2024-07-30T11:33:52Z

commit_rmn_ocb/types.go

+	// GetOnRampMaxSeqNums returns the maximum sequence number that this oracle finds on the OnRamp for each given chain
+	// (for the configured dest chain)
+	GetOnRampMaxSeqNums() ([]plugintypes.SeqNumChain, error)


I think we should be a bit more precise here in terms of what we want from this method and try to match existing terminology in the ramp contracts so we don't get confused.

The onramp has this method: getExpectedNextSequenceNumber which returns the next sequence number that is going to be used by the onramp. Is this what we want?

Or do we want the last-used sequence number, which would be getExpectedNextSequenceNumber() - 1?

getExpectedNextSequenceNumber is a method on the OffRamp, not the OnRamp right? I've renamed the OffRamp seq nums to OffRampNextSeqNums and kept OnRampMaxSeqNums, we can discuss changing the names further.

makramkd · 2024-07-30T11:35:10Z

commit_rmn_ocb/types.go

+	// GetOffRampMaxSeqNums returns the maximum sequence number that this oracle finds on the OffRamp for each given
+	// chain (for the configured dest chain)
+	GetOffRampMaxSeqNums() ([]plugintypes.SeqNumChain, error)


Another piece of terminology / consistency in order to avoid confusion: these have been called "max committed sequence numbers" so maybe call this GetMaxCommittedSeqNums

I've renamed this to OffRampNextSeqNums, but happy to discuss clearer names

commit_rmn_ocb/types.go

makramkd · 2024-07-30T11:47:19Z

commit_rmn_ocb/types.go

+	case ReportIntervalsSelected:
+		return BuildingReport
+	case ReportGenerated:
+		return WaitingForReportTransmission
+	case ReportEmpty:
+		return SelectingRangesForReport
+	case ReportNotYetTransmitted:
+		return WaitingForReportTransmission
+	case ReportTransmitted:
+		return SelectingRangesForReport
+	case ReportNotTransmitted:
+		return SelectingRangesForReport


nit: This would be more readable if the transitions that lead to the same state are grouped together, e.g

switch o.OutcomeType { case ReportIntervalsSelected: return BuildingReport case ReportGenerated: fallthrough case ReportNotYetTransmitted: return WaitingForReportTransmission case ReportEmpty: fallthrough case ReportTransmitted: fallthrough case ReportNotTransmitted: fallthrough default: return SelectingRangesForReport }

Feels like this might be better as a pure func rather than tied to the Outcome, but I'm not too opinionated about that. Also needs a set of tests to cover all the transitions.

commit_rmn_ocb/types.go

makramkd · 2024-07-30T12:02:56Z

commit_rmn_ocb/validate_observation.go

+
+func validateFChain(fChain map[cciptypes.ChainSelector]int) error {
+	for _, f := range fChain {
+		if f < 0 {


Should be f <= 0 since f is never 0.

f can be 0 for chains that are not RMN enabled

commit_rmn_ocb/types.go

makramkd · 2024-07-30T12:10:59Z

commit_rmn_ocb/plugin.go

+	return nil
+}
+
+func (p *Plugin) decodeOutcome(outcome ocr3types.Outcome) (CommitPluginOutcome, CommitPluginState) {


Feels like this should return an error instead of just returning SelectingRangesForReport if there is an error decoding

My logic is: if there's an issue decoding, just go to the "select interval" state instead of erroring-out

commit_rmn_ocb/observation.go

makramkd · 2024-08-13T08:33:32Z

commitrmnocb/metrics.go

@@ -0,0 +1 @@
+package commitrmnocb


Is the intention of this file to house some prometheus metrics?

makramkd · 2024-08-13T08:34:51Z

commitrmnocb/observation.go

+		// Assert there are no sequence number gaps in msgs
+		if i > 0 {
+			if msg.Header.SequenceNumber != msgs[i-1].Header.SequenceNumber+1 {
+				return [32]byte{}, fmt.Errorf("found non-consecutive sequence numbers when computing merkle root, "+
+					"gap between sequence nums %d and %d, messages: %v", msgs[i-1].Header.SequenceNumber,
+					msg.Header.SequenceNumber, msgs)
+			}
+		}


Nit: would this be useful as a separate helper that is unit tested?

See ConstructMerkleTree in execute/report/report.go. Not a drop-in helper since it uses exec specific types.

commitrmnocb/observation.go

makramkd · 2024-08-13T08:53:02Z

commitrmnocb/report.go

+	if outcome.OutcomeType != ReportGenerated {
+		return []ocr3types.ReportWithInfo[[]byte]{}, nil
+	}


Worth adding a log for this case?

This is the happy path (e.g. in the round when intervals are chosen), so I think logging here would just be verbose.

commitrmnocb/report.go

commitrmnocb/types.go

makramkd · 2024-08-13T08:57:35Z

pluginconfig/commit.go

+	// The maximum number of times to check if the previous report has been transmitted
+	MaxReportTransmissionCheckAttempts uint


Is this something we want all plugin instances in the DON to have in common? If so, it should go in the OffchainConfig struct below

What's the difference between CommitPluginConfig and CommitOffchainConfig?

makramkd · 2024-08-13T09:18:14Z

Looks good overall, mostly minor comments above 😄

winder

Mostly nits and questions. Looks great overall.

I did have some confusion around the transient nature of states and the outcome type -- especially in the Reports function where it would have been nice to check for a BuildingReport state rather than a ReportGenerated outcome.

commitrmnocb/factory.go

commitrmnocb/types.go

commitrmnocb/observation.go

commitrmnocb/outcome.go

winder · 2024-08-14T16:36:45Z

commitrmnocb/types.go

+
+const (
+	ReportIntervalsSelected OutcomeType = iota + 1
+	ReportGenerated


nit: the report doesn't generate until after the outcome. I'd also consider removing the Report prefix from all of these.

Suggested change

ReportGenerated

ReportGeneration

I prefer past tense, as in practice these are the states of the previous outcome. I think dropping "Report" might lose some clarity imo.

commitrmnocb/report.go

winder

I think most/all of the feedback I left can wait until a followup PR. Feel free to merge at your discretion.

github-actions · 2024-08-15T17:11:03Z

Test Coverage

Branch	Coverage
rs/commit_plugin_rmn_ocb	66.4%
ccip-develop	76.2%

rstout force-pushed the rs/commit_plugin_rmn_ocb branch 6 times, most recently from f3e3035 to 113e94e Compare July 9, 2024 21:43

rstout force-pushed the rs/commit_plugin_rmn_ocb branch 11 times, most recently from d62f0b3 to 1608eb2 Compare July 16, 2024 18:27

rstout force-pushed the rs/commit_plugin_rmn_ocb branch from f22aa47 to e26270d Compare July 18, 2024 03:48

dimkouv reviewed Jul 30, 2024

View reviewed changes

plugintypes/commit.go Outdated Show resolved Hide resolved

makramkd reviewed Jul 30, 2024

View reviewed changes

rstout force-pushed the rs/commit_plugin_rmn_ocb branch 6 times, most recently from f37494d to e245649 Compare August 9, 2024 17:53

rstout force-pushed the rs/commit_plugin_rmn_ocb branch 4 times, most recently from af41868 to 4506323 Compare August 12, 2024 21:30

makramkd reviewed Aug 13, 2024

View reviewed changes

rstout force-pushed the rs/commit_plugin_rmn_ocb branch from 0f8eced to ffb55ad Compare August 13, 2024 18:54

winder reviewed Aug 14, 2024

View reviewed changes

rstout added 18 commits August 14, 2024 13:02

Implement CommitPlugin with RMN OffChain Blessing

8295022

Outcome/consensus WIP

bfc0446

Outcome/Consensus cleanup

e6a6e21

Further clean up

b590b5c

Add report methods

c0ee3c7

Documentation, clean up

052b8db

Add validate_observation tests

3573dc8

Implement ObserveMerkleRoots using ccipReader

dc4aab4

Decouple from deprecated RMN logic

9d10839

Remove RMN

af34855

Remove OnChain iface

60a30a7

Fix test compile errors

893b400

Add PluginFactory

281c18d

Changes to get the new Commit Plugin working in the integration test

38e5cfa

Fix message hashing, allows the integration test to pass

cb6b791

Clean up

911bdb7

Add observation_test.go

f4d1eae

Address PR comments

a9cb55d

rstout force-pushed the rs/commit_plugin_rmn_ocb branch from ffb55ad to d259ddc Compare August 14, 2024 20:02

Add unit tests for Observation

3afd871

rstout force-pushed the rs/commit_plugin_rmn_ocb branch from d259ddc to 3afd871 Compare August 14, 2024 21:14

makramkd approved these changes Aug 15, 2024

View reviewed changes

winder approved these changes Aug 15, 2024

View reviewed changes

Address PR feedback

8e221bb

rstout merged commit 7bdc5fd into ccip-develop Aug 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CommitPlugin with RMN OffChain Blessing #17

Implement CommitPlugin with RMN OffChain Blessing #17

rstout commented Jul 2, 2024 •

edited

Loading

makramkd left a comment

makramkd Jul 30, 2024

rstout Aug 9, 2024

makramkd Jul 30, 2024

rstout Aug 9, 2024

makramkd Jul 30, 2024

makramkd Jul 30, 2024

rstout Aug 12, 2024

makramkd Jul 30, 2024

rstout Aug 12, 2024

makramkd Aug 13, 2024

rstout Aug 13, 2024

makramkd Aug 13, 2024

winder Aug 14, 2024

makramkd Aug 13, 2024

rstout Aug 13, 2024

makramkd Aug 13, 2024

rstout Aug 13, 2024

makramkd commented Aug 13, 2024

winder left a comment

winder Aug 14, 2024

rstout Aug 14, 2024

winder left a comment

github-actions bot commented Aug 15, 2024

		// The maximum number of times to check if the previous report has been transmitted
		MaxReportTransmissionCheckAttempts uint

Implement CommitPlugin with RMN OffChain Blessing #17

Implement CommitPlugin with RMN OffChain Blessing #17

Conversation

rstout commented Jul 2, 2024 • edited Loading

makramkd left a 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

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

makramkd commented Aug 13, 2024

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2024

Test Coverage

rstout commented Jul 2, 2024 •

edited

Loading