A draft API for validation of replicated shares #936
Conversation
This is a simple stream adapter that performs inline validation of anything that is replicated. It will error at the end of the stream if it finds a mismatch. (Future work might include periodic checkpoints.) The reason I'm putting this up is that it is a great example of how our borrowed contexts are a giant pain in the posterior. I'm starting to think that we need to think about moving to something with Arc so we can avoid this lifetime business. It's a real drag.
| ))); | ||
| let left_peer = ctx.role().peer(Direction::Left); | ||
| let right_peer = ctx.role().peer(Direction::Left); | ||
| let ctx_ref = &ctx; |
There was a problem hiding this comment.
do you want to borrow here or it is acceptable to clone the context and move it inside the box?
If we want to borrow from C, we would need to create a self-referential struct, so maybe we need an inner struct that is pinned and holds f that borrows from ctx and ctx
however, I was able to compile it just by let ctx = ctx.clone() but that's probably not what you want here
There was a problem hiding this comment.
Yeah, I managed to get this to compile with a similar trick. Though the tests are now failing because the TestWorld doesn't have a 'static lifetime. I put the 'static condition on to avoid problems creating the future, ...
|
|
||
| impl<C: Context + 'static> ReplicatedValidatorFinalization<C> { | ||
| fn new(active: ReplicatedValidatorActive<C>) -> Self { | ||
| let ReplicatedValidatorActive { |
There was a problem hiding this comment.
it may be nicer from API's perspective to let ReplicatedValidatorActive to turn itself into a pair of hashes
There was a problem hiding this comment.
As this is just internal, I didn't do that. Though I did implement From for HashValue, which made this function a little less ugly.
| } | ||
| } | ||
|
|
||
| fn update<S, V>(&mut self, s: &S) |
There was a problem hiding this comment.
For debugging, it might be useful if the update API takes both a Step and data, and creates a trace of the validator inputs. Then we can diagnose where things went wrong if there is a mismatch. (Obviously, we would want a flag so we only pay the cost of the detailed tracing when needed.)
There was a problem hiding this comment.
The context carries a step, so we could smuggle that in somewhere.
I'm more concerned that I've been unable to instantiate this code :)
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #936 +/- ##
==========================================
+ Coverage 89.42% 89.49% +0.06%
==========================================
Files 181 182 +1
Lines 24387 24578 +191
==========================================
+ Hits 21808 21995 +187
- Misses 2579 2583 +4 ☔ View full report in Codecov by Sentry. |
| } | ||
| } | ||
|
|
||
| impl Message for HashValue {} |
There was a problem hiding this comment.
I have also defined a hash struct that implements message in my draft. Should we move it to a more general crate since we seem to need it in several places?
| ctx.send_channel(left_peer) | ||
| .send(RecordId::FIRST, left_hash.clone()), | ||
| ctx.send_channel(right_peer) | ||
| .send(RecordId::FIRST, right_hash.clone()), |
There was a problem hiding this comment.
I don't think we need to send it to both, left and right. It seems sufficient to me if each party verifies one hash. It doesn't add significant costs if a party checks both hashes, however it also doesn't seem to add anything either.
| } | ||
| } | ||
|
|
||
| /// A `ReplicatedValidator` takes a stream of replicated shares of anything |
There was a problem hiding this comment.
does a stream ensure that the state of Sha2 is updated in the same order? A different order will result in a different hash.
This is a simple stream adapter that performs inline validation of anything that is replicated. It will error at the end of the stream if it finds a mismatch. (Future work might include periodic checkpoints.)
The reason I'm putting this up is that it is a great example of how our borrowed contexts are a giant pain in the posterior. I'm starting to think that we need to think about moving to something with Arc so we can avoid this lifetime business. It's a real drag.