Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@rstout
rstout committed Aug 15, 2024
1 parent 3afd871 commit 8e221bb
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 99 deletions.
9 changes: 5 additions & 4 deletions commitrmnocb/factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
)

const maxReportTransmissionCheckAttempts = 5
const maxQueryLength = 1024 * 1024 // 1MB

// PluginFactoryConstructor implements common OCR3ReportingPluginClient and is used for initializing a plugin factory
// and a validation service.
Expand Down Expand Up @@ -129,10 +130,10 @@ func (p *PluginFactory) NewReportingPlugin(config ocr3types.ReportingPluginConfi
), ocr3types.ReportingPluginInfo{
Name: "CCIPRoleCommit",
Limits: ocr3types.ReportingPluginLimits{
MaxQueryLength: 1024 * 1024, // 1MB
MaxObservationLength: 20_000, // 20kB
MaxOutcomeLength: 10_000, // 10kB
MaxReportLength: 10_000, // 10kB
MaxQueryLength: maxQueryLength,
MaxObservationLength: 20_000, // 20kB
MaxOutcomeLength: 10_000, // 10kB
MaxReportLength: 10_000, // 10kB
MaxReportCount: 10,
},
}, nil
Expand Down
75 changes: 40 additions & 35 deletions commitrmnocb/observation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ func (p *Plugin) Observation(
case SelectingRangesForReport:
offRampNextSeqNums := p.observer.ObserveOffRampNextSeqNums(ctx)
observation = Observation{
// TODO: observe OnRamp max seq nums. The use of offRampNextSeqNums here effectively disables batching,
// e.g. the ranges selected for each chain will be [x, x] (e.g. [46, 46]), which means reports will only
// contain one message per chain. Querying the OnRamp contract requires changes to reader.CCIP, which will
// need to be done in a future change.
OnRampMaxSeqNums: offRampNextSeqNums,
OffRampNextSeqNums: offRampNextSeqNums,
FChain: p.observer.ObserveFChain(),
Expand All @@ -54,7 +58,7 @@ func (p *Plugin) Observation(
}

default:
p.lggr.Warnw("Unexpected state", "state", nextState)
p.lggr.Errorw("Unexpected state", "state", nextState)
return observation.Encode()
}

Expand Down Expand Up @@ -93,33 +97,33 @@ func (o ObserverImpl) ObserveOffRampNextSeqNums(ctx context.Context) []plugintyp
return nil
}

if supportsDestChain {
sourceChains, err := o.chainSupport.KnownSourceChainsSlice()
if err != nil {
o.lggr.Warnw("call to KnownSourceChainsSlice failed", "err", err)
return nil
}
offRampNextSeqNums, err := o.ccipReader.NextSeqNum(ctx, sourceChains)
if err != nil {
o.lggr.Warnw("call to NextSeqNum failed", "err", err)
return nil
}
if !supportsDestChain {
return nil
}

if len(offRampNextSeqNums) != len(sourceChains) {
o.lggr.Warnf("call to NextSeqNum returned unexpected number of seq nums, got %d, expected %d",
len(offRampNextSeqNums), len(sourceChains))
return nil
}
sourceChains, err := o.chainSupport.KnownSourceChainsSlice()
if err != nil {
o.lggr.Warnw("call to KnownSourceChainsSlice failed", "err", err)
return nil
}
offRampNextSeqNums, err := o.ccipReader.NextSeqNum(ctx, sourceChains)
if err != nil {
o.lggr.Warnw("call to NextSeqNum failed", "err", err)
return nil
}

result := make([]plugintypes.SeqNumChain, len(sourceChains))
for i := range sourceChains {
result[i] = plugintypes.SeqNumChain{ChainSel: sourceChains[i], SeqNum: offRampNextSeqNums[i]}
}
if len(offRampNextSeqNums) != len(sourceChains) {
o.lggr.Errorf("call to NextSeqNum returned unexpected number of seq nums, got %d, expected %d",
len(offRampNextSeqNums), len(sourceChains))
return nil
}

return result
result := make([]plugintypes.SeqNumChain, len(sourceChains))
for i := range sourceChains {
result[i] = plugintypes.SeqNumChain{ChainSel: sourceChains[i], SeqNum: offRampNextSeqNums[i]}
}

return nil
return result
}

