Fix MinObservation to make observations per observer unique [CCIP-5050] #495
Make new OracleMinObservation for exec plugin consensus
- Loading branch information
commit fd06a9c1d080e83d5667c513c7377d58f14c44a3
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package consensus | ||
|
|
||
| import ( | ||
| "fmt" | ||
| cciptypes "github.com/smartcontractkit/chainlink-ccip/pkg/types/ccipocr3" | ||
| "github.com/smartcontractkit/libocr/commontypes" | ||
| "golang.org/x/crypto/sha3" | ||
| ) | ||
|
|
||
| type observersCounter[T any] struct { | ||
| data T | ||
| observers map[commontypes.OracleID]struct{} | ||
| } | ||
|
|
||
| // OracleMinObservation provides a way to ensure a minimum number of observations for | ||
| // some piece of data have occurred. It maintains an internal cache and provides a list | ||
| // of valid or invalid data points. | ||
| type OracleMinObservation[T any] interface { | ||
| Add(data T, oracleID commontypes.OracleID) | ||
| GetValid() []T | ||
asoliman92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // minObservation is a helper object to filter data based on observation counts. | ||
| // It keeps track of all inputs, determines if they are consistent | ||
| // with one another, and whether they meet the required count threshold. | ||
asoliman92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| type oracleMinObservation[T any] struct { | ||
| minObservation Threshold | ||
| cache map[cciptypes.Bytes32]*observersCounter[T] | ||
| idFunc func(T) [32]byte | ||
| } | ||
|
|
||
| // NewOracleMinObservation constructs a concrete MinObservation object. The | ||
| // supplied idFunc is used to generate a uniqueID for the type being observed. | ||
asoliman92 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| func NewOracleMinObservation[T any](minThreshold Threshold, idFunc func(T) [32]byte) OracleMinObservation[T] { | ||
| if idFunc == nil { | ||
| idFunc = func(data T) [32]byte { | ||
| return sha3.Sum256([]byte(fmt.Sprintf("%v", data))) | ||
| } | ||
| } | ||
|
Comment on lines
+39
to
+43
There was a problem hiding this comment. Thinking out loud, but could this version be implemented by overloading the ID func? There was a problem hiding this comment. thought about this and tried it, was't straightforward to use from the callers' though. don't remember the specific reason for it now. |
||
| return &oracleMinObservation[T]{ | ||
| minObservation: minThreshold, | ||
| cache: make(map[cciptypes.Bytes32]*observersCounter[T]), | ||
| idFunc: idFunc, | ||
| } | ||
| } | ||
|
|
||
| func (cv *oracleMinObservation[T]) Add(data T, oracleID commontypes.OracleID) { | ||
| id := cv.idFunc(data) | ||
| if _, ok := cv.cache[id]; ok { | ||
| cv.cache[id].observers[oracleID] = struct{}{} | ||
| } else { | ||
| cv.cache[id] = &observersCounter[T]{data: data, observers: make(map[commontypes.OracleID]struct{})} | ||
| } | ||
| } | ||
|
|
||
| func (cv *oracleMinObservation[T]) GetValid() []T { | ||
| var validated []T | ||
| for _, rc := range cv.cache { | ||
| if len(rc.observers) >= int(cv.minObservation) { | ||
| rc := rc | ||
| validated = append(validated, rc.data) | ||
| } | ||
| } | ||
| return validated | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| package consensus_test | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "github.com/smartcontractkit/libocr/commontypes" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "golang.org/x/crypto/sha3" | ||
|
|
||
| "github.com/smartcontractkit/chainlink-ccip/execute/exectypes" | ||
| "github.com/smartcontractkit/chainlink-ccip/internal/plugincommon/consensus" | ||
| cciptypes "github.com/smartcontractkit/chainlink-ccip/pkg/types/ccipocr3" | ||
| ) | ||
|
|
||
| func Test_CommitReportValidator_Oracle_ExecutePluginCommitData(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| min consensus.Threshold | ||
| reports []exectypes.CommitData | ||
| valid []exectypes.CommitData | ||
| }{ | ||
| { | ||
| name: "empty", | ||
| valid: nil, | ||
| }, | ||
| { | ||
| name: "single report, enough observations", | ||
| min: 1, | ||
| reports: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}}, | ||
| }, | ||
| valid: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "single report, not enough observations", | ||
| min: 2, | ||
| reports: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}}, | ||
| }, | ||
| valid: nil, | ||
| }, | ||
| { | ||
| name: "multiple reports, partial observations", | ||
| min: 2, | ||
| reports: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{3}}, | ||
| {MerkleRoot: [32]byte{1}}, | ||
| {MerkleRoot: [32]byte{2}}, | ||
| {MerkleRoot: [32]byte{1}}, | ||
| {MerkleRoot: [32]byte{2}}, | ||
| }, | ||
| valid: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}}, | ||
| {MerkleRoot: [32]byte{2}}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "multiple reports for same root", | ||
| min: 2, | ||
| reports: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 1}, | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 2}, | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 3}, | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 4}, | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 1}, | ||
| }, | ||
| valid: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}, BlockNum: 1}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "different executed messages same root", | ||
| min: 2, | ||
| reports: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{1, 2}}, | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{2, 3}}, | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{3, 4}}, | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{4, 5}}, | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{5, 6}}, | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{1, 2}}, | ||
| }, | ||
| valid: []exectypes.CommitData{ | ||
| {MerkleRoot: [32]byte{1}, ExecutedMessages: []cciptypes.SeqNum{1, 2}}, | ||
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Initialize the minObservation | ||
| idFunc := func(data exectypes.CommitData) [32]byte { | ||
| return sha3.Sum256([]byte(fmt.Sprintf("%v", data))) | ||
| } | ||
| validator := consensus.NewOracleMinObservation[exectypes.CommitData](tt.min, idFunc) | ||
| for i, report := range tt.reports { | ||
| validator.Add(report, commontypes.OracleID(i)) | ||
| } | ||
|
|
||
| // Test the results | ||
| got := validator.GetValid() | ||
| if !assert.ElementsMatch(t, got, tt.valid) { | ||
| t.Errorf("GetValid() = %v, valid %v", got, tt.valid) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_CommitReportValidator_Oracle_Generics(t *testing.T) { | ||
| type Generic struct { | ||
| number int | ||
| } | ||
|
|
||
| // Initialize the minObservation | ||
| idFunc := func(data Generic) [32]byte { | ||
| return sha3.Sum256([]byte(fmt.Sprintf("%v", data))) | ||
| } | ||
| validator := consensus.NewOracleMinObservation[Generic](2, idFunc) | ||
|
|
||
| wantValue := Generic{number: 1} | ||
| otherValue := Generic{number: 2} | ||
|
|
||
| validator.Add(wantValue, 1) | ||
| validator.Add(wantValue, 2) | ||
| validator.Add(otherValue, 3) | ||
|
|
||
| // Test the results | ||
|
|
||
| wantValid := []Generic{wantValue} | ||
| got := validator.GetValid() | ||
| if !assert.ElementsMatch(t, got, wantValid) { | ||
| t.Errorf("GetValid() = %v, valid %v", got, wantValid) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API doc comment should outline why this would be used instead of
MinObservation. I think you can say thatOracleMinObservationensures that duplicate observations from the same observer are only counted once? TBH this should be the default behavior, I'm not even sure we should have a separate MinObservation and OracleMinObservation.cc @winder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in DM I agree we should have one. It's a big change that will take some time and other plugins already use maps for most observations which by design prevents duplicates.