Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update proof verification #726

Merged
merged 8 commits into from
Oct 6, 2020
Merged

Update proof verification #726

merged 8 commits into from
Oct 6, 2020

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Oct 2, 2020

Summary of changes
Changes introduced in this pull request:

  • Fixes winning post proof check (and sub functions)
    • Bumps proofs version
  • Implements new ProofVerifier trait to allow for verification to be swapped out for a mock verifier for testing
    • There was also a bunch of duplicated and incorrect logic, so consolidating was necessary
    • This trait just has associated functions, and is just used with PhantomData so it has no runtime cost

I'll get ahead of a few questions since I assume it might not be intuitive from scanning changes:

Why not have the runtime Syscalls trait a supertrait (yes that's the name) of ProofVerifier?

While there is overlap, syscalls does not need the functionality of verifying winning post proofs or generating sector challenge ranges, so that is not a direct subset and would require implementing extra logic for other runtimes/VM.

Why not have StateManager own the ProofVerifier generic?

This is a bit more opinionated, but it made more sense to me to only require the generic type when calling a function that uses the VM. Negative to this is that it opens up a possibility of calling a function with a full and mock verifier, and they could theoretically disagree on state because the full verifier can fail where the mock verifier would succeed. This can always change when #713 is implemented (and our strategy of swapping this out is clear)

Why was the default proof verification removed from Syscalls trait?

Didn't want to add the generic bound on the trait, as only the DefaultSyscalls type has the option of using the generic. Also because the logic was moved into the ProofVerifier trait.

Why were the state_manager::utils functions changed to implementation of StateManager?

Idk, why was it setup the way it was in the first place? Got very verbose quick with introducing the extra generic.

Why do the RPC functions that use the VM default to FullVerifier implementation?

The mechanism for swapping verifier implementation at runtime isn't decided, and didn't want to add another generic bound to the RPC state when the node also defaults to FullVerifier.

Reference issue to close (if applicable)

Closes #712

Other information and links

@ec2
Copy link
Member

ec2 commented Oct 6, 2020

Why were the state_manager::utils functions changed to implementation of StateManager?
Probably because they are really DB wrappers was the original reason they weren't in the SM impl.

Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Although blockchain/state_manager/src/utils.rs could benefit from some documentation on the public fns

@austinabell
Copy link
Contributor Author

Why were the state_manager::utils functions changed to implementation of StateManager?
Probably because they are really DB wrappers was the original reason they weren't in the SM impl.

I don't know what you mean, the Lotus implementation separates into own functions not associated, which I assume is the reason, but it makes no sense to do in Rust and adds a lot of clunkyness with generics. (https://github.com/filecoin-project/lotus/blob/bc495a5c8cdf3934b4f559f735141bdeaf06c64d/chain/stmgr/utils.go)

@austinabell austinabell merged commit 3ec2337 into main Oct 6, 2020
@austinabell austinabell deleted the austin/proofsupdate branch October 6, 2020 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update winning post proof validation
3 participants