Laura/fetch certificates for stream follower #1121
Conversation
sui_core/src/safe_client.rs
Outdated
| match batch_info_response_item.clone() { | ||
| BatchInfoResponseItem(UpdateItem::Batch(signed_batch)) => { | ||
| // check signature of batch | ||
| let result = signed_batch.signature.check(&signed_batch.signature, signed_batch.authority); |
There was a problem hiding this comment.
Lets add a bunch of todos here for all the other checks:
- The signature should be valid over the set of transactions that the batch contains.
- And also over the hash of the previous batch.
- And the sequence numbers of trasnactions enclosed need to be correct and the size matching?
There was a problem hiding this comment.
I was going to do these now but these now but realized that this may need more discussion. I've added in todos here.
sui_core/src/safe_client.rs
Outdated
| BatchInfoResponseItem(UpdateItem::Transaction((_seq, digest))) => { | ||
| // make transaction info request which checks transaction certificate | ||
| let transaction_info_request = TransactionInfoRequest{ transaction_digest: digest }; | ||
| let transaction_info_response = self.handle_transaction_info_request(transaction_info_request).await; |
There was a problem hiding this comment.
This actually does not pipeline the requests, so it will be slow. We should fix the underlying implementation (best, todo) or make a pipelined variant for this use case.
There was a problem hiding this comment.
This is the code around the question I asked earlier slack. Is this where we would leverage a local store and only request ones that are not already there?
There was a problem hiding this comment.
Made an issue for this, and will work on this shortly #1127
There was a problem hiding this comment.
Right now, lets assume you need to download them and finalise this PR, we can later specify a flag on whether the requester wants to request them by default or not. Or you can implement the flag now i you prefer. Up to you.
sui_core/src/safe_client.rs
Outdated
| } | ||
| } | ||
| Err(_) => { | ||
| continue; |
There was a problem hiding this comment.
Are we sure we want to continue on err? If so lets comment why?
There was a problem hiding this comment.
The handle_batch_streaming on the AuthorityClient keeps processing until there are up to 10 errors found. This is because if we return on the first error, we might forgo processing of valid items later in the stream. But we also need to balance with byzantine tolerance, so we still limit the number of errors before we stop. Do you think we would do better to return on the first error?
There was a problem hiding this comment.
we might forgo processing of valid items later in the stream
If we have an error are we guaranteed that subsequent items in the stream make sense? Ie is there a condition under which the authority side of the streaming will send an error, and then continue streaming items?
There was a problem hiding this comment.
One message may have been corrupted in the encoding / decoding process? I imagine all sorts of unpredicted one-off things will eventually happen.
sui_types/src/crypto.rs
Outdated
| @@ -301,6 +301,8 @@ impl Signature { | |||
|
|
|||
| pub struct AuthoritySignature(pub dalek::Signature); | |||
|
|
|||
| impl BcsSignable for AuthoritySignature {} | |||
There was a problem hiding this comment.
The check function called on the signed_batch requires that the value implements BcsSignable.
There was a problem hiding this comment.
Hm, but are we not calling .check() on the batch, rather than on the signature?
lanvidr
force-pushed
the
laura/fetch_certificates_for_stream_follower
branch
3 times, most recently
from
4e02fa3 to
f34df16
Compare
lanvidr
force-pushed
the
laura/fetch_certificates_for_stream_follower
branch
from
f34df16 to
27e5f3e
Compare
Handles the 2nd & 3rd bullet points of #729