From c43dfcbbcc038873f6d4c70a8d795c413dd3131e Mon Sep 17 00:00:00 2001 From: kaleofduty <59616916+kaleofduty@users.noreply.github.com> Date: Tue, 13 May 2025 19:38:47 +0200 Subject: [PATCH] Improvements: - OCR3: Fix an issue that could lead to unhealthy oracles crashing due to accessing already reaped rounds - Documentation & logging improvements Based on d4cbd50eacfac4fb9a44d9ac6132852870a722a4 --- .../confighelper/confighelper.go | 10 +++++-- .../ocr3/protocol/report_attestation.go | 29 ++++++++++++++++--- .../ocr3confighelper/ocr3confighelper.go | 8 +++-- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/offchainreporting2plus/confighelper/confighelper.go b/offchainreporting2plus/confighelper/confighelper.go index bfa13307..fa5ea2e6 100644 --- a/offchainreporting2plus/confighelper/confighelper.go +++ b/offchainreporting2plus/confighelper/confighelper.go @@ -15,7 +15,7 @@ import ( "github.com/smartcontractkit/libocr/offchainreporting2plus/types" ) -// OracleIdentity is identical to the internal type in package config. +// OracleIdentity is identical to the internal type [config.OracleIdentity]. // We intentionally make a copy to make potential future internal modifications easier. type OracleIdentity struct { OffchainPublicKey types.OffchainPublicKey @@ -25,8 +25,9 @@ type OracleIdentity struct { TransmitAccount types.Account } -// PublicConfig is identical to the internal type in package config. -// We intentionally make a copy to make potential future internal modifications easier. +// PublicConfig is identical to the internal type [ocr2config.PublicConfig]. See +// the documentation there for details. We intentionally duplicate the internal +// type to make potential future internal modifications easier. type PublicConfig struct { DeltaProgress time.Duration DeltaResend time.Duration @@ -98,6 +99,7 @@ type OracleIdentityExtra struct { // ContractSetConfigArgsForIntegrationTest generates setConfig args for integration tests in core. // Only use this for testing, *not* for production. +// See [ocr2config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsForEthereumIntegrationTest( oracles []OracleIdentityExtra, f int, @@ -165,6 +167,7 @@ func ContractSetConfigArgsForEthereumIntegrationTest( // ContractSetConfigArgsForTestsWithAuxiliaryArgs generates setConfig args from // the relevant parameters. Only use this for testing, *not* for production. +// See [ocr2config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsForTestsWithAuxiliaryArgs( deltaProgress time.Duration, deltaResend time.Duration, @@ -252,6 +255,7 @@ func (a AuxiliaryArgs) rng() io.Reader { // ContractSetConfigArgsForTests generates setConfig args from the relevant // parameters. Only use this for testing, *not* for production. +// See [ocr2config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsForTests( deltaProgress time.Duration, deltaResend time.Duration, diff --git a/offchainreporting2plus/internal/ocr3/protocol/report_attestation.go b/offchainreporting2plus/internal/ocr3/protocol/report_attestation.go index a1f712c6..2de5dece 100644 --- a/offchainreporting2plus/internal/ocr3/protocol/report_attestation.go +++ b/offchainreporting2plus/internal/ocr3/protocol/report_attestation.go @@ -172,6 +172,15 @@ func (repatt *reportAttestationState[RI]) messageReportSignatures( } func (repatt *reportAttestationState[RI]) eventMissingOutcome(ev EventMissingOutcome[RI]) { + if repatt.rounds[ev.SeqNr] == nil { + repatt.logger.Debug("dropping EventMissingOutcome for unknown seqNr", commontypes.LogFields{ + "evSeqNr": ev.SeqNr, + "highWaterMark": repatt.highWaterMark, + "expiryRounds": repatt.expiryRounds(), + }) + return + } + if repatt.rounds[ev.SeqNr].verifiedCertifiedCommit != nil { repatt.logger.Debug("dropping EventMissingOutcome, already have Outcome", commontypes.LogFields{ "evSeqNr": ev.SeqNr, @@ -183,7 +192,17 @@ func (repatt *reportAttestationState[RI]) eventMissingOutcome(ev EventMissingOut } func (repatt *reportAttestationState[RI]) messageCertifiedCommitRequest(msg MessageCertifiedCommitRequest[RI], sender commontypes.OracleID) { - if repatt.rounds[msg.SeqNr] == nil || repatt.rounds[msg.SeqNr].verifiedCertifiedCommit == nil { + if repatt.rounds[msg.SeqNr] == nil { + repatt.logger.Debug("dropping MessageCertifiedCommitRequest for unknown seqNr", commontypes.LogFields{ + "msgSeqNr": msg.SeqNr, + "sender": sender, + "highWaterMark": repatt.highWaterMark, + "expiryRounds": repatt.expiryRounds(), + }) + return + } + + if repatt.rounds[msg.SeqNr].verifiedCertifiedCommit == nil { repatt.logger.Debug("dropping MessageCertifiedCommitRequest for outcome with unknown certified commit", commontypes.LogFields{ "msgSeqNr": msg.SeqNr, "sender": sender, @@ -211,8 +230,10 @@ func (repatt *reportAttestationState[RI]) messageCertifiedCommitRequest(msg Mess func (repatt *reportAttestationState[RI]) messageCertifiedCommit(msg MessageCertifiedCommit[RI], sender commontypes.OracleID) { if repatt.rounds[msg.CertifiedCommit.SeqNr] == nil { repatt.logger.Warn("dropping MessageCertifiedCommit for unknown seqNr", commontypes.LogFields{ - "msgSeqNr": msg.CertifiedCommit.SeqNr, - "sender": sender, + "msgSeqNr": msg.CertifiedCommit.SeqNr, + "sender": sender, + "highWaterMark": repatt.highWaterMark, + "expiryRounds": repatt.expiryRounds(), }) return } @@ -507,7 +528,7 @@ func (repatt *reportAttestationState[RI]) backgroundComputeReports(ctx context.C func (repatt *reportAttestationState[RI]) eventComputedReports(ev EventComputedReports[RI]) { if repatt.rounds[ev.SeqNr] == nil { - repatt.logger.Debug("dropping EventComputedReports from old round", commontypes.LogFields{ + repatt.logger.Debug("dropping EventComputedReports for unknown seqNr", commontypes.LogFields{ "evSeqNr": ev.SeqNr, "highWaterMark": repatt.highWaterMark, "expiryRounds": repatt.expiryRounds(), diff --git a/offchainreporting2plus/ocr3confighelper/ocr3confighelper.go b/offchainreporting2plus/ocr3confighelper/ocr3confighelper.go index 439335f9..d45f6480 100644 --- a/offchainreporting2plus/ocr3confighelper/ocr3confighelper.go +++ b/offchainreporting2plus/ocr3confighelper/ocr3confighelper.go @@ -11,8 +11,9 @@ import ( "golang.org/x/crypto/curve25519" ) -// PublicConfig is identical to the internal type in package config. -// We intentionally make a copy to make potential future internal modifications easier. +// PublicConfig is identical to the internal type [ocr3config.PublicConfig]. See +// the documentation there for details. We intentionally duplicate the internal +// type to make potential future internal modifications easier. type PublicConfig struct { DeltaProgress time.Duration DeltaResend time.Duration @@ -81,6 +82,7 @@ func PublicConfigFromContractConfig(skipResourceExhaustionChecks bool, change ty // ContractSetConfigArgsForTestsWithAuxiliaryArgsMercuryV02 generates setConfig // args for mercury v0.2. Only use this for testing, *not* for production. +// See [ocr3config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsForTestsMercuryV02( deltaProgress time.Duration, deltaResend time.Duration, @@ -130,6 +132,7 @@ func ContractSetConfigArgsForTestsMercuryV02( // ContractSetConfigArgsForTestsOCR3 generates setConfig args for OCR3. Only use // this for testing, *not* for production. +// See [ocr3config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsForTests( deltaProgress time.Duration, deltaResend time.Duration, @@ -196,6 +199,7 @@ func ContractSetConfigArgsForTests( // This function may be used in production. If you use this as part of multisig // tooling, make sure that the input parameters are identical across all // signers. +// See [ocr3config.PublicConfig] for documentation of the arguments. func ContractSetConfigArgsDeterministic( // The ephemeral secret key used to encrypt the shared secret ephemeralSk [curve25519.ScalarSize]byte,