Merge #265: Remove WitnessNode

61b7ddc refactor: Remove WitnessNode (Christian Lewe)
3be0fd9 refactor: Remove Error::NoMoreWitnesses (Christian Lewe)

Pull request description:

  Remove `WitnessNode` by merging its code with the nearly identical `ConstructNode`.

ACKs for top commit:
  apoelstra:
    ACK 61b7ddc; successfully ran local tests

Tree-SHA512: 1ab95ad4b73552cfa9285f7aeba10faa2c4936f708f722667c3c44f3ce1463351ca23319129fe0aa6640004ce665bb23fc6faa754c1c9ea81b71e83d06f62bb0

Author: apoelstra

src/bit_encoding/decode.rs

@@ -9,7 +9,7 @@ use crate::dag::{Dag, DagLike, InternalSharing};
 use crate::jet::Jet;
 use crate::merkle::cmr::Cmr;
 use crate::node::{
-    ConstructNode, CoreConstructible, DisconnectConstructible, JetConstructible, NoWitness,
+    ConstructNode, CoreConstructible, DisconnectConstructible, JetConstructible,
     WitnessConstructible,
 };
 use crate::types;
@@ -235,7 +235,7 @@ pub fn decode_expression<I: Iterator<Item = u8>, J: Jet>(
                 converted[i].get()?,
                 &Some(Arc::clone(converted[j].get()?)),
             )?),
