[BugFix] Make PSBT serializations 174 compliant by excluding empty taptree from serialization

[JeremyRubin]

According to BIP-174, the tap_tree field must be:

One or more tuples representing the depth, leaf version, and script for a leaf in the Taproot tree, allowing the entire tree to be reconstructed. The tuples must be in depth first search order so that the tree is correctly reconstructed. Each tuple is an 8-bit unsigned integer representing the depth in the Taproot tree for this script, an 8-bit unsigned integer representing the leaf version, the length of the script as a compact size unsigned integer, and the script itself.

allowing serializing an empty tap tree tuple breaks the "one or more"ness of the field, so serializations should ignore empty but initialized tap_trees.

see also rust-bitcoin/rust-bitcoin#1194, where this caused downstream issues.

[achow101]

In what situation are we creating empty output taprtrees? It seems like the only way this can happen is if a PSBT created by something else is being processed. In that case, we should reject the empty taptree during parsing rather than accepting the ostensibly bad PSBT and silently dropping the bad field. The general principle for PSBT is if it came in or if we filled the field internally, then it should be serialized. So changing the serialization in this way is incorrect IMO. Rather we should not accept empty taptrees and make it impossible to add one to the psbt in our own processing.

[JeremyRubin]

Here is a script you can run to generate the bad behavior. It is not using any external tools.

I am indifferent on if the empty TapTree is not serialized, or if it is an Option null.

ADDR=$(./bitcoin-cli -signet getnewaddress "" bech32m)
TXID=$(./bitcoin-cli -signet send "{\"$ADDR\":20}" | jq '.txid' | xargs echo)
VOUT=$(./bitcoin-cli -signet gettransaction $TXID true true | jq "(.decoded.vout | map(select(.scriptPubKey.address == \"$ADDR\")))[].n")
INPUT="[{\"txid\":\"$TXID\",\"vout\":$VOUT}]"
ADDR2=$(./bitcoin-cli -signet getnewaddress "" bech32m)
PSBT=$(./bitcoin-cli -signet walletcreatefundedpsbt "$INPUT" "[{\"$ADDR2\": 0.1}]" | jq ".psbt" | xargs echo)
echo "PSBT IS: " $PSBT

on master:

cHNidP8BAIkCAAAAAapfm08b0MipBvW9thL06f8rMbeazW7TIa0W9plHj4WoAAAAAAD9////AoCWmAAAAAAAIlEgC+blBlIP1iijRWxqjw1u9H02sqr7y8fno6/LdnvGqPl895x2AAAAACJRIM5wyjSexMbADl4K+AI1/68zyaDlE7guKvrEDUAjwqU1AAAAAAABASsAlDV3AAAAACJRIDfCpO/CIAqc0JKgBhsCfaPGdyroYtmH+4gQK/Mnn72UIRZGOixxmh9h2gqDIecYHcQHRa8w+Sokc//iDiqXz7uMGRkAHzYIzlYAAIABAACAAAAAgAAAAABhAAAAARcgRjoscZofYdoKgyHnGB3EB0WvMPkqJHP/4g4ql8+7jBkAAQUg1YCB33LpmkGemw3ncz7fcnjhL/bBG/PjH8vpgr2L3cUBBgAhB9WAgd9y6ZpBnpsN53M+33J44S/2wRvz4x/L6YK9i93FGQAfNgjOVgAAgAEAAIAAAACAAAAAAGIAAAAAAQUg9jMNus8cd+GAosBk9wn+pNP9wn7A+jy2Vq0cy+siJ8wBBgAhB/YzDbrPHHfhgKLAZPcJ/qTT/cJ+wPo8tlatHMvrIifMGQAfNgjOVgAAgAEAAIAAAACAAQAAAFEBAAAA

with this patch

