refactors to prepare for the new rust-simplicity version #143
+219
−207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apoelstra
force-pushed
the
2025-09/rust-simplicity-prep
branch
from
d45acd1 to
96e7df2
Compare
|
cc @canndrew can you take a look at this one? |
|
Thanks! My local tests show that actually I need to rebase though :/. |
This Default impl creates a new type inference context which then gets attached to a unit ConstructNode. This will be incompatible with the upstream change to make type inference contexts tied to scopes.
…ic ones Now we have a marker type WithNames which wraps a marker type from rust-simplicity, erasing its disconnect nodes and replacing its witness nodes with names. This gets us an analogue of ConstructNode, CommitNode, WitnessNode, RedeemNode, etc., all at once. Then replace: * to_commit_node, whose role was to forget names and finalize types, with a general method forget_names, which just forgets the names (then the user can call .finalize_types() themselves if they want) * to_witness_node, whose role was to replace names with actual witnesses, with a general method populate_witnesses, which does this (Actually, `to_witness_node` is not very general at all. It specifically converts a WtihNames<ConstructNode> to an unnamed ConstructNode. In this commit, it's possible to implement it in a general way, converting an arbitrary Node<WithNames<M>> to a Node<M>. But in a couple commits, when we replace ConstructNode with CommitNode in CompiledProgram, we will be forced to fix its signature to convert a CommitNode to a RedeemNode. The diff will be easier to read starting from this non-generic version than it would be if we started from the generic version.) Along the way, use core::convert::Infallible for error types so that we don't need to unwrap errors. This leaves the precondition on `populate_witnesses` that the user call `is_consistent` on witness nodes before converting from SimplicityHL to Simplicity. I believe I can remove this precondition and return an error if the witness values don't match the Simplicity node, but I need to update the library to get the IncompleteType type first, so that I can return a useful error. So I will defer this improvement.
This method takes a Named<ConstructNode> and produces a Named<CommitNode>. The goal is to finalize all the types without forgetting the names. This version simply
This commit represents an important change: in the user-facing types we are no longer holding ConstructNode objects, which mean that we are not handing the user incomplete types which are tied to a specific inference context. This means that within our code, whenever we need to manipulate types, we need to create a new local type inference context which is then dropped at the end of the function. We'll need this when we update rust-simplicity to a version that has only locally-scoped context objects. The bulk of the diff is in src/named.rs, changing the populate_witness method to convert a CommitNode to a RedeemNode, which is mostly just fixing type signatures by chasing compiler errors. Sorry for the noise. I also add a new stringly-typed error, which I'm not thrilled with, but this crate is already full of them.
Now the only use of ConstructNode, and incomplete types more broadly, occurs within compile.rs. This will make it much easier for us to update rust-simplicity to require scoped type inference contexts.
apoelstra
force-pushed
the
2025-09/rust-simplicity-prep
branch
from
96e7df2 to
1b8606d
Compare
We want to move builtins from the crate root into this module.
This module was introduced in BlockstreamResearch#145 as a public module. But actually it is only ever used by the compiler, and it doesn't really make sense outside the compile because of its use of `ProgNode`. We may want to later make the builtins module be an extension trait of Constructible or something, but as long as it depends on ProgNode it should not be public (and it cannot be public, after the next commit).
Now that no public types contain ConstructNodes, we don't need a type alias for a ConstructNode to be exported. Move this into compile.rs, make it private, and replace a couple isolated uses in pattern.rs with generic ones.
apoelstra
force-pushed
the
2025-09/rust-simplicity-prep
branch
from
1b8606d to
760c51c
Compare
|
Gonna queue this for merge with just Byron's ACK, but would appreciate a belated review from Andrew if he's around. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since BlockstreamResearch/rust-simplicity#305 we are not allowed to have type inference contexts (and therefore
ConstructNodes) that escape the lexical scope that defines them.We currently use
ConstructNodeinsideProgramNode. This PR changes this toCommitNode, which is more correct, as well as being necessary to avoid exposing badGhostCellergonomics to users of this library.