Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra sanity checks to config poller and contract transmitter #10593

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Add extra sanity checks to config poller and contract transmitter #10593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/services/relay/evm/config_poller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ func (cp *configPoller) LatestConfig(ctx context.Context, changedInBlock uint64)
if err != nil {
return ocrtypes.ContractConfig{}, err
}
if len(lgs) == 0 {
return ocrtypes.ContractConfig{}, errors.New("no logs found")
}
latestConfigSet, err := configFromLog(lgs[len(lgs)-1].Data)
if err != nil {
return ocrtypes.ContractConfig{}, err
Expand Down
10 changes: 6 additions & 4 deletions core/services/relay/evm/config_poller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ func TestConfigPoller(t *testing.T) {
lp := logpoller.NewLogPoller(lorm, ethClient, lggr, 100*time.Millisecond, 1, 2, 2, 1000)
require.NoError(t, lp.Start(ctx))
t.Cleanup(func() { lp.Close() })
logPoller, err := NewConfigPoller(lggr, lp, ocrAddress)
configPoller, err := NewConfigPoller(lggr, lp, ocrAddress)
require.NoError(t, err)
// Should have no config to begin with.
_, config, err := logPoller.LatestConfigDetails(testutils.Context(t))
_, config, err := configPoller.LatestConfigDetails(testutils.Context(t))
require.NoError(t, err)
require.Equal(t, ocrtypes2.ConfigDigest{}, config)
_, err = configPoller.LatestConfig(testutils.Context(t), 0)
require.Error(t, err)
// Set the config
contractConfig := setConfig(t, median.OffchainConfig{
AlphaReportInfinite: false,
Expand All @@ -89,13 +91,13 @@ func TestConfigPoller(t *testing.T) {
var digest [32]byte
gomega.NewGomegaWithT(t).Eventually(func() bool {
b.Commit()
configBlock, digest, err = logPoller.LatestConfigDetails(testutils.Context(t))
configBlock, digest, err = configPoller.LatestConfigDetails(testutils.Context(t))
require.NoError(t, err)
return ocrtypes2.ConfigDigest{} != digest
}, testutils.WaitTimeout(t), 100*time.Millisecond).Should(gomega.BeTrue())

// Assert the config returned is the one we configured.
newConfig, err := logPoller.LatestConfig(testutils.Context(t), configBlock)
newConfig, err := configPoller.LatestConfig(testutils.Context(t), configBlock)
require.NoError(t, err)
// Note we don't check onchainConfig, as that is populated in the contract itself.
assert.Equal(t, digest, [32]byte(newConfig.ConfigDigest))
Expand Down
6 changes: 6 additions & 0 deletions core/services/relay/evm/contract_transmitter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ func (oc *contractTransmitter) Transmit(ctx context.Context, reportCtx ocrtypes.
var rs [][32]byte
var ss [][32]byte
var vs [32]byte
if len(signatures) > 32 {
return errors.New("too many signatures, maximum is 32")
}
for i, as := range signatures {
r, s, v, err := evmutil.SplitSignature(as.Signature)
if err != nil {
Expand Down Expand Up @@ -138,6 +141,9 @@ func parseTransmitted(log []byte) ([32]byte, uint32, error) {
if err != nil {
return [32]byte{}, 0, err
}
if len(transmitted) < 2 {
return [32]byte{}, 0, errors.New("transmitted event log has too few arguments")
}
configDigest := *abi.ConvertType(transmitted[0], new([32]byte)).(*[32]byte)
epoch := *abi.ConvertType(transmitted[1], new(uint32)).(*uint32)
return configDigest, epoch, err
Expand Down
3 changes: 3 additions & 0 deletions core/services/relay/evm/functions/config_poller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ func (cp *configPoller) LatestConfig(ctx context.Context, changedInBlock uint64)
if err != nil {
return ocrtypes.ContractConfig{}, err
}
if len(lgs) == 0 {
return ocrtypes.ContractConfig{}, errors.New("no logs found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this always indicate an error or should we just warn here? For example, what if polling frequency is set to be more frequent than the rate of block production?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polling frequency doesn't matter. This method is used to fetch the latest config, with a known block number retrieved from LatestConfigDetails(). In fact this check should never fail and it likely never did in prod, otherwise we would've seen panics about underflow from the line below.

This PR is just being extra safe and having explicit errors.

}
latestConfigSet, err := configFromLog(lgs[len(lgs)-1].Data, cp.pluginType)
if err != nil {
return ocrtypes.ContractConfig{}, err
Expand Down
2 changes: 2 additions & 0 deletions core/services/relay/evm/functions/config_poller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ func runTest(t *testing.T, pluginType functions.FunctionsPluginType, expectedDig
_, config, err := configPoller.LatestConfigDetails(testutils.Context(t))
require.NoError(t, err)
require.Equal(t, ocrtypes2.ConfigDigest{}, config)
_, err = configPoller.LatestConfig(testutils.Context(t), 0)
require.Error(t, err)

pluginConfig := &functionsConfig.ReportingPluginConfigWrapper{
Config: &functionsConfig.ReportingPluginConfig{
Expand Down
6 changes: 6 additions & 0 deletions core/services/relay/evm/functions/contract_transmitter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ func (oc *contractTransmitter) Transmit(ctx context.Context, reportCtx ocrtypes.
var rs [][32]byte
var ss [][32]byte
var vs [32]byte
if len(signatures) > 32 {
return errors.New("too many signatures, maximum is 32")
}
for i, as := range signatures {
r, s, v, err := evmutil.SplitSignature(as.Signature)
if err != nil {
Expand Down Expand Up @@ -169,6 +172,9 @@ func parseTransmitted(log []byte) ([32]byte, uint32, error) {
if err != nil {
return [32]byte{}, 0, err
}
if len(transmitted) < 2 {
return [32]byte{}, 0, errors.New("transmitted event log has too few arguments")
}
configDigest := *abi.ConvertType(transmitted[0], new([32]byte)).(*[32]byte)
epoch := *abi.ConvertType(transmitted[1], new(uint32)).(*uint32)
return configDigest, epoch, err
Expand Down
8 changes: 8 additions & 0 deletions core/services/relay/evm/functions/contract_transmitter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,14 @@ func TestContractTransmitter_Transmit_V1(t *testing.T) {
reportBytes, err := codec.EncodeReport(processedRequests)
require.NoError(t, err)

// success
require.NoError(t, ot.Transmit(testutils.Context(t), ocrtypes.ReportContext{}, reportBytes, []ocrtypes.AttributedOnchainSignature{}))
require.Equal(t, coordinatorAddress, ocrTransmitter.toAddress)

// failure on too many signatures
signatures := []ocrtypes.AttributedOnchainSignature{}
for i := 0; i < 33; i++ {
signatures = append(signatures, ocrtypes.AttributedOnchainSignature{})
}
require.Error(t, ot.Transmit(testutils.Context(t), ocrtypes.ReportContext{}, reportBytes, signatures))
}