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

[Client Refactoring 1] Extract authority missing certificate update out of order handling pipeline #356

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 4, 2022

Step 1 towards the big Client refactoring (#275).
In the existing implementation, during any quorum communication (i.e. both calling handle_order and calling handle_confirmation_order to a quorum of authorities), for each input object, we inspect the current sequence number of each authority in the quorum. If they are out-of-sync, we provide them the missing certificates at that time.
This implementation is strange in a few ways:

  1. It is completely unnecessary to do this twice, once during handle_order and once during handle_confirmation_order. We should only need to do this once at the very beginning.
  2. It makes the logic really complicated by doing this while we are also trying to handle order at the same time.
  3. It makes the result of any order handling impossible to interpret, because it returns the result of handling not just the current order, but also all the past missing certificates.
  4. It looks messy.

This PR extracts the missing certificate update logic out to a separate function, and we do this once explicitly at the beginning of execution_transaction.

@lxfind lxfind force-pushed the no-certificates-state branch 3 times, most recently from 7ee3884 to 06e2df1 Compare February 4, 2022 04:11
@lxfind lxfind changed the title [Client Refactoring] Extract authority missing certificate update out of order handling pipeline [Client Refactoring 1] Extract authority missing certificate update out of order handling pipeline Feb 4, 2022
@lxfind lxfind force-pushed the no-certificates-state branch from 06e2df1 to 9d11510 Compare February 4, 2022 06:23
@oxade
Copy link
Contributor

oxade commented Feb 4, 2022

@lxfind nice! Thanks for jumping on this.
You might want to look at some refactoring and modularization work @gdanezis is doing here in case of overlaps/conflicts.
I think there's a lot of similarities and I particular like the MapReduce view in George proposes.
#336

@gdanezis
Copy link
Collaborator

gdanezis commented Feb 4, 2022

As I pointed out in #290 and #252 this logic for updating authorities is incomplete and incorrect when it comes to object creation and deletion. My latest PR #336 starts adding logic to make these more robust, that @patrickkuo and I will be linking to the client functions. I guess all this to say: do not worry about the current logic too much, its likely to change radically.

Having said that, I am not against restructuring things, and then using the new structure to add the correct functionality.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

As I say in the comments, I have no objection landing this (in particular if you depend on it for subsequent PRs) but a lot of the logic will be changing very soon.

inner_signed_order.authority == name,
FastPayError::ErrorWhileProcessingTransferOrder
);
inner_signed_order.check(&committee)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be checking data that comes from authorities in a more centralized way, ie within the AuthorityClient logic, rather that at each point the returns are used. This is probably for a separate PR, you are just doing here what the code did.

@@ -952,15 +974,16 @@ where
&mut self,
order: Order,
) -> Result<(CertifiedOrder, OrderEffects), anyhow::Error> {
let inputs = order.input_objects();
let known_certificates = self.get_known_certificates(order.sender(), &inputs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The key bug is here: to ensure that an order can be processed it is not sufficient (any more) to ensure that all past certificates relating to the input objects are uploaded, because processing these orders may in turn require orders that do not involve the input objects to be processed (but that lead to the creation of the objects). All this logic has to go, and be replaced by sync_certificate_to_authority_with_timeout and sync_authority_source_to_destination which correctly walk the dependency graph backwards.

@lxfind lxfind merged commit 47d2a6a into main Feb 4, 2022
@lxfind lxfind deleted the no-certificates-state branch February 4, 2022 15:48
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.

3 participants