feat: Change proof verification and proposal creation#1641
feat: Change proof verification and proposal creation#1641bernard-avalabs wants to merge 56 commits intomainfrom
Conversation
ffi/src/proofs/change.rs
Outdated
There was a problem hiding this comment.
I'm not crazy about the readability and invariants here. Can you create a table of of what the following states mean and which ones are valid and invalid?
| verification | proposal_state | explanation |
|---|---|---|
| None | None | They get created in this state? |
| None | Immutable | Invalid? |
| None | Committed | Invalid? |
| Some(VerificationContext) | None | Verified but not proposed? |
| Some(VerificationContext) | Immutable | Proposed but not committed? |
| Some(VerificationContext) | Committed | Proposed and committed? |
Some of these states are invalid (maybe?) so we either must (a) structure it so they cannot be represented, or (b) add lots of state checks and assertions to prevent bad state.
(a) is definitely more rusty, here's one way to do that (assuming I'm right above, but I could easily be wrong):
pub struct ChangeProofContext<'db, State> {
state: State<'db>,
proof: FrozenChangeProof,
}
struct InitialState<'_>;
struct VerifiedState<'_>(VerificationContext));
struct ProposedState<'db>(crate::ProposalHandle<'db>);
struct CommittedState<'db>(HashKey);
I also don't really like "initialState" in this design either. I think I'd prefer to only have these contain verified proofs. Wrapping it in a ChangeProofContext when the verification is incomplete seems incorrect. I think this leaves you with:
- a
TryIntoimplementation fromFrozenChangeProoftoChangeProofContext<'db, VerifiedState> - a
propose()method onChangeProofContext<'db, VerifiedState> - a
commit()method onChangeProofContext<'db, ProposedState>
If you find you really still need 'verify_and_propose' then it should go through both states. This prevents anyone from being able to write code that creates a proposal from an unverified CP.
I'm also a bit worried about the db lifetime and what happens if you make some of these calls with a different db (propose on db1, commit on db2). This could be solved by keeping a reference to the db inside the context and not specifying it after it's created.
There was a problem hiding this comment.
I've changed the interface based on your feedback. One difference is that, instead of following the typestate pattern with different states for ChangeProofContext, I've created separate structs for each context: ChangeProofContext, VerifiedProofContext, ProposedProofContext, etc. I think this makes it slightly more readable given the matching ChangeProofResult, VerifiedProofResult, and ProposedProofResult, and more cleanly distinguishes the different states in Golang.
I've also kept ChangeProofContext to represent an unverified change proof. This allows (without more interface changes) a change proof to be created without calling verify on it. It shouldn't be possible to use an unverified change proof directly as it must be verified first before a proposal can be created from it.
Used a single enum to track the state of a change proof context. Also compare the database handle with the saved version to ensure a previously created change proof proposal is being used on the same database.
…proof-api-v3-part3
…bs/firewood into bernard/change-proof-api-v3-part3
…proof-api-v3-part3
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.gothat 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.