cHNidP8BAIkCAAAAATpwsU9MR76awftD8FkbSHpWSl7aXJShh0Zt/Qhs2nxxAAAAAAD9////AoCWmAAAAAAAIlEgXVnPV8AvZ8aaQrx5yD9j3f/2sC5HyZRFZSiAmJpNTMx895x2AAAAACJRINESxWA49wL5zMro5bHRFQs1uYXY/rkh7YHWo8ryt4pNAAAAAAABASsAlDV3AAAAACJRIGnS8YOeOIEB3SGewSxxyHAVyL1G1hKZpzDMNablWo/hIRaSI43XyWhcnQ9kvKrZQuFwJhg/MtXjsYIoywerZApowBkAHzYIzlYAAIABAACAAAAAgAAAAABdAAAAARcgkiON18loXJ0PZLyq2ULhcCYYPzLV47GCKMsHq2QKaMAAAQUgyUyJb6VUpCjCmi7Z1CGvPn6K3BFlzxPpsJ3szGn4eQMhB8lMiW+lVKQowpou2dQhrz5+itwRZc8T6bCd7Mxp+HkDGQAfNgjOVgAAgAEAAIAAAACAAAAAAF4AAAAAAQUgsx/g/7PwwPDAdHz2Qvl1IHuYKRppAYZoSAqyyEU9DXohB7Mf4P+z8MDwwHR89kL5dSB7mCkaaQGGaEgKsshFPQ16GQAfNgjOVgAAgAEAAIAAAACAAQAAAEkBAAAA

decoded master:

{
  "tx": {
    "txid": "aae4ec9b7a539b7ac60ad39dda5c3a537aaf5c2b42f744343f1c110af156d0fa",
    "hash": "aae4ec9b7a539b7ac60ad39dda5c3a537aaf5c2b42f744343f1c110af156d0fa",
    "version": 2,
    "size": 137,
    "vsize": 137,
    "weight": 548,
    "locktime": 0,
    "vin": [
      {
        "txid": "a8858f4799f616ad21d36ecd9ab7312bffe9f412b6bdf506a9c8d01b4f9b5faa",
        "vout": 0,
        "scriptSig": {
          "asm": "",
          "hex": ""
        },
        "sequence": 4294967293
      }
    ],
    "vout": [
      {
        "value": 0.10000000,
        "n": 0,
        "scriptPubKey": {
          "asm": "1 0be6e506520fd628a3456c6a8f0d6ef47d36b2aafbcbc7e7a3afcb767bc6a8f9",
          "desc": "rawtr(0be6e506520fd628a3456c6a8f0d6ef47d36b2aafbcbc7e7a3afcb767bc6a8f9)#sfw45af0",
          "hex": "51200be6e506520fd628a3456c6a8f0d6ef47d36b2aafbcbc7e7a3afcb767bc6a8f9",
          "address": "tb1pp0nw2pjjpltz3g69d34g7rtw737ndv42l09u0ear4l9hv77x4rusu9dad7",
          "type": "witness_v1_taproot"
        }
      },
      {
        "value": 19.89998460,
        "n": 1,
        "scriptPubKey": {
          "asm": "1 ce70ca349ec4c6c00e5e0af80235ffaf33c9a0e513b82e2afac40d4023c2a535",
          "desc": "rawtr(ce70ca349ec4c6c00e5e0af80235ffaf33c9a0e513b82e2afac40d4023c2a535)#sydncjf5",
          "hex": "5120ce70ca349ec4c6c00e5e0af80235ffaf33c9a0e513b82e2afac40d4023c2a535",
          "address": "tb1peecv5dy7cnrvqrj7ptuqyd0l4ueung89zwuzu2h6csx5qg7z556sjz72rf",
          "type": "witness_v1_taproot"
        }
      }
    ]
  },
  "global_xpubs": [
  ],
  "psbt_version": 0,
  "proprietary": [
  ],
  "unknown": {
  },
  "inputs": [
    {
      "witness_utxo": {
        "amount": 20.00000000,
        "scriptPubKey": {
          "asm": "1 37c2a4efc2200a9cd092a0061b027da3c6772ae862d987fb88102bf3279fbd94",
          "desc": "rawtr(37c2a4efc2200a9cd092a0061b027da3c6772ae862d987fb88102bf3279fbd94)#ryczaytf",
          "hex": "512037c2a4efc2200a9cd092a0061b027da3c6772ae862d987fb88102bf3279fbd94",
          "address": "tb1pxlp2fm7zyq9fe5yj5qrpkqna50r8w2hgvtvc07ugzq4lxfulhk2qlkpf56",
          "type": "witness_v1_taproot"
        }
      },
      "taproot_bip32_derivs": [
        {
          "pubkey": "463a2c719a1f61da0a8321e7181dc40745af30f92a2473ffe20e2a97cfbb8c19",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/0/97",
          "leaf_hashes": [
          ]
        }
      ],
      "taproot_internal_key": "463a2c719a1f61da0a8321e7181dc40745af30f92a2473ffe20e2a97cfbb8c19"
    }
  ],
  "outputs": [
    {
      "taproot_internal_key": "d58081df72e99a419e9b0de7733edf7278e12ff6c11bf3e31fcbe982bd8bddc5",
      "taproot_tree": [
      ],
      "taproot_bip32_derivs": [
        {
          "pubkey": "d58081df72e99a419e9b0de7733edf7278e12ff6c11bf3e31fcbe982bd8bddc5",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/0/98",
          "leaf_hashes": [
          ]
        }
      ]
    },
    {
      "taproot_internal_key": "f6330dbacf1c77e180a2c064f709fea4d3fdc27ec0fa3cb656ad1ccbeb2227cc",
      "taproot_tree": [
      ],
      "taproot_bip32_derivs": [
        {
          "pubkey": "f6330dbacf1c77e180a2c064f709fea4d3fdc27ec0fa3cb656ad1ccbeb2227cc",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/1/337",
          "leaf_hashes": [
          ]
        }
      ]
    }
  ],
  "fee": 0.00001540
}

