[follower] Remove transaction from batch, and verify the digests received. #1431
Conversation
gdanezis
changed the title
sui_core/src/safe_client.rs
Outdated
|
|
||
| let x = match &batch_info_item { | ||
| Ok(BatchInfoResponseItem(UpdateItem::Batch(signed_batch))) => { | ||
| println!("batch {:?}", signed_batch.batch); |
There was a problem hiding this comment.
should this be a log debug line?
There was a problem hiding this comment.
Oh no, forgot my debug prints :)
| if let Err(err) = client.check_update_item_batch_response( | ||
| req_clone, | ||
| &signed_batch, | ||
| &txs_and_last_batch, |
There was a problem hiding this comment.
Does this need to be populated with the transactions from the batch first? I think this is what confused me before, that we don't store the digests of each transaction in the batch. If we use all the digests to create the batch, but then we don't pass in all the digests to reconstruct the batch, is it still possible we get the same batch after reconstruction?
There was a problem hiding this comment.
As you can see for the first batch, I just check the signature, and only check the second batch for its actual strucure. Maybe there are some gaps, but indeed without the transactions before it makes no sense to check very much.
| ); | ||
| Ok(BatchInfoResponseItem(UpdateItem::Transaction((seq, digest)))) => { | ||
| // A stream always starts with a batch, so the previous should have initialized it. | ||
| // And here we insert the tuple into the batch. |
There was a problem hiding this comment.
In one of the batch tests, I saw that there were a few individual transactions sent before a batch was sent, also is it not ever expected that we would have individual transactions sent only like how we see in the benchmark with the default parameters set?
There was a problem hiding this comment.
Hm, right now we indeed have a gap in testing. We test only against a mock server. It would be nice to test against a real authority to make sure we catch something like this.
There was a problem hiding this comment.
As I remembered the follower server side should be sending batch --> (trasnactions+ -> batch) + , so all transactions are fully enclosed by batches. We should test to see if there is any issue with this?
…ived. (#1431) * Remove trasnaction from batch, and verify the digests received. Co-authored-by: George Danezis <george@danez.is>
This is a patch to
refactor_stream_clientthat removes the list of transactions from the batch, and uses the transactions received to verify the batches and ensure they are signed.