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.

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

[Functions] Remove OCR Config Digest check #11249

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

justinkaseman
Copy link
Contributor

No description provided.

@justinkaseman justinkaseman requested a review from a team as a code owner November 9, 2023 22:03
Copy link
Contributor

github-actions bot commented Nov 9, 2023

I see that you haven't updated any README files. Would it make sense to do so?

}

// Bounded by "MaxRequestBatchSize" on the Job's ReportingPluginConfig
for (uint256 i = 0; i < requestIds.length; ++i) {
for (uint256 i = 0; i < numberOfFulfillments; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not mix completely unrelated changes. Send out a separate PR for readability. Same for FunctionsBilling.sol.

@@ -19,9 +19,6 @@ abstract contract OCR2Base is ConfirmedOwner, OCR2Abstract {
i_uniqueReports = uniqueReports;
Copy link
Contributor

Choose a reason for hiding this comment

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

We were planning to remove unique reports, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

// solhint-disable-next-line custom-errors
require(configInfo.latestConfigDigest == configDigest, "configDigest mismatch");
// The following check is disabled to allow both current and proposed routes to submit reports using the same OCR config digest
// Replay attacks are not a risk to Chanlink Functions because
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling out replay attacks you can make a more generic statement that request IDs are globally unique and validated in Coordinator.

Also typo "Chanlink".

@@ -320,8 +316,10 @@ abstract contract OCR2Base is ConfirmedOwner, OCR2Abstract {
emit Transmitted(configDigest, uint32(epochAndRound >> 8));

ConfigInfo memory configInfo = s_configInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used when determining the minimum signers, but I checked it with and without, it's cheaper gas-wise without.

// solhint-disable-next-line custom-errors
require(rs.length == ss.length, "signatures out of registration");
if (rs.length != expectedNumSignatures) revert ReportInvalid("wrong number of signatures");
if (rs.length != ss.length) revert ReportInvalid("signatures out of registration");
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message is confusing to me, what does it mean "out of registration" :) Maybe @stchrysa can help re-word.

@justinkaseman justinkaseman force-pushed the FUN-1053-ocr-config-digest branch from 32ddb86 to 9e40ca8 Compare November 10, 2023 00:57
@justinkaseman justinkaseman requested a review from bolekk November 10, 2023 00:57
@justinkaseman justinkaseman force-pushed the FUN-1053-ocr-config-digest branch from 9e40ca8 to 7ed2b3e Compare November 10, 2023 00:59
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

1 similar comment
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@justinkaseman justinkaseman force-pushed the FUN-1053-ocr-config-digest branch 2 times, most recently from d415dab to 865a47d Compare November 10, 2023 01:35
bolekk
bolekk previously approved these changes Nov 10, 2023
@@ -151,7 +151,9 @@ contract FunctionsCoordinator is OCR2Base, IFunctionsCoordinator, FunctionsBilli
numberOfFulfillments != onchainMetadata.length ||
numberOfFulfillments != offchainMetadata.length
) {
revert ReportInvalid();
revert ReportInvalid(
"All fields on the report must be equal: requestIds, results, errors, onchainMetadata, offchainMetadata"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "of equal length"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@justinkaseman justinkaseman added this pull request to the merge queue Nov 10, 2023
Merged via the queue into develop with commit 96ae30c Nov 10, 2023
106 checks passed
@justinkaseman justinkaseman deleted the FUN-1053-ocr-config-digest branch November 10, 2023 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants