Reachable Line in Serialization of Incomple TapTree

[JeremyRubin]

rust-bitcoin/src/util/psbt/serialize.rs

Line 325 in 67e88e7

_ => unreachable!(),

[tcharding]

Would this resolve this issue

diff --git a/src/util/psbt/serialize.rs b/src/util/psbt/serialize.rs
index c10b98f2..d3327f05 100644
--- a/src/util/psbt/serialize.rs
+++ b/src/util/psbt/serialize.rs
@@ -306,6 +306,12 @@ impl Deserialize for (Vec<TapLeafHash>, KeySource) {
 }
 
 impl Serialize for TapTree {
+    /// Serialize a [`TapTree`].
+    ///
+    /// # Panics
+    ///
+    /// Caller is responsible for finalising the tap tree before serialising. Panics if called on a
+    /// non-finalised tap tree.
     fn serialize(&self) -> Vec<u8> {
         match (self.0.branch().len(), self.0.branch().last()) {
             (1, Some(Some(root))) => {
@@ -321,8 +327,7 @@ impl Serialize for TapTree {
                 }
                 buf
             }
-        // This should be unreachable as we Taptree is already finalized
-            _ => unreachable!(),
+            _ => panic!("attempted to serialize a non-finalized taptree"),
         }
     }
 }

[JeremyRubin]

Hah no; the issue is that this line can be reached by attempting to roundtrip PSBTs emitted from e.g. bitcoin core, so we need a patch which can handle serializing incomplete taptrees properly at serialize time.

[tcharding]

I see, thanks for clarifying!

[apoelstra]

There's something of an open design issue here: my understanding is that Core will emit an empty array for "no taptree" (Taptrees are serialized as arrays of tuples of control block data IIRC). There are some other cases where we can hit this but I think this is the interesting one. Our choices are:

• Refuse to deserialize incomplete taptrees on the theory that they're not done and shouldn't be transmitted, adding a special case to deserialize Core's output as an Option<Taptree> or something
• Refuse to deserialize incomplete trees and just reject Core's output, on principled grounds
• Deserialize Core's trees and figure out a sensible way to re-serialize them

One thing that would inform this decision is figuring out how well our current API prevents the construction of incomplete trees. I had thought that the ways to make trees were (a) deserializing from consensus; (b) deserializing from PSBT; (c) contsructing with a TaptreeBuilder and then finalizing ... and we should be able to reject incomplete trees in all cases.

As a final, more general question, @JeremyRubin has suggested that we simply don't panic in serialization code as a rule, on the basis (I think) that serde allows us to return an error and refuse to serialize, and even if this is a "invariant violation, halt and catch fire" situation if we're just finding out in our serialization code then we've already missed our chance to fail early.

[JeremyRubin]

Well, in theory it's not a big deal to refuse to desrialize incomplete trees since Bitcoin Core with the taproot fields is yet to be released according to @achow101, and we're getting a patch for it upstream, so we can nip it in the bud so to speak.

w.r.t. not panicking and returning an error, a must_use result can be "panic'd" at the call site by unwrapping, which lets users choose if they want to panic or not.

Our APIs should actually sufficient for TapTree, the issue was that this code was abusing crate-only visibility of an inner field.