Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #808 +/- ##
==========================================
+ Coverage 91.10% 91.16% +0.06%
==========================================
Files 110 110
Lines 20913 21206 +293
Branches 20913 21206 +293
==========================================
+ Hits 19052 19333 +281
- Misses 1488 1498 +10
- Partials 373 375 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
76fdfcc to
0886566
Compare
|
Benchmark results for revision c3a446c:
Full results
Compare the results above with those for the default branch. |
There seems to be a performance drop, perhaps? |
0886566 to
ca6fd05
Compare
ec9b624 to
fd14151
Compare
592faa4 to
01886c1
Compare
thomasathorne
left a comment
There was a problem hiding this comment.
Generally looks great!
Have a few questions about the different error cases and how they are currently tested (or not tested). I may have misunderstood something as some of the errors output in the tests aren't what I expected! 😅 At any rate, the different cases could be clarified a bit.
Co-authored-by: Emma Turner <emma.turner@trili.tech>
fec1f02 to
ec13689
Compare
This PR now also contains a small PVM state restructuring which @emturner discovered offsets the slight dip in performance. |
ec13689 to
8a3a56e
Compare
| } | ||
| } | ||
|
|
||
| /// Write `message` to the outbox at the current level. Returns `Err(OutboxFull)` |
There was a problem hiding this comment.
| /// Write `message` to the outbox at the current level. Returns `Err(FullOutbox)` |
8a3a56e to
301d866
Compare
thomasathorne
left a comment
There was a problem hiding this comment.
Ok, I've understood the different error cases now---sorry for my confusion earlier.
I still think if you want to make it really clear what is being tested in each case, adding a few more cases to the OutboxProofError enum would help. Unless I am missing something I think there are still two cases that aren't tested: the case where the outbox is uninitialised and the case where we try to read from a level that doesn't exist yet.
Splitting LevelNotFound into three cases (OutboxUninitialised, LevelTooOld and LevelNotReachedYet) would clarify this.
But this is more a stylistic choice than anything else, happy for you to resolve this comment if you want.
| return Err(SbiError::OutputTooLarge); | ||
| } | ||
| let boxed_slice = vec![0u8; size].into_boxed_slice(); | ||
| Ok(OutboxMessage(boxed_slice)) | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<Box<[u8]>> for OutboxMessage { | ||
| type Error = OutboxProofError; |
There was a problem hiding this comment.
On line 483, the error type is SbiError where as on line 491 the error type is OutboxProofError. try_from Error type should really be generic across contexts.
Does it make sense to use an OutboxError type then map those to Proof or SBI errors depending on the context? The other thing that we could do is to have try_from return the SbiError then map it to a Proof error as you need it.
Closes RV-872. Closes RV-877.
What
The outbox can now produce a proof for an outbox message. A proof contains the message metadata and a Merkle proof which ties the inclusion of the given message in the outbox with a particular state hash.
Why
The outbox needs to support inclusion proofs for messages.
How
The
OutboxProoftype is introduced, along withOutputandOutputInfo, which model Tezos protocol types.Outbox proofs employ the same proof production mechanism used for fraud proofs. The difference is that the proven state transition is not one step of the PVM but a successful read of the given message from the outbox. The outbox is thus also expanded with the ability to read a single message.
Manually Testing
In particular, the
test_outbox_proofs_dummy_kerneltest.Regressions
A regression for an outbox proof for the first message of the last level of the dummy kernel is checked in.
Tasks for the Author