Check round ids #15183

eduard-cl · 2024-11-11T13:02:59Z

Add same epochAndRound check to the contract and _checkRounds() to the tests.

github-actions · 2024-11-11T13:03:46Z

I see you updated files related to contracts. Please run pnpm changeset in the contracts directory to add a changeset.

github-actions · 2024-11-11T13:04:33Z

Static analysis results are available

Hey @eduard-cl, you can view Slither reports in the job summary here or download them as artifact here.
Please check them before merging and make sure you have addressed all issues.

github-actions · 2024-11-11T13:05:10Z

AER Report: Operator UI CI

aer_workflow , commit , Breaking Changes GQL Check

1. Workflow conclusion is failure:[convictional/trigger-workflow-and-wait@`f69fa9e`]

Source of Error:

Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2024-11-12T15:14:51.9383232Z Checking conclusion [failure]
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2024-11-12T15:14:51.9384184Z Checking status [completed]
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2024-11-12T15:14:51.9385085Z Conclusion is not success, it's [failure].
Run convictional/trigger-workflow-and-wait@f69fa9eedd3c62a599220f4d5745230e237904be	2024-11-12T15:14:51.9386458Z Propagating failure to upstream job

Why: The triggered workflow did not complete successfully. The conclusion status was "failure" instead of "success," causing the upstream job to fail as well.

Suggested fix: Investigate the logs of the triggered workflow (ID: 11800155115) to identify the specific cause of failure and address the underlying issue. Ensure all dependencies and configurations are correct.

AER Report: CI Core ran successfully ✅

aer_workflow , commit

contracts/src/v0.8/feeds/DualAggregator.sol

Oozyx · 2024-11-11T14:07:37Z

Only checking the epochAndRound doesn't work. We can end up with a situation where the transmitter constantly resends the same transmission and it won't revert.

Oozyx · 2024-11-11T21:44:06Z

contracts/src/v0.8/feeds/DualAggregator.sol

+      // Verify signatures attached to report
+      _verifySignatures(reportContext, report, rs, ss, rawVs);


Just realized we should always verify signatures. This should be placed under _validateReport

Why? To optimize gas costs it's better if we only verify the signatures one time per report.

It's an attack vector. You can provide existing reports to the secondary feed that were not signed.

Yes, but the existing reports have to have the same timestamp and answer than the one already validated. I thought the biggest benefit of this implementation was to not have to double check the signatures if a report has previously arrived by the other aggregator path.

contracts/src/v0.8/feeds/DualAggregator.sol

alejoberardino · 2024-11-11T22:16:40Z

contracts/src/v0.8/feeds/DualAggregator.sol

+    }
+
+    // Lock/unlock the primary feed and pay the transmitters
+    s_primaryLocked = isSecondary;


This is not easy to understand, as we're only locking for a single block. Let's change this variable to something more direct, saying that the last update was from the secondary feed, and then communicate the implications of this on the reading side

That's true, renamed it to s_latestSecondary

…nal-tests Add tests

Oozyx · 2024-11-12T14:18:51Z

contracts/src/v0.8/feeds/DualAggregator.sol

+    if (epochAndRound != s_hotVars.latestEpochAndRound || !s_latestSecondary) {
+      if (epochAndRound <= s_hotVars.latestEpochAndRound) {
+        revert StaleReport();
+      }


I'm finding these conditions hard to follow. Can you include some comments explaining what different edge cases we are validating for?

Added some comments, it's true it's a bit hard to understand, because I inverted some conditionals to not have duplicated code. To help you understand the code, it's the same as this:

// In case the report is the same as the latest and it arrived throw the secondary feed, // we have to validate it without transmitting a duplicated report if (epochAndRound == s_hotVars.latestEpochAndRound && s_latestSecondary) { s_latestSecondary = isSecondary; _payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender); return; } // revert because it's older or already arrived throw the primary feed too (if not we would // have match the previous check) if (epochAndRound <= s_hotVars.latestEpochAndRound) { revert StaleReport(); } _report(s_hotVars, reportContext[0], epochAndRound, report_, isSecondary); // Store if the latest report was secondary or not s_latestSecondary = isSecondary; _payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);

The only difference is that I inverted the first conditional to not have this lines duplicated:

s_latestSecondary = isSecondary; _payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);

Oozyx · 2024-11-12T14:35:08Z

