chore: Add global lock for execute_rental_request_proposal, fix guards overriding each other#83
chore: Add global lock for execute_rental_request_proposal, fix guards overriding each other#83
Conversation
andrew-lee-work
left a comment
There was a problem hiding this comment.
I left a comment on the new guard in execute_rental_request_proposal.
| let _guard_request = | ||
| CallerGuard::new(user, "request").expect("Fatal: Concurrent call on user"); | ||
| let _guard_subnet = CallerGuard::new(Principal::anonymous(), "nns") | ||
| .expect("Fatal: Concurrent call on subnet"); |
There was a problem hiding this comment.
It looks like guard_subnet is scoped to execute_rental_request_proposal. In that case it would make more sense to use a tag specific to the function instead of sharing a tag with the subnet locks. (Similar to how refund uses the "refund" tag which is specific to it.)
I think there should also be a lock on the subnet, so there's only one update on the subnet at a time. That would look like:
let _guard_subnet =
CallerGuard::new(payload.subnet_id, "nns").expect("Fatal: Concurrent call on subnet");
(Copied from below.) Perhaps that's actually what was intended here, since it's called "guard_subnet", not "guard_method".
There was a problem hiding this comment.
You're right, the _guard_subnet is a mistake. But the subnet_id is not available yet, so I opted for the global lock called "nns" (which makes sure that there cannot be concurrent calls to this function).
In addition to NNS's check that there can be only one Rental Request at a time, the SRC also enforces that there can be no concurrent execution of execute_rental_request_proposal. Also give guards different names if there are more than one to prevent them freeing each other because they get overridden.