Redesign clvm-traits to be more generic #314

Rigidity · 2023-11-12T17:58:56Z

This PR overhauls the clvm-traits and clvm-derive crates, making them more generic.

This is to simplify implementing the following functionality when needed in the future:

Ability to serialize and deserialize CLVM values directly to bytes.
Ability to calculate the tree hash of a value without going through the CLVM Allocator.
Walk trees produced by CLVM values for any other reason, such as debugging.

Task list:

Rewrite ToClvm and FromClvm traits to be more generic, and update clvm-derive accordingly.
Simplify the process of implementing the traits with macros, to get around lack of support for impl Trait in type aliases.
Improve the code quality of clvm-derive, and rename curried_args and proper_list to curry and list, respectively.
Add support for deriving enums with the enum variant encoded as the first argument in a tuple or list.
Add support for deriving enums with no discriminant (a simple or operation).
Settle on final naming for a.value_to_ptr and a.value_from_ptr from AllocatorExt. Perhaps this should be a top-level function.
Update documentation, and clean up doc tests.
Thoroughly review every change and make sure everything is tested. It's critical that no major bugs surface in production wallet code.

Out of scope for this PR:

Implement the use of the derive macros for various existing structs and enums.
Add support for currying and uncurrying known quantities of arguments without allocating on the heap.
Get rid of the various redundant implementations of SingletonArgs and SingletonSolution split amongst the code base.

Rigidity · 2023-11-13T23:35:03Z

@arvidn this should be good to review, tests are passing on my end and I finished the last few things.

MatchByte struct.

tree hashes of values.

arvidn

I think your first approach (where Allocator was a template and could be swapped out) was simpler than this approach. As I mention in the comments, I'm not excited about the basic structure of this abstraction being so complex you need a macro to implement the trait.

I'm thinking that maybe it would even make sense to not reuse the ToClvm and FromClvm traits. What would you think of, instead add a new trait, something like ClvmSerialize and ClvmDeserialize which converts to- and from serialized binary representation. There will be a little bit of overlap with ToClvm and FromClvm, but not very much, and I think the resulting code will be a lot more legible.

I'm not convinced that it's important to be able to compute the tree hash directly via this trait. The most efficient tree hash implementations are not recursive anyway, and it seems risky to introduce a sub-optimal one. Perhaps efficient tree-hash of rust types should even be yet another trait. I think that would keep things really simple too

arvidn · 2023-11-16T12:42:03Z

chia-protocol/fuzz/fuzz_targets/parse-program.rs

-
-fuzz_target!(|data: &[u8]| {
-    let _ = Program::from_bytes(data);
-});


did you mean to remove this?

arvidn · 2023-11-16T12:46:49Z

clvm-traits/src/clvm_value.rs

