BlockstreamResearch / simfony

Rust-like high-level language that compiles down to Simplicity bytecode. Work in progress.
19 stars 6 forks source link

Add new witness parser #63

Closed uncomputable closed 1 month ago

uncomputable commented 1 month ago

Add a parser for the new witness JSON format:

{
  "name": {
    "value": "42",
    "type": "u8"
  }
}

First we use serde_json to parse the JSON file. Then, we parse the type field as a ResolvedType. Finally, we parse the value field as a const expression that can be evaluated at compile time to a value, namely the witness value. Const expressions enable us to write witness files in the same way we write Simfony programs.

Replace the reimplementation of NamedConstructNode with a simpler reimplementation of ConstructNode where witness nodes contain their name. Other nodes don't need to be named. This change is messy in terms of lines of code, but it should be quite mechanical.

Implement a conversion from NamedConstructNode to WitnessNode, which enables pruning. I haven't tested how well rust-simplicity prunes compiled Simfony programs. We might need to push a followup PR to fix issues if we find them, but there is little we can do on the Simfony side, in my estimation.

We could do with more unit tests, but I felt like pushing this now.

apoelstra commented 1 month ago

In c705ac908470db608dc9b27e280d37e1bdae9ba2:

I don't understand this commit. The description says that it removes generics from Error::from_span, but actually it removes them from Error::with_span. The description says this is okay since Error::with_span "already handles generic inputs" but it doesn't, because you're removing that ability.

This also implements From<Error> for String so that ? can be used to throw away error information. I think this is just bad practice.

apoelstra commented 1 month ago

In 6792d3714f974628fdafc5fad34b9844b351f769:

I also don't think you should have a From<HashMap> impl for WitnessValues. I think you should just add a .insert method on WitnessValues. This will make it easier for you to sanity-check or error in the case that a witness is assigned multiple times, for example.

apoelstra commented 1 month ago

In 698ae51af2b43184edeaa0f12bf6a31ebb67d9bb:

You shouldn't be serde-deserializing an owned String and thenparsing that. This is inefficient and unidiomatic. You should implement a Visitor and use the visit_str method on that. See https://github.com/rust-bitcoin/bitcoin_hashes/blob/master/src/serde_macros.rs

apoelstra commented 1 month ago

Overall 41d025f5d68af9b8777b8d2020e319cff7c1f4d9 looks great, other than the above programs.

I'd like to see an additional test where we try to use the same witness node twice, so we can see what happens.

uncomputable commented 1 month ago

Updated the serde implementations and removed impl From<Error> for String. Added unit tests for witness error paths.

I actually need to update the serde implementation of WitnessValues to return when the same name is assigned multiple values. Wait for the next push.

uncomputable commented 1 month ago

Now the deserialization of WitnessValues checks for duplicate names. Even when the same name is assigned the same value twice, there is an error. When there are unknown fields besides value or type, there is an error. The goal is to keep witness JSON files unambiguous and, minus formatting, "non-malleable". This is also why each value has a type, so strings like "42" can clearly parsed as u8, u16, etc.

@apoelstra Feel free to give this PR another review