[fastx authority] Unify lock checks on order and certificate #313
Conversation
fastpay_core/src/authority.rs
Outdated
| &self, | ||
| order: &Order, | ||
| object_ref: ObjectRef, | ||
| object: Option<Object>, |
There was a problem hiding this comment.
The Option here seems a bit odd because the call will always fail if object is None. Perhaps we should take an Object here and do the "object not found" at the call site?
Would also be less bothered about this if the function was not pub because then it's clear that it's an internal API + that some wonkiness may exist.
There was a problem hiding this comment.
This is an internal API. We are now non-pub for both. And we call with Object, not Option.
fastpay_core/src/authority.rs
Outdated
| } | ||
| fp_ensure!( | ||
| !all_objects.is_empty(), | ||
| FastPayError::InsufficientObjectNumber |
There was a problem hiding this comment.
Pre-existing issue, but I find this error name a bit confusing (sounds like it could refer to object sequence numbers). It looks like we only raise it when there are zero input objects, so perhaps we could rename it to (e.g.) NoObjectInputs? Alternatively, if we also want to raise it in cases where there are too few/too many input objects for the order type, we could call it something like ObjectInputArityViolation.
fastpay_core/src/authority.rs
Outdated
| @@ -300,7 +296,17 @@ impl AuthorityState { | |||
| recipient: Address, | |||
| mut gas_object: Object, | |||
| ) -> FastPayResult<ExecutionStatus> { | |||
| let gas_used = gas::calculate_object_transfer_cost(&inputs[0]); | |||
| if inputs.is_empty() { | |||
There was a problem hiding this comment.
Pre-existing issue, but should we check that the size of inputs is exactly one? If there are "extra" mutable inputs, they will (I think) not appear as outputs in the TransactionEffects and may have order locks set that are never cleared.
There was a problem hiding this comment.
Totally. In fact #314 is about the non-transfer case.
| match self.check_one_lock(order, object_ref, object) { | ||
| Ok((object_ref, object)) => all_objects.push((object_ref, object)), | ||
| Err(e) => { | ||
| errors.push(e); |
There was a problem hiding this comment.
I have some (minor) concerns about this. Returning every error to the client instead of short-circuiting is definitely better from a UX perspective, but it also forces the authority to do extra work that it cannot charge the client for.
However, I'm ok with this because (a) the work done seems fairly cheap, (b) I don't think this change gives an attacker any more DDOS-ing power--even if we kept the short-circuiting policy, the attacker can give the authority a long list of valid objects with an invalid one at the end. But thought I would at least raise this issue.
There was a problem hiding this comment.
My thinking here is similar. Its a trade-off between how much we read from the DB, and how many times an order/cert is submitted to discover all missing objects.
gdanezis
force-pushed
the
clean-up-authority-errors
branch
from
9058c03 to
fc96555
Compare
This PR addresses issue #296 - we use a unified logic to check all locks for an order when we first handle the order and when we handle the certificate. And return all errors relating to missing or wrong version objects, rather than just the first.