contracts/src/v0.8/feeds/DualAggregator.sol

        s_hotVars.latestSecondaryRoundId = roundId;
        emit SecondaryRoundIdUpdated(roundId);

        _payTransmitter(s_hotVars, report_.juelsPerFeeCoin, uint32(initialGas), msg.sender);
        return;
      }
-      // In case the report doesn't exist, lock the primary feed
-      s_primaryLocked = true;
-    } else {


suggestion: I think we could keep this else and use it for the case where the primary feed deals with receiving an existing, but latest report.

if (epochAndRound == s_hotVars.latestEpochAndRound && s_primaryLocked))

I see this if, else block as where the logic is evaluated for all edge cases where an existing report is sent to either feed. Early returns would happen here as well.

The rest of the function would be to handle and record new reports.

This is very subjective so feel free to disagree.

Could you develop more this idea? Not sure I'm following how it should be implemented.

cl-sonarqube-production · 2024-11-12T15:32:27Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

pszal · 2024-11-14T12:33:46Z

contracts/src/v0.8/feeds/DualAggregator.sol

@@ -804,37 +804,46 @@ contract DualAggregator is OCR2Abstract, OwnerIsCreator, AggregatorV2V3Interface
    // Validate the report data
    _validateReport(reportContext, report, rs, ss);

+    // Verify signatures attached to report
+    _verifySignatures(reportContext, report, rs, ss, rawVs);


Why it got moved? I have to be missing something, but isn't the main gas saving due to skipping this step if the report is already recorded?

eduard-cl added 2 commits November 11, 2024 14:00

check rounds

d7c6f01

forge fmt

b67bcdc

eduard-cl requested a review from a team as a code owner November 11, 2024 13:03

Oozyx reviewed Nov 11, 2024

View reviewed changes

contracts/src/v0.8/feeds/DualAggregator.sol Outdated Show resolved Hide resolved

eduard-cl and others added 2 commits November 11, 2024 15:30

modify transmit function

cc60767

Add tests

037dd75

Oozyx reviewed Nov 11, 2024

View reviewed changes

contracts/src/v0.8/feeds/DualAggregator.sol Show resolved Hide resolved

alejoberardino reviewed Nov 11, 2024

View reviewed changes

eduard-cl and others added 3 commits November 12, 2024 12:23

Merge pull request #15193 from smartcontractkit/feeds-project-additio…

3067d87

…nal-tests Add tests

transmit function improved

ba122ad

improve transmit logic + add 2 tests

674785d

Oozyx reviewed Nov 12, 2024

View reviewed changes

add comments + verify signatures early

c112e39

Oozyx approved these changes Nov 12, 2024

View reviewed changes

Oozyx merged commit 2a5cb74 into feeds-project-develop Nov 12, 2024
84 of 103 checks passed

Oozyx deleted the feeds-project-rounds-fix branch November 12, 2024 16:04

pszal reviewed Nov 14, 2024

View reviewed changes

eduard-cl mentioned this pull request Nov 14, 2024

Transmit function #15091

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check round ids #15183

Check round ids #15183

eduard-cl commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

Oozyx commented Nov 11, 2024

Oozyx Nov 11, 2024

eduard-cl Nov 12, 2024

Oozyx Nov 12, 2024

eduard-cl Nov 12, 2024

alejoberardino Nov 11, 2024

eduard-cl Nov 12, 2024

Oozyx Nov 12, 2024

eduard-cl Nov 12, 2024

Oozyx Nov 12, 2024 •

edited

Loading

eduard-cl Nov 12, 2024

cl-sonarqube-production bot commented Nov 12, 2024

pszal Nov 14, 2024

		// Verify signatures attached to report
		_verifySignatures(reportContext, report, rs, ss, rawVs);

Check round ids #15183

Check round ids #15183

Conversation

eduard-cl commented Nov 11, 2024

github-actions bot commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 • edited Loading

Static analysis results are available

github-actions bot commented Nov 11, 2024 • edited Loading

AER Report: Operator UI CI

1. Workflow conclusion is failure:[convictional/trigger-workflow-and-wait@f69fa9e]

AER Report: CI Core ran successfully ✅

Oozyx commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oozyx Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Nov 12, 2024

Quality Gate passed

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

1. Workflow conclusion is failure:[convictional/trigger-workflow-and-wait@`f69fa9e`]

Oozyx Nov 12, 2024 •

edited

Loading