// ObserveMerkleRoots computes the merkle roots for the given sequence number ranges
Expand All @@ -139,19 +143,19 @@ func (o ObserverImpl) ObserveMerkleRoots(
msgs, err := o.ccipReader.MsgsBetweenSeqNums(ctx, chainRange.ChainSel, chainRange.SeqNumRange)
if err != nil {
o.lggr.Warnw("call to MsgsBetweenSeqNums failed", "err", err)
} else {
root, err := o.computeMerkleRoot(ctx, msgs)
if err != nil {
o.lggr.Warnw("call to computeMerkleRoot failed", "err", err)
} else {
merkleRoot := cciptypes.MerkleRootChain{
ChainSel: chainRange.ChainSel,
SeqNumsRange: chainRange.SeqNumRange,
MerkleRoot: root,
}
roots = append(roots, merkleRoot)
}
continue
}
root, err := o.computeMerkleRoot(ctx, msgs)
if err != nil {
o.lggr.Warnw("call to computeMerkleRoot failed", "err", err)
continue
}
merkleRoot := cciptypes.MerkleRootChain{
ChainSel: chainRange.ChainSel,
SeqNumsRange: chainRange.SeqNumRange,
MerkleRoot: root,
}
roots = append(roots, merkleRoot)
}
}

Expand Down Expand Up @@ -183,6 +187,7 @@ func (o ObserverImpl) computeMerkleRoot(ctx context.Context, msgs []cciptypes.Me
hashes = append(hashes, msgHash)
}

// TODO: Do not hard code the hash function, it should be derived from the message hasher
tree, err := merklemulti.NewTree(hashutil.NewKeccak(), hashes)
if err != nil {
return [32]byte{}, fmt.Errorf("failed to construct merkle tree from %d leaves: %w", len(hashes), err)
Expand Down
25 changes: 6 additions & 19 deletions commitrmnocb/observation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func Test_ObserveOffRampNextSeqNums(t *testing.T) {
o := ObserverImpl{
nodeID: nodeID,
lggr: logger.Test(t),
msgHasher: NewMessageHasher(),
msgHasher: mocks.NewMessageHasher(),
ccipReader: reader,
chainSupport: chainSupport,
}
Expand Down Expand Up @@ -449,7 +449,7 @@ func Test_ObserveMerkleRoots(t *testing.T) {
o := ObserverImpl{
nodeID: nodeID,
lggr: logger.Test(t),
msgHasher: NewMessageHasher(),
msgHasher: mocks.NewMessageHasher(),
ccipReader: reader,
chainSupport: chainSupport,
}
Expand Down Expand Up @@ -481,7 +481,7 @@ func Test_computeMerkleRoot(t *testing.T) {
MessageID: mustNewMessageID("0x1a"),
SequenceNumber: 112,
}},
messageHasher: NewMessageHasher(),
messageHasher: mocks.NewMessageHasher(),
expMerkleRoot: "1a00000000000000000000000000000000000000000000000000000000000000",
expErr: false,
},
Expand All @@ -500,7 +500,7 @@ func Test_computeMerkleRoot(t *testing.T) {
MessageID: mustNewMessageID("0x87"),
SequenceNumber: 114,
}},
messageHasher: NewMessageHasher(),
messageHasher: mocks.NewMessageHasher(),
expMerkleRoot: "94c7e711e6f2acf41dca598ced55b6925e55aaed83520dc5ea6cbc054344564b",
expErr: false,
},
Expand All @@ -515,14 +515,14 @@ func Test_computeMerkleRoot(t *testing.T) {
MessageID: mustNewMessageID("0x12"),
SequenceNumber: 36,
}},
messageHasher: NewMessageHasher(),
messageHasher: mocks.NewMessageHasher(),
expMerkleRoot: "",
expErr: true,
},
{
name: "Empty messages",
messageHeaders: []cciptypes.RampMessageHeader{},
messageHasher: NewMessageHasher(),
messageHasher: mocks.NewMessageHasher(),
expMerkleRoot: "",
expErr: true,
},
Expand Down Expand Up @@ -580,19 +580,6 @@ func mustNewMessageID(msgIDHex string) cciptypes.Bytes32 {
return msgID
}

type MessageHasher struct{}

func NewMessageHasher() *MessageHasher {
return &MessageHasher{}
}

func (m *MessageHasher) Hash(ctx context.Context, msg cciptypes.Message) (cciptypes.Bytes32, error) {
// simply return the msg id as bytes32
var b32 [32]byte
copy(b32[:], msg.Header.MessageID[:])
return b32, nil
}

type BadMessageHasher struct{}

func NewBadMessageHasher() *BadMessageHasher {
Expand Down
Loading

0 comments on commit 8e221bb

Please sign in to comment.