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

Verifier plugin interface #1324

Merged
merged 14 commits into from
Jan 30, 2024
Merged

Verifier plugin interface #1324

merged 14 commits into from
Jan 30, 2024

Conversation

edmundnoble
Copy link
Contributor

@edmundnoble edmundnoble commented Dec 5, 2023

This PR provides a prototype of an interface designed to provide generalized verification of Pact transactions using proofs included in the transaction. A KIP will be forthcoming with more details and a higher level view.

The idea is to make it easy to integrate new types of proofs of a transaction's validity, whether they're some new type of signatures, ZK proofs, SPV proofs, or anything else similar. Pact can effectively treat them all the same, as an opaque piece of data and a set of capabilities that it purports to provide. The Pact container layer, usually chainweb, is responsible for actually verifying that the contents of these proofs provide the capabilities they purport to; Pact code only knows that they are scoped to the capabilities they provide.

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output
  • Documentation has been updated if new natives or FV properties have been added. To generate new documentation, issue cabal run tests. If they pass locally, docs are generated.
  • Any changes that could be relevant to users have been recorded in the changelog
  • In case of changes to the Pact trace output (pact -t), make sure pact-lsp is in sync.
    • Not sure if this is necessary.

Additionally, please justify why you should or should not do the following:

  • Confirm replay/back compat
    • The enforce-verifier native is guarded behind a flag. It's up to the container layer to ensure that verifiers are not accepted before a fork point, and to set the flag.
  • Benchmark regressions
    • There are no changes to code that doesn't involve verifiers.
  • (For Kadena engineers) Run integration-tests against a Chainweb built with this version of Pact

where
enforceVerifier :: RNativeFun e
enforceVerifier i as = case as of
[TLitString verifierName, TList {_tList = (traverse (preview _TLitString) -> Just args) }] -> do
Copy link
Member

Choose a reason for hiding this comment

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

spurious do (but personally i'd not cram everything into views and >>= and just add some let bindings)

Copy link
Contributor

@sirlensalot sirlensalot left a comment

Choose a reason for hiding this comment

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

Please also update description/address PR boilerplate.

Incomplete review, need to discuss some things

Copy link
Contributor

@sirlensalot sirlensalot left a comment

Choose a reason for hiding this comment

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

EDIT I think I understand the change better, so my main concern is not with how this is implemented but future semantic consistency with keysets, where we can't put the cat back in the bag -- keyset enforces will be allowed outside of defcaps, so should we be similarly lenient for this enforce?

Main comment is, please consider not adding the boolean csEvaluating and just doing a callstack check to restrict native application.

Copy link
Contributor

@sirlensalot sirlensalot left a comment

Choose a reason for hiding this comment

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

Minor nit

@edmundnoble
Copy link
Contributor Author

I think I understand the change better, so my main concern is not with how this is implemented but future semantic consistency with keysets, where we can't put the cat back in the bag -- keyset enforces will be allowed outside of defcaps, so should we be similarly lenient for this enforce?

It's arguably worse to allow verifier enforces outside of defcaps because while a signer has presumably at least seen the code of the transaction, a verifier has no way to do so. Regardless, to me, a keyset enforce outside of a defcap is a definite, unambiguous bug. I would support looking at a possible migration in the future so that newly deployed modules make it illegal. There exist historical examples and I think that's because capabilities are newer than keysets and guards, but that's not something any new code should do.

@edmundnoble edmundnoble changed the title SPI interface prototype Verifier plugin interface Dec 22, 2023
Old code didn't properly detect when a composed capability was managed.

Co-authored-by: Jose Cardona <jose@kadena.io>
@edmundnoble
Copy link
Contributor Author

I'm re-requesting review from @sirlensalot specifically for the last two commits in here; @jmcardon spotted an issue where managed caps that are being composed don't actually show up as CapComposed on the cap stack, so we really do need a separate stack of caps that are currently being evaluated in EvalState.

Specifically I chose to push and pop from this stack in the with-capability and compose-capability native functions and not in the cap runtime (specifically evalCap) because evalCap is sort of recursive and it just confused me, whereas I think I know from the natives' definitions exactly where the cap is being evaluated and where it's done evaluating. If there was a better place to put this, especially close to evalCap, I want to know.

@edmundnoble edmundnoble merged commit d37f549 into master Jan 30, 2024
8 checks passed
@emilypi emilypi deleted the edmund/spi branch January 30, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants