[fastx client] Add logic for a client to sync a cert to an authority, and two authorities.

[gdanezis]

This addresses issue #252

• Fix an idempotency bug in authority.
• Started sync logic between two clients.
• Add function to sample authority and limit time used.
• Test the sync protocol vanilla.
• Test sync protocol with deletions (blocked due to filled bug).

Out of scope for the moment:

• Use local client store to get certs or parents transaction digests.

[huitseeker]

This is a superb example of the flow for relying on another authority to backfill a late one. Yet it only explores its happy path (none of the authorities are byzantine and the source authority has all we need).

So here's the ask: should it be part of the client's public API ? Should it be an integration test? example?
Should it rather be package-private, and then reused for the client to build more complex logic? (which seems to be where #252 is going?)

[huitseeker]

Should we open an issue for that?

[gdanezis]

Ok we can just kill the broken test.

[huitseeker]

Do we need to require every parent certificate from the authority, or could it be that we're just missing one or two? Could we find out by inspecting FastPayError::MissingEarlierConfirmations?

[gdanezis]

Right now, sadly, the authority does not provide us in one shot with all missing objects. Probably we should re-factor to do that (the information is there and already read from the DB).

[gdanezis]

@huitseeker & @sblackshear Thanks for the early feedback -- I am still working on the todos here, but very useful to get these comments in early.

[huitseeker]

Main comment: let's have a place to embed witnesses of byzantine behavior in the byzantine authority error.

[huitseeker]

I strongly believe this should embed the error and the data assets that triggered raising this.
The reason why is because those assets should end up in arbitration, something I can tokio-tracing around, but only if I have the assets in question.

In concrete terms, that means this enum variant should include ByzantineAuthoritySuspicion { authority: AuthorityName, error: dyn Error + Send + Sync }`.

If that's impossible (e.g. because we require a flurry of traits on FastPayError), we should at least embed the error.display() string that triggered this.

[gdanezis]

Yeah, I like this idea. We should think how to do this, since the nature of the data may be different for different forms of evidence.

[huitseeker]

#[derive(Debug, thiserror::Error)]
pub enum FastPayError {
    ....
    #[error("A terrible error showed byzantine behavior {0}")]
    ByzantineError(#[from] EmbeddedError),
}

impl From<FooError> for FastPayError {
    fn from(error: FooError) -> Self {
        Self::ByzantineError(EmbeddedError(error.into()))
    }
}

impl From<BarError> for FastPayError {
    fn from(error: BarError) -> Self {
        Self::ByzantineError(EmbeddedError(error.into()))
    }
}

...

/// An error that occured when checking for correct behavior
#[derive(Debug, thiserror::Error)]
#[error(transparent)]
pub struct EmbeddedError(
    Box<dyn std::error::Error + Send + Sync>,
);

This should cover it for a start, then there's Hash + Eq + Serialize + Deserialize

[gdanezis]

That is nice! Lets open an issue to do this, and try to fix the critical correctness bugs in the client beforehand.

[huitseeker]

Tip top! I've opened #301

[oxade]

@gdanezis thanks for this and the extensive comments on the functions!