+/// This is used for building trees with the `ToClvm` and `FromClvm` traits.
+pub enum ClvmValue<'a, Node> {
+    /// An atomic value in CLVM, represented with bytes.
+    Atom(&'a [u8]),


I haven't fully wrapped my head around this patch yet, but it worries me a bit to expose an atom as a reference to a slice. I'm hoping to, in the future, make Allocator not expose the slices directly as references, but as proxy objects, to allow internal mutability of the Allocator to convert types lazily (G1, G2 elements, big integers etc.).

The problem is in cases where we need multiple references to atom buffers, and the second one will need to be converted on the fly. The first reference we've already handed out may be invalidated, so the allocator itself isn't mutable at that point.

I have reasons to believe that the execution of CLVM programs could see a material speed-up if the conversions would be lazy (and skipped entirely in many cases)

arvidn · 2023-11-16T12:49:46Z

clvm-traits/src/from_clvm.rs

-        Ok(ptr)
-    }
+#[macro_export]
+macro_rules! from_clvm {


I can see it being tempting to stick all this in a macro, it makes me a little bit nervous though, since it becomes a bit opaque at the call sites. Did you explore using type aliases to make the signature a bit shorter instead?

arvidn · 2023-11-16T12:58:35Z

clvm-traits/src/from_clvm.rs

+    Node: Clone,
+{
+    fn from_clvm<'a>(
+        f: &mut impl FnMut(&Node) -> ClvmValue<'a, Node>,


I think f could use a better name, or at least a comment explaining what it's supposed to do and be used for

arvidn · 2023-11-16T13:52:27Z

clvm-traits/src/conversions.rs

+                let mut sha256 = Sha256::new();
+                sha256.update([2]);
+                sha256.update(first.0);
+                sha256.update(rest.0);


this doesn't look right. don't you need to recurse left and right?
I was going to point out that the fact that this trait must be recursive (afaiu) is a security risk when called on input of untrusted structure (e.g. a NodePtr), but would be fine on an object known at compile time.

Actually, given that the existing tree hash algorithms we have are not recursive, I would think those would be preferable over this one in all cases. The main benefit of this feature you're adding is that we can convert directly between a concrete rust type to serialized binary form. In this case, that we could compute the tree hash for a structure without first turning it into a CLVM tree.

I would think a better approach (rather than implementing tree_hash() here, would be to improve the existing ones to work with any type implementing ToClvm

arvidn · 2023-11-16T14:16:29Z

clvm-traits/src/conversions.rs

+        let ptr = value.tree_hash().unwrap().0;
+        assert_eq!(
+            hex::encode(ptr),
+            "7c3670f319e07cff6d433e4c22e0895f1f0a10bad5bbcd23c32e3bc5589c23cb"


it's not obvious that this hash is correct by inspecting this test. I think it would be good to test this implementation of tree_hash against the existing implementations. Maybe even in a fuzzer.

arvidn · 2023-11-16T14:21:17Z

clvm-traits/src/error.rs

+#[derive(Error, Debug, Clone, PartialEq, Eq)]
+pub enum ToClvmError {
+    #[error("limit reached")]
+    LimitReached,


it would be less generic and more concrete (I think) to call this something like out of memory. As far as I can tell, it's only used when the Allocator hits its memory limit

arvidn · 2023-11-16T14:22:48Z

clvm-traits/src/conversions.rs

+                _ => Ok(a.new_atom(bytes).or(Err(ToClvmError::LimitReached))?),
+            },
+            ClvmValue::Pair(first, rest) => {
+                Ok(a.new_pair(first, rest).or(Err(ToClvmError::LimitReached))?)


Maybe I misunderstand something, how come you don't need to call to_ptr() on first and rest? Have they already been converted to allocations in a?

arvidn · 2023-11-16T14:26:37Z

clvm-traits/src/lib.rs

@@ -90,9 +96,9 @@ mod tests {
    }

    #[test]
-    fn test_proper_list() {
+    fn test_list() {


renaming proper_list -> list and curried_args -> args are not convtroversial. I would land that change right away.

arvidn · 2023-11-16T14:36:11Z

clvm-traits/src/error.rs

-    #[error("expected cons")]
-    ExpectedCons(NodePtr),
+#[derive(Error, Debug, Clone, PartialEq, Eq)]
+pub enum FromClvmError {


it seems reasonable to split up the error codes for these

coveralls-official · 2023-11-16T15:05:57Z

Pull Request Test Coverage Report for Build 6871666220

679 of 733 (92.63%) changed or added relevant lines in 22 files are covered.
20 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.8%) to 86.588%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
chia-wallet/src/puzzles/nft.rs	0	1	0.0%
clvm-traits/src/conversions.rs	43	44	97.73%
clvm-traits/src/error.rs	1	2	50.0%
clvm-traits/src/from_clvm.rs	13	14	92.86%
clvm-traits/src/match_byte.rs	17	18	94.44%
chia-protocol/src/program.rs	0	2	0.0%
chia-wallet/src/puzzles/singleton.rs	0	3	0.0%
chia-wallet/src/proof.rs	0	4	0.0%
clvm-derive/src/helpers.rs	36	46	78.26%
clvm-derive/src/from_clvm.rs	255	269	94.8%

Files with Coverage Reduction	New Missed Lines	%
chia-protocol/src/coin.rs	1	97.78%
chia-protocol/src/message_struct.rs	1	28.57%
chia-wallet/src/proof.rs	1	0.0%
clvm-derive/src/helpers.rs	1	80.6%
clvm-utils/src/curried_program.rs	1	96.43%
wheel/src/run_program.rs	1	69.44%
wheel/src/run_generator.rs	2	83.11%
chia-protocol/src/program.rs	3	59.57%
src/fast_forward.rs	4	98.35%
wheel/src/api.rs	5	68.65%

Totals
Change from base Build 6799173174:	0.8%
Covered Lines:	10310
Relevant Lines:	11907

💛 - Coveralls

arvidn · 2023-11-16T15:55:27Z

src/fast_forward.rs

@@ -110,8 +112,8 @@ pub fn fast_forward_singleton(
        return Err(Error::PuzzleHashMismatch);
    }

-    let singleton = CurriedProgram::<SingletonArgs>::from_clvm(a, puzzle)?;
-    let mut new_solution = SingletonSolution::from_clvm(a, solution)?;
+    let singleton: CurriedProgram<NodePtr, SingletonArgs<NodePtr>> = FromPtr::from_ptr(a, puzzle)?;


why do you need the NodePtr template argument here? isn't that implied by FromPtr?

You would think. I think the Rust compiler has limitations on what it can infer

arvidn · 2023-11-16T15:56:01Z

src/fast_forward.rs

-        let puzzle = spend.puzzle_reveal.to_clvm(&mut a).expect("to_clvm");
-        let solution = spend.solution.to_clvm(&mut a).expect("to_clvm");
+        let puzzle = node_from_bytes(&mut a, spend.puzzle_reveal.as_slice()).expect("to_clvm");
+        let solution = node_from_bytes(&mut a, spend.solution.as_slice()).expect("to_clvm");


I think we should preserve the ability for Program to convert to and from NodePtr

Yeah, I agree - I'll make sure this is included

maintainability.

Completely redesign clvm-traits to be more generic

25cb5a9

Rigidity mentioned this pull request Nov 12, 2023

Clvm traits v2 #259

Closed

Rigidity changed the title ~~Completely redesign clvm-traits to be more generic~~ Redesign clvm-traits to be more generic Nov 12, 2023

Rigidity added 5 commits November 12, 2023 15:14

Rename proper_list and curried_args

63e4a9b

Implement derive for enums, add bounds

b170dfc

Add support for raw enums in clvm-derive

b9cad06

Refactor conversions into FromPtr and ToPtr

5522142

Add missing documentation

61b4a6a

Rigidity marked this pull request as ready for review November 13, 2023 23:33

Rigidity added 5 commits November 13, 2023 22:14

Add clone bound to Raw

9bcebfe

Add PartialEq, Eq, PartialOrd, Ord, Hash to

3247abb

MatchByte struct.

Add TreeHash and ToTreeHash traits for computing

fef6c97

tree hashes of values.

Use Bytes32 instead of [u8; 32]

0f70262

Fix incorrect number serialization

8690886

arvidn reviewed Nov 16, 2023

View reviewed changes

Rigidity marked this pull request as draft November 16, 2023 18:10

Refactor code for improved readability and

95073f9

maintainability.

Rigidity closed this Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign clvm-traits to be more generic #314

Redesign clvm-traits to be more generic #314

Rigidity commented Nov 12, 2023 •

edited

Loading

Rigidity commented Nov 13, 2023

arvidn left a comment

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

arvidn Nov 16, 2023

coveralls-official bot commented Nov 16, 2023

arvidn Nov 16, 2023

Rigidity Nov 16, 2023

arvidn Nov 16, 2023

Rigidity Nov 16, 2023

Redesign clvm-traits to be more generic #314

Redesign clvm-traits to be more generic #314

Conversation

Rigidity commented Nov 12, 2023 • edited Loading

Rigidity commented Nov 13, 2023

arvidn left a 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

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

coveralls-official bot commented Nov 16, 2023

Pull Request Test Coverage Report for Build 6871666220

💛 - 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

Rigidity commented Nov 12, 2023 •

edited

Loading