-            DecodeNode::Witness => Node(ArcNode::witness(&inference_context, NoWitness)),
+            DecodeNode::Witness => Node(ArcNode::witness(&inference_context, None)),
             DecodeNode::Fail(entropy) => Node(ArcNode::fail(&inference_context, entropy)),
             DecodeNode::Hidden(cmr) => {
                 if !hidden_set.insert(cmr) {

src/human_encoding/mod.rs

@@ -13,7 +13,7 @@ mod parse;
 use crate::dag::{DagLike, MaxSharing};
 use crate::jet::Jet;
 use crate::node::{self, CommitNode, NoWitness};
-use crate::{Cmr, Imr, Value, WitnessNode};
+use crate::{Cmr, ConstructNode, Imr, Value};
 
 use std::collections::HashMap;
 use std::str;
@@ -213,9 +213,9 @@ impl<J: Jet> Forest<J> {
     pub fn to_witness_node(
         &self,
         witness: &HashMap<Arc<str>, Value>,
-    ) -> Option<Arc<WitnessNode<J>>> {
+    ) -> Option<Arc<ConstructNode<J>>> {
         let main = self.roots.get("main")?;
-        Some(main.to_witness_node(witness, self.roots()))
+        Some(main.to_construct_node(witness, self.roots()))
     }
 }
 

src/human_encoding/named_node.rs

@@ -6,13 +6,12 @@ use crate::dag::{InternalSharing, MaxSharing, PostOrderIterItem};
 use crate::human_encoding::{Error, ErrorSet, Position, WitnessOrHole};
 use crate::jet::Jet;
 use crate::node::{
-    self, Commit, CommitData, CommitNode, Converter, Inner, NoDisconnect, NoWitness, Node, Witness,
-    WitnessData,
+    self, Commit, CommitData, CommitNode, Construct, ConstructData, Constructible as _, Converter,
+    CoreConstructible as _, Inner, NoDisconnect, NoWitness, Node,
 };
-use crate::node::{Construct, ConstructData, Constructible as _, CoreConstructible as _};
 use crate::types;
 use crate::types::arrow::{Arrow, FinalArrow};
-use crate::{encode, Value, WitnessNode};
+use crate::{encode, ConstructNode, Value};
 use crate::{BitWriter, Cmr, Imr};
 
 use std::collections::HashMap;
@@ -108,19 +107,19 @@ impl<J: Jet> NamedCommitNode<J> {
             .unwrap()
     }
 
-    pub fn to_witness_node(
+    pub fn to_construct_node(
         &self,
         witness: &HashMap<Arc<str>, Value>,
         disconnect: &HashMap<Arc<str>, Arc<NamedCommitNode<J>>>,
-    ) -> Arc<WitnessNode<J>> {
+    ) -> Arc<ConstructNode<J>> {
         struct Populator<'a, J: Jet> {
             witness_map: &'a HashMap<Arc<str>, Value>,
             disconnect_map: &'a HashMap<Arc<str>, Arc<NamedCommitNode<J>>>,
             inference_context: types::Context,
             phantom: PhantomData<J>,
         }
 
-        impl<J: Jet> Converter<Named<Commit<J>>, Witness<J>> for Populator<'_, J> {
+        impl<J: Jet> Converter<Named<Commit<J>>, Construct<J>> for Populator<'_, J> {
             type Error = ();
 
             fn convert_witness(
@@ -140,9 +139,9 @@ impl<J: Jet> NamedCommitNode<J> {
             fn convert_disconnect(
                 &mut self,
                 data: &PostOrderIterItem<&Node<Named<Commit<J>>>>,
-                maybe_converted: Option<&Arc<Node<Witness<J>>>>,
+                maybe_converted: Option<&Arc<Node<Construct<J>>>>,
                 _: &Arc<str>,
-            ) -> Result<Option<Arc<Node<Witness<J>>>>, Self::Error> {
+            ) -> Result<Option<Arc<Node<Construct<J>>>>, Self::Error> {
                 if let Some(converted) = maybe_converted {
                     Ok(Some(converted.clone()))
                 } else {
@@ -172,16 +171,16 @@ impl<J: Jet> NamedCommitNode<J> {
                 &mut self,
                 _: &PostOrderIterItem<&Node<Named<Commit<J>>>>,
                 inner: Inner<
-                    &Arc<Node<Witness<J>>>,
+                    &Arc<Node<Construct<J>>>,
                     J,
-                    &Option<Arc<WitnessNode<J>>>,
+                    &Option<Arc<ConstructNode<J>>>,
                     &Option<Value>,
                 >,
-            ) -> Result<WitnessData<J>, Self::Error> {
+            ) -> Result<ConstructData<J>, Self::Error> {
                 let inner = inner
                     .map(|node| node.cached_data())
                     .map_witness(|maybe_value| maybe_value.clone());
-                Ok(WitnessData::from_inner(&self.inference_context, inner)
+                Ok(ConstructData::from_inner(&self.inference_context, inner)
                     .expect("types are already finalized"))
             }
         }
@@ -260,7 +259,7 @@ impl<J: Jet> NamedConstructNode<J> {
                 .as_ref()
                 .map(|data| &data.cached_data().internal)
                 .map_disconnect(|_| &None)
-                .map_witness(|_| NoWitness),
+                .map_witness(|_| None),
         )?;
         let named_data = NamedConstructData {
             internal: construct_data,

src/human_encoding/parse/mod.rs

@@ -591,7 +591,7 @@ mod tests {
                 assert_eq!(main.cmr().to_string(), cmr);
 
                 let program = main
-                    .to_witness_node(witness, &forest)
+                    .to_construct_node(witness, &forest)
                     .finalize_unpruned()
                     .expect("finalize");
 

src/lib.rs

@@ -65,7 +65,7 @@ pub use crate::merkle::{
     tmr::Tmr,
     FailEntropy, HasCmr,
 };
-pub use crate::node::{CommitNode, ConstructNode, Hiding, RedeemNode, WitnessNode};
+pub use crate::node::{CommitNode, ConstructNode, Hiding, RedeemNode};
 pub use crate::value::{Value, Word};
 pub use simplicity_sys as ffi;
 use std::fmt;
@@ -88,8 +88,6 @@ pub enum Error {
     Type(types::Error),
     // Execution error
     Execution(bit_machine::ExecutionError),
-    /// Witness iterator ended early
-    NoMoreWitnesses,
     /// Tried to parse a jet but the name wasn't recognized
     InvalidJetName(String),
     /// Policy error
@@ -107,7 +105,6 @@ impl fmt::Display for Error {
             Error::Type(ref e) => fmt::Display::fmt(e, f),
             Error::Execution(ref e) => fmt::Display::fmt(e, f),
             Error::InvalidJetName(s) => write!(f, "unknown jet `{}`", s),
-            Error::NoMoreWitnesses => f.write_str("no more witness data available"),
             #[cfg(feature = "elements")]
             Error::Policy(ref e) => fmt::Display::fmt(e, f),
         }
@@ -121,7 +118,6 @@ impl std::error::Error for Error {
             Error::DisconnectRedeemTime => None,
             Error::Type(ref e) => Some(e),
             Error::Execution(ref e) => Some(e),
-            Error::NoMoreWitnesses => None,
             Error::InvalidJetName(..) => None,
             #[cfg(feature = "elements")]
             Error::Policy(ref e) => Some(e),

src/node/commit.rs

@@ -3,7 +3,7 @@
 use crate::dag::{DagLike, MaxSharing, NoSharing, PostOrderIterItem};
 use crate::jet::Jet;
 use crate::types::arrow::{Arrow, FinalArrow};
-use crate::{encode, types};
+use crate::{encode, types, Value};
 use crate::{Amr, BitIter, BitWriter, Cmr, Error, FirstPassImr, Imr};
 
 use super::{
@@ -213,8 +213,8 @@ impl<J: Jet> CommitNode<J> {
                 &mut self,
                 _: &PostOrderIterItem<&CommitNode<J>>,
                 _: &NoWitness,
-            ) -> Result<NoWitness, Self::Error> {
-                Ok(NoWitness)
+            ) -> Result<Option<Value>, Self::Error> {
+                Ok(None)
             }
 
             fn convert_disconnect(
@@ -229,7 +229,12 @@ impl<J: Jet> CommitNode<J> {
             fn convert_data(
                 &mut self,
                 _: &PostOrderIterItem<&CommitNode<J>>,
-                inner: Inner<&Arc<ConstructNode<J>>, J, &Option<Arc<ConstructNode<J>>>, &NoWitness>,
+                inner: Inner<
+                    &Arc<ConstructNode<J>>,
+                    J,
+                    &Option<Arc<ConstructNode<J>>>,
+                    &Option<Value>,
+                >,
             ) -> Result<ConstructData<J>, Self::Error> {
                 let inner = inner
                     .map(|node| node.arrow())

src/node/construct.rs

@@ -1,18 +1,17 @@
 // SPDX-License-Identifier: CC0-1.0
 
 use crate::dag::{InternalSharing, PostOrderIterItem};
-use crate::encode;
 use crate::jet::Jet;
 use crate::types::{self, arrow::Arrow};
-use crate::{BitIter, BitWriter, Cmr, FailEntropy};
+use crate::{encode, BitIter, BitWriter, Cmr, FailEntropy, RedeemNode, Value, Word};
 
-use crate::value::Word;
 use std::io;
 use std::marker::PhantomData;
 use std::sync::Arc;
 
 use super::{
     Commit, CommitData, CommitNode, Converter, Inner, Marker, NoDisconnect, NoWitness, Node,
+    Redeem, RedeemData,
 };
 use super::{CoreConstructible, DisconnectConstructible, JetConstructible, WitnessConstructible};
 
@@ -33,7 +32,7 @@ pub struct Construct<J> {
 
 impl<J: Jet> Marker for Construct<J> {
     type CachedData = ConstructData<J>;
-    type Witness = NoWitness;
+    type Witness = Option<Value>;
     type Disconnect = Option<Arc<ConstructNode<J>>>;
     type SharingId = ConstructId;
     type Jet = J;
@@ -90,7 +89,7 @@ impl<J: Jet> ConstructNode<J> {
             fn convert_witness(
                 &mut self,
                 _: &PostOrderIterItem<&ConstructNode<J>>,
-                _: &NoWitness,
+                _: &Option<Value>,
             ) -> Result<NoWitness, Self::Error> {
                 Ok(NoWitness)
             }
@@ -119,6 +118,104 @@ impl<J: Jet> ConstructNode<J> {
         self.convert::<InternalSharing, _, _>(&mut FinalizeTypes(PhantomData))
     }
 
+    /// Finalize the witness program as an unpruned redeem program.
+    ///
+    /// Witness nodes must be populated with sufficient data,
+    /// to ensure that the resulting redeem program successfully runs on the Bit Machine.
+    /// Furthermore, **all** disconnected branches must be populated,
+    /// even those that are not executed.
+    ///
+    /// The resulting redeem program is **not pruned**.
+    ///
+    /// ## See
+    ///
+    /// [`RedeemNode::prune`]
+    pub fn finalize_unpruned(&self) -> Result<Arc<RedeemNode<J>>, crate::Error> {
+        struct Finalizer<J>(PhantomData<J>);
+
+        impl<J: Jet> Converter<Construct<J>, Redeem<J>> for Finalizer<J> {
+            type Error = crate::Error;
+
+            fn convert_witness(
+                &mut self,
+                data: &PostOrderIterItem<&ConstructNode<J>>,
+                wit: &Option<Value>,
+            ) -> Result<Value, Self::Error> {
+                if let Some(ref wit) = wit {
+                    Ok(wit.shallow_clone())
+                } else {
+                    // We insert a zero value into unpopulated witness nodes,
+                    // assuming that this node will later be pruned out of the program.
+                    //
+                    // Pruning requires running a program on the Bit Machine,
+                    // which in turn requires a program with fully populated witness nodes.
+                    // It would be horrible UX to force the caller to provide witness data
+                    // even for unexecuted branches, so we insert zero values here.
+                    //
+                    // If this node is executed after all, then the caller can fix the witness
+                    // data based on the returned execution error.
+                    //
+                    // Zero values may "accidentally" satisfy a program even if the caller
+                    // didn't provide any witness data. However, this is only the case for the
+                    // most trivial programs. The only place where we must be careful is our
+                    // unit tests, which tend to include these kinds of trivial programs.
+                    let ty = data.node.arrow().target.finalize()?;
+                    Ok(Value::zero(&ty))
+                }
+            }
+
+            fn convert_disconnect(
+                &mut self,
+                _: &PostOrderIterItem<&ConstructNode<J>>,
+                maybe_converted: Option<&Arc<RedeemNode<J>>>,
+                _: &Option<Arc<ConstructNode<J>>>,
+            ) -> Result<Arc<RedeemNode<J>>, Self::Error> {
+                if let Some(child) = maybe_converted {
+                    Ok(Arc::clone(child))
+                } else {
+                    Err(crate::Error::DisconnectRedeemTime)
+                }
+            }
+
+            fn convert_data(
+                &mut self,
+                data: &PostOrderIterItem<&ConstructNode<J>>,
+                inner: Inner<&Arc<RedeemNode<J>>, J, &Arc<RedeemNode<J>>, &Value>,
+            ) -> Result<Arc<RedeemData<J>>, Self::Error> {
+                let converted_data = inner
+                    .map(|node| node.cached_data())
+                    .map_disconnect(|node| node.cached_data())
+                    .map_witness(Value::shallow_clone);
+                Ok(Arc::new(RedeemData::new(
+                    data.node.arrow().finalize()?,
+                    converted_data,
+                )))
+            }
+        }
+
+        self.convert::<InternalSharing, _, _>(&mut Finalizer(PhantomData))
+    }
+
+    /// Finalize the witness program as a pruned redeem program.
+    ///
+    /// Witness nodes must be populated with sufficient data,
+    /// to ensure that the resulting redeem program successfully runs on the Bit Machine.
+    /// Furthermore, **all** disconnected branches must be populated,
+    /// even those that are not executed.
+    ///
+    /// The resulting redeem program is **pruned** based on the given transaction environment.
+    ///
+    /// ## See
+    ///
+    /// [`RedeemNode::prune`]
+    pub fn finalize_pruned(
+        &self,
+        env: &J::Environment,
+    ) -> Result<Arc<RedeemNode<J>>, crate::Error> {
+        let unpruned = self.finalize_unpruned()?;
+        unpruned.prune(env).map_err(crate::Error::Execution)
+    }
+
     /// Decode a Simplicity expression from bits, without witness data.
     ///
     /// # Usage
@@ -278,10 +375,10 @@ impl<J: Jet> DisconnectConstructible<Option<Arc<ConstructNode<J>>>> for Construc
     }
 }
 
-impl<J> WitnessConstructible<NoWitness> for ConstructData<J> {
-    fn witness(inference_context: &types::Context, witness: NoWitness) -> Self {
+impl<J> WitnessConstructible<Option<Value>> for ConstructData<J> {
+    fn witness(inference_context: &types::Context, _witness: Option<Value>) -> Self {
         ConstructData {
-            arrow: Arrow::witness(inference_context, witness),
+            arrow: Arrow::witness(inference_context, NoWitness),
             phantom: PhantomData,
         }
     }
@@ -340,7 +437,7 @@ mod tests {
     fn occurs_check_3() {
         let ctx = types::Context::new();
         // A similar example that caused a slightly different deadlock in the past.
-        let wit = Arc::<ConstructNode<Core>>::witness(&ctx, NoWitness);
+        let wit = Arc::<ConstructNode<Core>>::witness(&ctx, None);
         let drop = Arc::<ConstructNode<Core>>::drop_(&wit);
 
         let comp1 = Arc::<ConstructNode<Core>>::comp(&drop, &drop).unwrap();

src/node/convert.rs

@@ -152,6 +152,10 @@ pub trait Converter<N: Marker, M: Marker> {
 /// Does not do any type-checking and may attach an invalid witness to a combinator.
 ///
 /// If it encounters a disconnect node, it simply returns an error.
+///
+/// **This finalizer should not be used in production.
+/// It introduces default values ([`Value::zero`]) to handle missing witness data,
+/// which might trigger unexpected spending paths.**
 // FIXME we should do type checking, but this would require a method to check
 // type compatibility between a Value and a type::Final.
 pub struct SimpleFinalizer<W: Iterator<Item = Value>> {
@@ -169,10 +173,13 @@ impl<W: Iterator<Item = Value>, J: Jet> Converter<Commit<J>, Redeem<J>> for Simp
 
     fn convert_witness(
         &mut self,
-        _: &PostOrderIterItem<&CommitNode<J>>,
+        data: &PostOrderIterItem<&CommitNode<J>>,
         _: &NoWitness,
     ) -> Result<Value, Self::Error> {
-        self.iter.next().ok_or(crate::Error::NoMoreWitnesses)
+        Ok(self
+            .iter
+            .next()
+            .unwrap_or_else(|| Value::zero(&data.node.arrow().target)))
     }
 
     fn convert_disconnect(