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

Enable collection of secp256k1_verify and secp256r1_verify operators #501

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rigidity
Copy link
Contributor

@Rigidity Rigidity commented Nov 27, 2024

With BLS signatures, you can simply run a puzzle with its solution to figure out all of the public key and message pairs that need to be signed, since they're included in the list of output conditions.

However, the secp256k1 and secp256r1 opcodes will fail at runtime if the signature passed into the solution doesn't match, instead of returning the expected values in conditions.

This PR enables a similar workflow by adding a new run_program_with_signatures function that skips and collects these operators and returns them in a list. Instead of failing the program, they will be treated as if they were successful in validating the signature, to allow the rest of the program to proceed. You can iterate the list of collected ops to find the public keys, messages, and signatures needed for signing.

The only problem with this is you need a way to update the original solution afterward to use the new signatures (and then you could run it for real to see if it actually worked). This is the responsibility of the code consuming this API, and can potentially be done in two ways:

  1. Walk the solution as a tree of NodePtrs and compare the NodePtr directly via a HashMap. This assumes nothing has been done to change the NodePtr itself (quoting does not, for example).
  2. Instead of nil and relying on the NodePtr matching, you could use a completely random fake signature and check for equality of the signature value in order to replace it with the real signature later.

I believe this PR will enable considerable simplification of wallet code, if my assumptions are correct.

@Rigidity Rigidity marked this pull request as ready for review November 27, 2024 01:06
Copy link

Pull Request Test Coverage Report for Build 12041720866

Details

  • 64 of 66 (96.97%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 93.443%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/run_program.rs 46 47 97.87%
src/runtime_dialect.rs 0 1 0.0%
Totals Coverage Status
Change from base Build 11855487378: 0.1%
Covered Lines: 5615
Relevant Lines: 6009

💛 - Coveralls

@Rigidity Rigidity requested a review from arvidn November 27, 2024 01:18
#[cfg(feature = "pre-eval")]
pre_eval: Option<PreEval>,
#[cfg(feature = "pre-eval")]
posteval_stack: Vec<Box<PostEval>>,
collected_ops: Option<Vec<CollectedOp>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I do have reservations about making more complex this code which is already a bit too complex and nasty. You might be able to implement this functionality with pre_eval, and you can surely do it by creating a new separate "wallet/debugging" dialect where you can override the opcodes you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, a new dialect might be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue is the apply op function takes an immutable reference. Maybe we should maybe dialects mutable? Alternatively it could use a RefCell or Mutex I suppose, but that seems unideal

@richardkiss
Copy link
Contributor

This is a great observation and there are some great ideas here.

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.

2 participants