Enable collection of secp256k1_verify and secp256r1_verify operators #501

Rigidity · 2024-11-27T00:58:37Z

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:

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).
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.

coveralls-official · 2024-11-27T01:08:24Z

Pull Request Test Coverage Report for Build 12057164038

Details

64 of 74 (86.49%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.02%) to 93.324%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/collect_dialect.rs	64	74	86.49%

Totals
Change from base Build 11855487378:	-0.02%
Covered Lines:	5620
Relevant Lines:	6022

💛 - Coveralls

richardkiss · 2024-11-27T02:49:44Z

src/run_program.rs

    #[cfg(feature = "pre-eval")]
    pre_eval: Option<PreEval>,
    #[cfg(feature = "pre-eval")]
    posteval_stack: Vec<Box<PostEval>>,
+    collected_ops: Option<Vec<CollectedOp>>,


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.

Good point, a new dialect might be cleaner

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

I've done a similar thing with the python api without mutable dialects here, but I guess pyo3 is essentially RefCell-like objects everywhere. Might be worth trying it both ways: mutable dialects and with a RefCell to see what is easiest. RefCell is usually kind of a bad smell because if you make a mistake, you get a runtime error instead of a compile error. But I think in this case, it should be pretty easy to avoid a runtime error so RefCell might be the least intrusive without being too gross.

richardkiss · 2024-11-27T02:49:58Z

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

arvidn

I would prefer a solution (which I believe Richard suggested) where you make a new Dialect class that collects all the public keys and messages, and forwards everything to the default ChiaDialect.

I think that would make this feature the most contained.

Rigidity · 2024-11-27T14:21:17Z

I would prefer a solution (which I believe Richard suggested) where you make a new Dialect class that collects all the public keys and messages, and forwards everything to the default ChiaDialect.

I think that would make this feature the most contained.

Could even make it wrap a generic inner dialect. I'll give this a shot

Rigidity · 2024-11-27T14:59:29Z

This new implementation uses a custom Dialect and RefCell, since it requires no changes to the existing code. This raises the question of whether this should be part of the core CLVM crate or if it should just be part of the wallet code (or chia-wallet-sdk)? As is, it can be done already externally without any changes.

If we make Dialect::op take self mutably, it would probably be slightly more efficient/safe with the downside of requiring a breaking change to the API.

richardkiss · 2024-11-27T18:29:44Z

src/run_program.rs

@@ -1580,4 +1581,35 @@ mod tests {

        assert_eq!(result.unwrap().0, cost);
    }
+
+    #[test]


Is it possible to move this to collect_dialect.rs? It'd be nice to have no changes to run_program.rs

Yeah I'll move it

richardkiss · 2024-11-27T18:30:20Z

src/secp_ops.rs

@@ -7,8 +7,8 @@ use k256::ecdsa::{Signature as K1Signature, VerifyingKey as K1VerifyingKey};
 use p256::ecdsa::signature::hazmat::PrehashVerifier;
 use p256::ecdsa::{Signature as P1Signature, VerifyingKey as P1VerifyingKey};

-const SECP256R1_VERIFY_COST: Cost = 1850000;
-const SECP256K1_VERIFY_COST: Cost = 1300000;
+pub const SECP256R1_VERIFY_COST: Cost = 1850000;


Prob don't need this change anymore.

Actually, I guess you do.

I wonder if all of the costs should be defined in one place, like src/costs.rs and exported publicly, for consistency?

To avoid changing the public API on accident, I changed it to pub(crate) for now.

richardkiss · 2024-11-27T18:33:29Z

src/collect_dialect.rs

+        let response = self.dialect.op(allocator, op, args, max_cost, extensions);
+
+        let op_len = allocator.atom_len(op);
+        if op_len != 4 {


This is a nice trick to restrict to these new opcode, but it deserves a comment at least since it may cause surprising things to happen if we add additional length four operators in future.

Yeah and frankly I should probably capture BLS opcodes too.

Rigidity · 2024-11-28T01:35:44Z

This can be implemented externally to clvm_rs so barring a more general solution I'm going to leave this for now.

Signature operator collection

cfc1b03

Rigidity force-pushed the signature-collection branch from ba4d756 to cfc1b03 Compare November 27, 2024 00:59

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

Rigidity requested a review from arvidn November 27, 2024 01:18

richardkiss reviewed Nov 27, 2024

View reviewed changes

arvidn reviewed Nov 27, 2024

View reviewed changes

Rigidity added 3 commits November 27, 2024 09:56

CollectDialect

ddd19d9

Remove comment

47cb339

Remove newline

51d9a32

Rigidity requested review from arvidn and richardkiss November 27, 2024 14:57

richardkiss reviewed Nov 27, 2024

View reviewed changes

Rigidity added 3 commits November 27, 2024 14:31

Move test

4ab98e2

Revert newline removal

199392c

Add comment

3587056

Rigidity requested a review from richardkiss November 27, 2024 19:36

pub(crate)

ee81b38

Rigidity marked this pull request as draft November 28, 2024 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable collection of secp256k1_verify and secp256r1_verify operators #501

Enable collection of secp256k1_verify and secp256r1_verify operators #501

Rigidity commented Nov 27, 2024 •

edited

Loading

coveralls-official bot commented Nov 27, 2024 •

edited

Loading

richardkiss Nov 27, 2024

Rigidity Nov 27, 2024

Rigidity Nov 27, 2024

richardkiss Nov 27, 2024

richardkiss commented Nov 27, 2024

arvidn left a comment

Rigidity commented Nov 27, 2024 •

edited

Loading

Rigidity commented Nov 27, 2024 •

edited

Loading

richardkiss Nov 27, 2024 •

edited

Loading

Rigidity Nov 27, 2024

richardkiss Nov 27, 2024

richardkiss Nov 27, 2024

Rigidity Nov 27, 2024

Rigidity Nov 27, 2024

richardkiss Nov 27, 2024

Rigidity Nov 27, 2024

Rigidity commented Nov 28, 2024

Enable collection of secp256k1_verify and secp256r1_verify operators #501

Are you sure you want to change the base?

Enable collection of secp256k1_verify and secp256r1_verify operators #501

Conversation

Rigidity commented Nov 27, 2024 • edited Loading

coveralls-official bot commented Nov 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12057164038

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkiss commented Nov 27, 2024

arvidn left a comment

Choose a reason for hiding this comment

Rigidity commented Nov 27, 2024 • edited Loading

Rigidity commented Nov 27, 2024 • edited Loading

richardkiss Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rigidity commented Nov 28, 2024

Rigidity commented Nov 27, 2024 •

edited

Loading

coveralls-official bot commented Nov 27, 2024 •

edited

Loading

Rigidity commented Nov 27, 2024 •

edited

Loading

Rigidity commented Nov 27, 2024 •

edited

Loading

richardkiss Nov 27, 2024 •

edited

Loading