Improvements:

- OCR3.1 polishing
- Handle ContractConfigTracker.Notify channel rotation

Based on 27d60332a83ba0d6c33ccf02410d38a26301956f.

Co-authored-by: kaleofduty <59616916+kaleofduty@users.noreply.github.com>

Author: gtklocker

internal/jmt/jmt.go

@@ -7,10 +7,11 @@ import (
 	"math"
 	"slices"
 	"sort"
+
+	"github.com/smartcontractkit/libocr/internal/util"
 )
 
 type KeyValue struct {
-	// Key length must be greater than 0.
 	Key []byte
 	// Value of nil is permitted and indicates a desire to delete Key.
 	Value []byte
@@ -67,10 +68,6 @@ func BatchUpdate(
 		seenDigestedKeys := make(map[Digest]struct{}, len(keyValueUpdates))
 
 		for i, keyValue := range keyValueUpdates {
-			if len(keyValue.Key) == 0 {
-				return NodeKey{}, fmt.Errorf("%d-th keyValueUpdate: key is empty", i)
-			}
-
 			keyDigest := DigestKey(keyValue.Key)
 			if _, ok := seenDigestedKeys[keyDigest]; ok {
 				return NodeKey{}, fmt.Errorf("%d-th keyValueUpdate: duplicate key %v in keyValueUpdates", i, keyValue.Key)
@@ -599,7 +596,7 @@ func Read(
 
 	if n, ok := rootNode.(*LeafNode); ok {
 		if n.KeyDigest == DigestKey(key) {
-			return n.Value, nil
+			return util.NilCoalesceSlice(n.Value), nil
 		} else {
 			return nil, nil
 		}
@@ -788,7 +785,10 @@ func readRange(
 				truncated = true
 			} else {
 				keysPlusValuesBytes += len(n.Key) + len(n.Value)
-				keyValues = append(keyValues, KeyValue{n.Key, n.Value})
+				keyValues = append(keyValues, KeyValue{
+					util.NilCoalesceSlice(n.Key),
+					util.NilCoalesceSlice(n.Value),
+				})
 			}
 		}
 	}

offchainreporting2plus/internal/managed/track_config.go

@@ -28,17 +28,25 @@ func (state *trackConfigState) run() {
 	// Check immediately after startup
 	tCheckLatestConfigDetails := time.After(0)
 
-	chNotify := state.configTracker.Notify()
+	ignoreNotify := false
 
 	for {
+		var chNotify <-chan struct{}
+		if ignoreNotify {
+			chNotify = nil
+		} else {
+			chNotify = state.configTracker.Notify()
+		}
+
 		select {
 		case _, ok := <-chNotify:
 			if ok {
 				// Check immediately for new config
 				tCheckLatestConfigDetails = time.After(0 * time.Second)
 				state.logger.Info("TrackConfig: ContractConfigTracker.Notify() fired", nil)
 			} else {
-				chNotify = nil
+				ignoreNotify = true
+				chNotify = nil //nolint:ineffassign
 				state.logger.Error("TrackConfig: ContractConfigTracker.Notify() was closed, which should never happen. Will ignore ContractConfigTracker.Notify() from now", nil)
 			}
 		case <-tCheckLatestConfigDetails:

offchainreporting2plus/internal/ocr3_1/protocol/state_sync_tree.go

@@ -479,6 +479,7 @@ func (stasy *stateSyncState[RI]) messageTreeSyncChunkResponse(msg MessageTreeSyn
 		stasy.treeSyncState.logger.Warn("could not create kv read/write transaction", commontypes.LogFields{
 			"err": err,
 		})
+		return
 	}
 	defer kvReadWriteTxn.Discard()
 

offchainreporting2plus/internal/shim/ocr3_1_key_value_store.go

@@ -211,6 +211,8 @@ func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) Commit() error {
 }
 
 func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) Delete(key []byte) error {
+	key = util.NilCoalesceSlice(key)
+
 	if !(len(key) <= ocr3_1types.MaxMaxKeyValueKeyBytes) {
 		return fmt.Errorf("key length %d exceeds maximum %d", len(key), ocr3_1types.MaxMaxKeyValueKeyBytes)
 	}
@@ -434,6 +436,9 @@ func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) ApplyWriteSet(write
 }
 
 func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) Write(key []byte, value []byte) error {
+	key = util.NilCoalesceSlice(key)
+	value = util.NilCoalesceSlice(value)
+
 	start := time.Now()
 	defer func() {
 		s.metrics.txWriteDurationSeconds.Observe(float64(time.Since(start).Seconds()))
@@ -446,8 +451,6 @@ func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) Write(key []byte, v
 		return fmt.Errorf("value length %d exceeds maximum %d", len(value), ocr3_1types.MaxMaxKeyValueValueBytes)
 	}
 
-	value = util.NilCoalesceSlice(value)
-
 	s.mu.Lock()
 	if s.closedForWriting {
 		s.mu.Unlock()
@@ -482,6 +485,8 @@ func (s *SemanticOCR3_1KeyValueDatabaseReadTransaction) Discard() {
 }
 
 func (s *SemanticOCR3_1KeyValueDatabaseReadTransaction) Read(key []byte) ([]byte, error) {
+	key = util.NilCoalesceSlice(key)
+
 	if !(len(key) <= ocr3_1types.MaxMaxKeyValueKeyBytes) {
 		return nil, fmt.Errorf("key length %d exceeds maximum %d", len(key), ocr3_1types.MaxMaxKeyValueKeyBytes)
 	}
@@ -714,7 +719,8 @@ func (s *SemanticOCR3_1KeyValueDatabaseReadWriteTransaction) VerifyAndWriteTreeS
 	prevIdx := startIndex
 	for i, kv := range keyValues {
 		if kv.Value == nil {
-			return protocol.VerifyAndWriteTreeSyncChunkResultByzantine, fmt.Errorf("leaf %v has nil value", kv)
+
+			keyValues[i].Value = []byte{}
 		}
 		idx := hashPluginKey(kv.Key)
 		if bytes.Compare(idx[:], startIndex[:]) < 0 {

offchainreporting2plus/ocr3_1types/plugin.go

@@ -24,19 +24,27 @@ type ReportingPluginFactory[RI any] interface {
 // Deprecated: Use KeyValueStateReader instead.
 type KeyValueReader = KeyValueStateReader
 
-// Provides read access to the replicated KeyValueState.
+// KeyValueStateReader provides read access to the replicated KeyValueState.
 type KeyValueStateReader interface {
-	// A return value of nil indicates that the key does not exist.
+	// Read reads the value associated with the given key.
+	// A nil key is interpreted as an empty slice.
+	// If the key exists, the returned value will be non-nil.
+	// If the key does not exist, it returns (nil, nil).
 	Read(key []byte) ([]byte, error)
 }
 
 // Deprecated: Use KeyValueStateReadWriter instead.
 type KeyValueReadWriter = KeyValueStateReadWriter
 
-// Provides read and write access to the replicated KeyValueState.
+// KeyValueStateReadWriter provides read and write access to the replicated KeyValueState.
 type KeyValueStateReadWriter interface {
 	KeyValueStateReader
+	// Write writes the value for the provided key.
+	// A nil key or value is interpreted as an empty slice.
+	// To delete a key, use the Delete method.
 	Write(key []byte, value []byte) error
+	// Delete deletes the provided key and its associated value.
+	// A nil key is interpreted as an empty slice.
 	Delete(key []byte) error
 }
 
@@ -67,10 +75,10 @@ type KeyValueStateReadWriter interface {
 // the round. For each report, ShouldAcceptAttestedReport will be called, iff
 // the oracle is in the set of transmitters for the report. If
 // ShouldAcceptAttestedReport returns true, ShouldTransmitAcceptedReport will be
-// called. However, an ReportingPlugin must also correctly handle the case where
+// called. However, a ReportingPlugin must also correctly handle the case where
 // faults occur.
 //
-// In particular, an ReportingPlugin must deal with cases where:
+// In particular, a ReportingPlugin must deal with cases where:
 //   - only a subset of the functions on the ReportingPlugin are invoked for a
 //     given round
 //   - the observation returned by Observation is not included in the list of
@@ -83,7 +91,7 @@ type KeyValueStateReadWriter interface {
 //
 // # Engineering requirements for ReportingPlugin implementations
 //
-// All functions on an ReportingPlugin must be thread-safe.
+// All functions on a ReportingPlugin must be thread-safe.
 //
 // The execution of the functions in the ReportingPlugin is on the critical path
 // of the protocol's execution. A blocking function may block the oracle from
@@ -111,7 +119,7 @@ type KeyValueStateReadWriter interface {
 //     a function from returning an error on context expiration.
 //
 // For a given OCR protocol instance, there can be many (consecutive) instances
-// of an ReportingPlugin, e.g. due to software restarts. If you need
+// of a ReportingPlugin, e.g. due to software restarts. If you need
 // ReportingPlugin state to survive across restarts, you should probably
 // persist it in the KeyValueState. A ReportingPlugin instance will only ever serve a
 // single protocol instance. State is not preserved between protocol instances.
@@ -282,8 +290,8 @@ type ReportingPlugin[RI any] interface {
 	//
 	// Don't do anything slow in here.
 	//
-	// The KeyValueReader gives read access to the key-value store in the state
-	// that it is after the StateTransition for seqNr is computed.
+	// The keyValueStateReader gives read access to the replicated KeyValueState
+	// in the state that it is after the StateTransition for seqNr is computed.
 	Committed(
 		ctx context.Context,
 		seqNr uint64,
@@ -327,7 +335,7 @@ type ReportingPlugin[RI any] interface {
 	// As mentioned above, you should gracefully handle only a subset of a
 	// ReportingPlugin's functions being invoked for a given report. For
 	// example, due to reloading persisted pending transmissions from the
-	// database upon oracle restart, this function  may be called with reports
+	// database upon oracle restart, this function may be called with reports
 	// that no other function of this instance of this interface has ever
 	// been invoked on.
 	ShouldTransmitAcceptedReport(

offchainreporting2plus/ocr3types/plugin.go

@@ -55,7 +55,7 @@ type ReportingPluginConfig struct {
 
 type ReportWithInfo[RI any] struct {
 	Report types.Report
-	// Metadata about the report passed to transmitter, keyring, etc..., e.g.
+	// Metadata about the report passed to transmitter, keyring, etc., e.g.
 	// to trace flow of report through the system.
 	Info RI
 }
@@ -108,15 +108,15 @@ type OutcomeContext struct {
 // A ReportingPlugin allows plugging custom logic into the OCR3 protocol. The
 // OCR protocol handles cryptography, networking, ensuring that a sufficient
 // number of nodes is in agreement about any report, transmitting the report to
-// the contract, etc... The ReportingPlugin handles application-specific logic.
+// the contract, etc. The ReportingPlugin handles application-specific logic.
 // To do so, the ReportingPlugin defines a number of callbacks that are called
 // by the OCR protocol logic at certain points in the protocol's execution flow.
 // The report generated by the ReportingPlugin must be in a format understood by
-// contract that the reports are transmitted to.
+// the contract that the reports are transmitted to.
 //
 // We assume that each correct node participating in the protocol instance will
 // be running the same ReportingPlugin implementation. However, not all nodes
-// may be correct; up to f nodes be faulty in arbitrary ways (aka byzantine
+// may be correct; up to f nodes may be faulty in arbitrary ways (aka byzantine
 // faults). For example, faulty nodes could be down, have intermittent
 // connectivity issues, send garbage messages, or be controlled by an adversary.
 //
@@ -126,10 +126,10 @@ type OutcomeContext struct {
 // the round. For each report, ShouldAcceptAttestedReport will be called, iff
 // the oracle is in the set of transmitters for the report. If
 // ShouldAcceptAttestedReport returns true, ShouldTransmitAcceptedReport will be
-// called. However, an ReportingPlugin must also correctly handle the case where
+// called. However, a ReportingPlugin must also correctly handle the case where
 // faults occur.
 //
-// In particular, an ReportingPlugin must deal with cases where:
+// In particular, a ReportingPlugin must deal with cases where:
 //
 // - only a subset of the functions on the ReportingPlugin are invoked for a
 // given round
@@ -147,7 +147,7 @@ type OutcomeContext struct {
 // call traces. E.g., the ReportingPlugin's Observation function may have been
 // invoked on node A, but not on node B.
 //
-// All functions on an ReportingPlugin should be thread-safe.
+// All functions on a ReportingPlugin should be thread-safe.
 //
 // The execution of the functions in the ReportingPlugin is on the critical path
 // of the protocol's execution. A blocking function may block the oracle from
@@ -160,10 +160,10 @@ type OutcomeContext struct {
 // to configure timeouts.
 //
 // For a given OCR protocol instance, there can be many (consecutive) instances
-// of an ReportingPlugin, e.g. due to software restarts. If you need
+// of a ReportingPlugin, e.g. due to software restarts. If you need
 // ReportingPlugin state to survive across restarts, you should store it in the
 // Outcome or persist it. A ReportingPlugin instance will only ever serve a
-// single protocol instance. Outcomes and other state are are not preserved
+// single protocol instance. Outcomes and other state are not preserved
 // between protocol instances. A fresh protocol instance will start with a clean
 // state. Carrying state between different protocol instances is up to the
 // ReportingPlugin logic.
@@ -179,7 +179,7 @@ type ReportingPlugin[RI any] interface {
 	//
 	// You may assume that the outctx.SeqNr is increasing monotonically (though
 	// *not* strictly) across the lifetime of a protocol instance and that
-	// outctx.previousOutcome contains the consensus outcome with sequence
+	// outctx.PreviousOutcome contains the consensus outcome with sequence
 	// number (outctx.SeqNr-1).
 	Query(ctx context.Context, outctx OutcomeContext) (types.Query, error)
 
@@ -188,18 +188,18 @@ type ReportingPlugin[RI any] interface {
 	//
 	// You may assume that the outctx.SeqNr is increasing monotonically (though
 	// *not* strictly) across the lifetime of a protocol instance and that
-	// outctx.previousOutcome contains the consensus outcome with sequence
+	// outctx.PreviousOutcome contains the consensus outcome with sequence
 	// number (outctx.SeqNr-1).
 	Observation(ctx context.Context, outctx OutcomeContext, query types.Query) (types.Observation, error)
 
 	// Should return an error if an observation isn't well-formed.
-	// Non-well-formed  observations will be discarded by the protocol. This
+	// Non-well-formed observations will be discarded by the protocol. This
 	// function should be pure. This is called for each observation, don't do
 	// anything slow in here.
 	//
 	// You may assume that the outctx.SeqNr is increasing monotonically (though
 	// *not* strictly) across the lifetime of a protocol instance and that
-	// outctx.previousOutcome contains the consensus outcome with sequence
+	// outctx.PreviousOutcome contains the consensus outcome with sequence
 	// number (outctx.SeqNr-1).
 	ValidateObservation(ctx context.Context, outctx OutcomeContext, query types.Query, ao types.AttributedObservation) error
 
@@ -227,7 +227,7 @@ type ReportingPlugin[RI any] interface {
 	//
 	// You may assume that the outctx.SeqNr is increasing monotonically (though
 	// *not* strictly) across the lifetime of a protocol instance and that
-	// outctx.previousOutcome contains the consensus outcome with sequence
+	// outctx.PreviousOutcome contains the consensus outcome with sequence
 	// number (outctx.SeqNr-1).
 	//
 	// You may assume that the provided list of attributed observations has been
@@ -244,10 +244,9 @@ type ReportingPlugin[RI any] interface {
 	// This is likely to change in the future. It will likely be returning a
 	// list of report batches, where each batch goes into its own Merkle tree.
 	//
-	// You may assume that the outctx.SeqNr is increasing monotonically (though
+	// You may assume that seqNr is increasing monotonically (though
 	// *not* strictly) across the lifetime of a protocol instance and that
-	// outctx.previousOutcome contains the consensus outcome with sequence
-	// number (outctx.SeqNr-1).
+	// outcome contains the consensus outcome with sequence number (seqNr-1).
 	Reports(ctx context.Context, seqNr uint64, outcome Outcome) ([]ReportPlus[RI], error)
 
 	// Decides whether a report should be accepted for transmission. Any report
@@ -269,7 +268,7 @@ type ReportingPlugin[RI any] interface {
 	// As mentioned above, you should gracefully handle only a subset of a
 	// ReportingPlugin's functions being invoked for a given report. For
 	// example, due to reloading persisted pending transmissions from the
-	// database upon oracle restart, this function  may be called with reports
+	// database upon oracle restart, this function may be called with reports
 	// that no other function of this instance of this interface has ever
 	// been invoked on.
 	ShouldTransmitAcceptedReport(ctx context.Context, seqNr uint64, reportWithInfo ReportWithInfo[RI]) (bool, error)