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

soft delete nonce in s_consumers so that request IDs do not repeat #12405

Merged
merged 10 commits into from
Mar 14, 2024
Merged

soft delete nonce in s_consumers so that request IDs do not repeat #12405

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4aca8af
soft delete nonce in s_consumers so that request IDs do not repeat. WIP
jinhoonbang Mar 12, 2024
7326acb
forge unit tests passing except for exact billing amount
jinhoonbang Mar 12, 2024
cef7a63
add forge tests and optimize gas
jinhoonbang Mar 12, 2024
75361ea
regenerate wrappers
jinhoonbang Mar 12, 2024
74d44a1
remove comment
jinhoonbang Mar 12, 2024
91717d9
Merge branch 'develop' into vrfv2plus-unique-request-id
jinhoonbang Mar 13, 2024
7f34448
address comments
jinhoonbang Mar 13, 2024
f89c9fc
fix test
jinhoonbang Mar 13, 2024
80feabe
add changeset
jinhoonbang Mar 13, 2024
8dd28f9
Merge branch 'develop' into vrfv2plus-unique-request-id
jinhoonbang Mar 13, 2024
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
17 changes: 12 additions & 5 deletions contracts/src/v0.8/vrf/dev/SubscriptionAPI.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
// consumer is valid without reading all the consumers from storage.
address[] consumers;
}
struct ConsumerConfig {
bool active;
uint64 nonce;
}
// Note a nonce of 0 indicates an the consumer is not assigned to that subscription.
mapping(address => mapping(uint256 => uint64)) /* consumer */ /* subId */ /* nonce */ internal s_consumers;
mapping(address => mapping(uint256 => ConsumerConfig)) /* consumerAddress */ /* subId */ /* consumerConfig */
internal s_consumers;
mapping(uint256 => SubscriptionConfig) /* subId */ /* subscriptionConfig */ internal s_subscriptionConfigs;
mapping(uint256 => Subscription) /* subId */ /* subscription */ internal s_subscriptions;
// subscription nonce used to construct subId. Rises monotonically
Expand Down Expand Up @@ -405,14 +410,16 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr
if (consumers.length == MAX_CONSUMERS) {
revert TooManyConsumers();
}
mapping(uint256 => uint64) storage nonces = s_consumers[consumer];
if (nonces[subId] != 0) {
ConsumerConfig storage consumerConfig = s_consumers[consumer][subId];
if (consumerConfig.active) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we move this check before if (consumers.length == MAX_CONSUMERS) {, since attempting to add same consumer again should be an idempotent operation instead of reverting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense technically. Can change it

// Idempotence - do nothing if already added.
// Ensures uniqueness in s_subscriptions[subId].consumers.
return;
}
// Initialize the nonce to 1, indicating the consumer is allocated.
nonces[subId] = 1;
// consumerConfig.nonce is 0 if the consumer had never sent a request to this subscription
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider starting nonce at 1, to maintain parity with previous implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 0 makes more sense because previously 0 indicated inactive consumer but now, we have a active boolean flag.

Also, we increment nonce before generating the first requestID here. So if we start the nonce at 1 here, first requestID will be generated with nonce 2

Copy link
Contributor

@vreff vreff Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, as long as the boolean is true to start the next SSTORE will not be cold.

// otherwise, consumerConfig.nonce is non-zero
// in both cases, use consumerConfig.nonce as is and set active status to true
consumerConfig.active = true;
consumers.push(consumer);

emit SubscriptionConsumerAdded(subId, consumer);
Expand Down
24 changes: 14 additions & 10 deletions contracts/src/v0.8/vrf/dev/VRFCoordinatorV2_5.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,9 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
}
// Its important to ensure that the consumer is in fact who they say they
// are, otherwise they could use someone else's subscription balance.
// A nonce of 0 indicates consumer is not allocated to the sub.
mapping(uint256 => uint64) storage nonces = s_consumers[msg.sender];
uint64 nonce = nonces[subId];
if (nonce == 0) {
mapping(uint256 => ConsumerConfig) storage consumerConfigs = s_consumers[msg.sender];
ConsumerConfig memory consumerConfig = consumerConfigs[subId];
if (!consumerConfig.active) {
revert InvalidConsumer(subId, msg.sender);
}
// Input validation using the config storage word.
Expand All @@ -291,9 +290,9 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
// Note we do not check whether the keyHash is valid to save gas.
// The consequence for users is that they can send requests
// for invalid keyHashes which will simply not be fulfilled.
++nonce;
++consumerConfig.nonce;
uint256 preSeed;
(requestId, preSeed) = _computeRequestId(req.keyHash, msg.sender, subId, nonce);
(requestId, preSeed) = _computeRequestId(req.keyHash, msg.sender, subId, consumerConfig.nonce);

bytes memory extraArgsBytes = VRFV2PlusClient._argsToBytes(_fromBytes(req.extraArgs));
s_requestCommitments[requestId] = keccak256(
Expand All @@ -318,7 +317,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
extraArgsBytes,
msg.sender
);
nonces[subId] = nonce;
consumerConfigs[subId] = consumerConfig;

return requestId;
}
Expand Down Expand Up @@ -621,7 +620,12 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
for (uint256 i = 0; i < consumersLength; ++i) {
address consumer = consumers[i];
for (uint256 j = 0; j < provingKeyHashesLength; ++j) {
(uint256 reqId, ) = _computeRequestId(s_provingKeyHashes[j], consumer, subId, s_consumers[consumer][subId]);
(uint256 reqId, ) = _computeRequestId(
s_provingKeyHashes[j],
consumer,
subId,
s_consumers[consumer][subId].nonce
);
if (s_requestCommitments[reqId] != 0) {
return true;
}
Expand All @@ -637,7 +641,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
if (pendingRequestExists(subId)) {
revert PendingRequestExists();
}
if (s_consumers[consumer][subId] == 0) {
if (!s_consumers[consumer][subId].active) {
revert InvalidConsumer(subId, consumer);
}
// Note bounded by MAX_CONSUMERS
Expand All @@ -653,7 +657,7 @@ contract VRFCoordinatorV2_5 is VRF, SubscriptionAPI, IVRFCoordinatorV2Plus {
break;
}
}
delete s_consumers[consumer][subId];
s_consumers[consumer][subId].active = false;
emit SubscriptionConsumerRemoved(subId, consumer);
}

Expand Down
17 changes: 9 additions & 8 deletions contracts/src/v0.8/vrf/dev/testhelpers/VRFCoordinatorV2PlusUpgradedVersion.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,9 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
// Its important to ensure that the consumer is in fact who they say they
// are, otherwise they could use someone else's subscription balance.
// A nonce of 0 indicates consumer is not allocated to the sub.
uint64 currentNonce = s_consumers[msg.sender][req.subId];
if (currentNonce == 0) {
mapping(uint256 => ConsumerConfig) storage consumerConfigs = s_consumers[msg.sender];
ConsumerConfig memory consumerConfig = consumerConfigs[req.subId];
if (!consumerConfig.active) {
revert InvalidConsumer(req.subId, msg.sender);
}
// Input validation using the config storage word.
Expand All @@ -267,8 +268,8 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
// Note we do not check whether the keyHash is valid to save gas.
// The consequence for users is that they can send requests
// for invalid keyHashes which will simply not be fulfilled.
uint64 nonce = currentNonce + 1;
(uint256 requestId, uint256 preSeed) = _computeRequestId(req.keyHash, msg.sender, req.subId, nonce);
++consumerConfig.nonce;
(uint256 requestId, uint256 preSeed) = _computeRequestId(req.keyHash, msg.sender, req.subId, consumerConfig.nonce);

VRFV2PlusClient.ExtraArgsV1 memory extraArgs = _fromBytes(req.extraArgs);
bytes memory extraArgsBytes = VRFV2PlusClient._argsToBytes(extraArgs);
Expand All @@ -294,7 +295,7 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
extraArgsBytes,
msg.sender
);
s_consumers[msg.sender][req.subId] = nonce;
s_consumers[msg.sender][req.subId] = consumerConfig;

return requestId;
}
Expand Down Expand Up @@ -548,7 +549,7 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
s_provingKeyHashes[j],
subConfig.consumers[i],
subId,
s_consumers[subConfig.consumers[i]][subId]
s_consumers[subConfig.consumers[i]][subId].nonce
);
if (s_requestCommitments[reqId] != 0) {
return true;
Expand All @@ -565,7 +566,7 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
if (pendingRequestExists(subId)) {
revert PendingRequestExists();
}
if (s_consumers[consumer][subId] == 0) {
if (!s_consumers[consumer][subId].active) {
revert InvalidConsumer(subId, consumer);
}
// Note bounded by MAX_CONSUMERS
Expand Down Expand Up @@ -712,7 +713,7 @@ contract VRFCoordinatorV2PlusUpgradedVersion is
}

for (uint256 i = 0; i < migrationData.consumers.length; i++) {
s_consumers[migrationData.consumers[i]][migrationData.subId] = 1;
s_consumers[migrationData.consumers[i]][migrationData.subId] = ConsumerConfig({active: true, nonce: 0});
}

s_subscriptions[migrationData.subId] = Subscription({
Expand Down
Loading
Loading