decode patched

{
  "tx": {
    "txid": "4e7d1ab0ae861c82a973c23bf13cebf24ce3af009c591a4169ea56b95e72b92a",
    "hash": "4e7d1ab0ae861c82a973c23bf13cebf24ce3af009c591a4169ea56b95e72b92a",
    "version": 2,
    "size": 137,
    "vsize": 137,
    "weight": 548,
    "locktime": 0,
    "vin": [
      {
        "txid": "717cda6c08fd6d4687a1945cda5e4a567a481b59f043fbc19abe474c4fb1703a",
        "vout": 0,
        "scriptSig": {
          "asm": "",
          "hex": ""
        },
        "sequence": 4294967293
      }
    ],
    "vout": [
      {
        "value": 0.10000000,
        "n": 0,
        "scriptPubKey": {
          "asm": "1 5d59cf57c02f67c69a42bc79c83f63ddfff6b02e47c99445652880989a4d4ccc",
          "desc": "rawtr(5d59cf57c02f67c69a42bc79c83f63ddfff6b02e47c99445652880989a4d4ccc)#qa5395xa",
          "hex": "51205d59cf57c02f67c69a42bc79c83f63ddfff6b02e47c99445652880989a4d4ccc",
          "address": "tb1pt4vu747q9anudxjzh3uus0mrmhlldvpwglyeg3t99zqf3xjdfnxqpsn99y",
          "type": "witness_v1_taproot"
        }
      },
      {
        "value": 19.89998460,
        "n": 1,
        "scriptPubKey": {
          "asm": "1 d112c56038f702f9cccae8e5b1d1150b35b985d8feb921ed81d6a3caf2b78a4d",
          "desc": "rawtr(d112c56038f702f9cccae8e5b1d1150b35b985d8feb921ed81d6a3caf2b78a4d)#z88xvjpv",
          "hex": "5120d112c56038f702f9cccae8e5b1d1150b35b985d8feb921ed81d6a3caf2b78a4d",
          "address": "tb1p6yfv2cpc7up0nnx2arjmr5g4pv6mnpwcl6ujrmvp663u4u4h3fxsrqclw3",
          "type": "witness_v1_taproot"
        }
      }
    ]
  },
  "global_xpubs": [
  ],
  "psbt_version": 0,
  "proprietary": [
  ],
  "unknown": {
  },
  "inputs": [
    {
      "witness_utxo": {
        "amount": 20.00000000,
        "scriptPubKey": {
          "asm": "1 69d2f1839e388101dd219ec12c71c87015c8bd46d61299a730cc35a6e55a8fe1",
          "desc": "rawtr(69d2f1839e388101dd219ec12c71c87015c8bd46d61299a730cc35a6e55a8fe1)#lcncmzh8",
          "hex": "512069d2f1839e388101dd219ec12c71c87015c8bd46d61299a730cc35a6e55a8fe1",
          "address": "tb1pd8f0rqu78zqsrhfpnmqjcuwgwq2u302x6cffnfeses66de263lss3w2r37",
          "type": "witness_v1_taproot"
        }
      },
      "taproot_bip32_derivs": [
        {
          "pubkey": "92238dd7c9685c9d0f64bcaad942e17026183f32d5e3b18228cb07ab640a68c0",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/0/93",
          "leaf_hashes": [
          ]
        }
      ],
      "taproot_internal_key": "92238dd7c9685c9d0f64bcaad942e17026183f32d5e3b18228cb07ab640a68c0"
    }
  ],
  "outputs": [
    {
      "taproot_internal_key": "c94c896fa554a428c29a2ed9d421af3e7e8adc1165cf13e9b09deccc69f87903",
      "taproot_bip32_derivs": [
        {
          "pubkey": "c94c896fa554a428c29a2ed9d421af3e7e8adc1165cf13e9b09deccc69f87903",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/0/94",
          "leaf_hashes": [
          ]
        }
      ]
    },
    {
      "taproot_internal_key": "b31fe0ffb3f0c0f0c0747cf642f975207b98291a69018668480ab2c8453d0d7a",
      "taproot_bip32_derivs": [
        {
          "pubkey": "b31fe0ffb3f0c0f0c0747cf642f975207b98291a69018668480ab2c8453d0d7a",
          "master_fingerprint": "1f3608ce",
          "path": "m/86'/1'/0'/1/329",
          "leaf_hashes": [
          ]
        }
      ]
    }
  ],
  "fee": 0.00001540
}

