feat: Change proof serialization/deserialization#1638
Conversation
bernard-avalabs
requested review from
RodrigoVillar,
alarso16,
demosdemon and
rkuris
as code owners
| r.NoError(err) | ||
| } | ||
|
|
||
| func TestChangeProofDiffersAfterUpdate(t *testing.T) { |
There was a problem hiding this comment.
It's not totally clear what this is testing. Wouldn't being able to commit a change proof and resulting in the correct root mean that they contain the correct data? It seems that this might just be an intermediate test until that functionality is present, but if that's the case, will you add a //TODO: delete this once verification and commitment are implemented. If you think this test will continue to be necessary after that, can you argue why?
There was a problem hiding this comment.
This test is based on TestRangeProofDiffersAfterUpdate. Similar to TestChangeProofEmptyDB, its mainly to check basic functionality, namely the ability to create a serialized change proof from two non-empty revisions without error, and to verify that serialized versions of distinct change proofs are different.
It can catch serialization issues that simply checking the root hash will miss. However, TestRoundTripChangeProofSerialization is a more comprehensive test for serialization/deserialization.
It don't think it hurts to keep it since it is a short test. But I can remove it if you don't think it is necessary.
firewood/src/proofs/de.rs
Outdated
| found => Err(ReadError::InvalidHeader( | ||
| InvalidHeader::UnsupportedVersion { found }, | ||
| )), |
There was a problem hiding this comment.
nit: would have preferred this error is handled before the main code instead of indenting. Also a shame bool::ok_or is not stable yet. Maybe:
if header.version != 0 {
return Err(ReadError::InvalidHeader(
InvalidHeader::UnsupportedVersion { found },
))
}
let mut reader = ...
There was a problem hiding this comment.
I've made this change.
…bs/firewood into bernard/change-proof-api-v3-part2
firewood/src/proofs/de.rs
Outdated
| .read_item::<u8>() | ||
| .map_err(|err| err.set_item("option discriminant"))? | ||
| { | ||
| 0 => Ok(BatchOp::Put { |
There was a problem hiding this comment.
nit: you can use the constants here in the match pattern as well.
## Why this should be merged Builds on #1638. Verifies a change proof and creates a proposal on the destination trie. ## How this works Mostly follows the implementation from range proofs with some additional checks that are specific to change proofs. The interface for range proofs has a separate function for verify (`fwd_range_proof_verify`) and verify and propose (`fwd_db_verify_range_proof`). The change proof interface only has verify, but the comments explains that it also prepares a proposal. I have follow the comment by implementing proof verification and proposal creation in the verify function. ## How this was tested A basic test was added in `proofs_test.go` that creates a change proof and verifies it and check that it does not return an error. More end-to-end tests will be added in a later PR.
4 participants
Why this should be merged
Second PR for change proof FFI (builds on #1637). This PR includes
fwd_change_proof_to_bytesandfwd_change_proof_from_bytesfor serialization/deserization of a change proof.How this works
Mostly follows the serialization/deserialization implementation in range proof. Main change is to support serializing/deserializing
BatchOps which are used for storing the difference between two revisions.How this was tested
Basic serialization and deserialization tests in
proofs_test.go, including a round trip test where a change proof is serialized, deserialized, and serialized again, and verifying the two serialized proofs match.