[JeremyRubin]

(I also find the empty leaf_hashes field a bit sus, but couldn't find anything in the spec around if they are allowed to be empty)

[JeremyRubin]

On further research, I do think this is the correct behavior, but the issue is that the field name m_tap_tree should be refactored (in follow up work?) to be m_taproot_builder, because it is the builder (which also contains the pubkey if key-only). The "true" field of tap_tree is only available when derived from taproot_builder by calling the function to check for presence of branches.

Thus there is no "stopping this issue at the source", it's just a logic bug in serializing of PSBT from core.

[achow101]

I think the correct behavior is to change PSBTOutput::FromSignatureData to only fill in m_tap_tree when SignatureData.tr_builder has a merkle root. We should also check that the tree is not empty when deserializing

Also I think the decdepsbt change is strictly incorrect. If the field is there and we did not fail parsing, then we need to output it when decoding. Otherwise we cannot trust decodepsbt for tests to be sure that unexpected fields do not sneak in.

This could also use a test.

[JeremyRubin]

Added a patch to prevent invalid deserialization. I think that with that patch, the approach used here is sufficient because not deserializing an empty Tap Tree is something that can, now, only be the result of an internal logic error.

W.r.t. "preventing it at the source", I tried this patch independent of any others, but it didn't seem to work. Maybe you can figure out where the bad state is coming from?

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 9bcbe1cee..f6a67be5a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -885,20 +885,28 @@ class TRDescriptor final : public DescriptorImpl
 protected:
     std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript> scripts, FlatSigningProvider& out) const override
     {
-        TaprootBuilder builder;
+        std::optional<TaprootBuilder> builder;
         assert(m_depths.size() == scripts.size());
         for (size_t pos = 0; pos < m_depths.size(); ++pos) {
-            builder.Add(m_depths[pos], scripts[pos], TAPROOT_LEAF_TAPSCRIPT);
+            if (!builder.has_value()) {
+                builder = TaprootBuilder{};
+            }
+            builder->Add(m_depths[pos], scripts[pos], TAPROOT_LEAF_TAPSCRIPT);
         }
-        if (!builder.IsComplete()) return {};
+        if (builder && !builder->IsComplete()) return {};
         assert(keys.size() == 1);
         XOnlyPubKey xpk(keys[0]);
         if (!xpk.IsFullyValid()) return {};
-        builder.Finalize(xpk);
-        WitnessV1Taproot output = builder.GetOutput();
-        out.tr_trees[output] = builder;
-        out.pubkeys.emplace(keys[0].GetID(), keys[0]);
-        return Vector(GetScriptForDestination(output));
+        if (builder) {
+            builder->Finalize(xpk);
+            WitnessV1Taproot output = builder->GetOutput();
+            out.tr_trees[output] = *builder;
+            out.pubkeys.emplace(keys[0].GetID(), keys[0]);
+            return Vector(GetScriptForDestination(output));
+        } else {
+            out.pubkeys.emplace(keys[0].GetID(), keys[0]);
+            return Vector(GetScriptForDestination(WitnessV1Taproot(xpk)));
+        }
     }
     bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const override
     {

[achow101]

Alternative in #25858

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25877 (refactor: Do not use CScript for tapleaf scripts until the tapleaf version is known by roconnor-blockstream)
• #25858 (psbt: Only include PSBT_OUT_TAP_TREE when the output has a script path by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[fanquake]

Going to close this in favour of #25858 for now, as it seems that PR has a number of concept/approach ACKs (including from @JeremyRubin). Jeremy please let me know if this is incorrect.