RPC/txoutproof: Support including (and verifying) proofs of wtxid

luke-jr commented at 11:05 pm on June 30, 2025: member

Feature requested by @SomberNight for Electrum’s Lightning stuff. (Please confirm this does what you need)

Unfortunately WIP because the wtxid merkle root calculation doesn’t match and I don’t have time to figure out why not yet (maybe after concept ACK? or if anyone else wants to take a look…)

Mostly backward compatible, but a segwit-enabled proof will verify the generation tx in old versions.

Allow calling GetWitnessCommitmentIndex with just the generation transaction (not a CBlock) 605b372b1f

WIP: RPC/txoutproof: Support including (and verifying) proofs of wtxid 3abfd6d3d9

DrahtBot commented at 11:05 pm on June 30, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32844.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32189 (refactor: Txid type safety (parent PR) by marcofleon)

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.

DrahtBot added the label CI failed on Jul 1, 2025

in src/rpc/txoutproof.cpp:222 in 3abfd6d3d9

220+                std::vector<unsigned int> wtxid_indexes;
221+                const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
222+                if (memcmp(wtxid_root.begin(), &merkleBlock.m_gentx->vout[witness_commit_outidx].scriptPubKey[6], 32)) {
223+                    // FIXME: For some reason, this isn't matching currently
224+                    LogPrintf("%s vs %s DEBUG", wtxid_root.GetHex(), HexStr(MakeUCharSpan(merkleBlock.m_gentx->vout[witness_commit_outidx].scriptPubKey)));
225+                    return {};

SomberNight commented at 0:13 am on July 1, 2025:

Is the cause of the mismatch that you thought the scriptPubKey directly contains the wtxid tree root? It actually contains Double-SHA256(witness root hash|witness reserved value) So you also have to extract the witness reserved value from the coinbase input witness. see https://github.com/bitcoin/bips/blob/77b0bb297ad4e4e6b04b2c078c5e3b620a7090e2/bip-0141.mediawiki#commitment-structure

SomberNight commented at 0:25 am on July 1, 2025: contributor

Feature requested by @SomberNight for Electrum’s Lightning stuff. (Please confirm this does what you need)

Wow, thank you for working on this! I was meaning to open an issue to ask for this, but did not even get to that yet.

The request I would like an electrum server, assuming txindex=1, to be able to serve is:

bc.tx.get_merkle_witness(txid), (so given just a txid,)

return a dict containing:

wtxid
block_height
block_hash
pos (index of tx in block)
merkle or wmerkle
- exactly one of the two
- merkle is the Satoshi-SPV proof, in case there is no witness commitment in the block
- wmerkle is the witness-SPV proof otherwise
cb_tx, the generation/coinbase tx
cb_proof, the Satoshi-SPV proof for the generation tx

Obviously the electrum server can act as middleware and transform the response, assuming the necessary fields are available or obtainable.

As far as I can tell, but I haven’t tried yet, the RPC response from this PR can be transformed to what I describe above. So the RPC looks to be doing what I need. :)

I think it is non-trivial, so let me detail how an electrum/light client would then use bc.tx.get_merkle_witness(txid):

it is assumed client already has a full raw tx, and wants to verify whether it has been mined
client also has full header chain, proof-of-work verified
client calls bc.tx.get_merkle_witness(txid)
client calculates wtxid of full tx it has, compares it with received wtxid
using cb_tx, cb_proof, block_height, block_hash
- client Satoshi-SPVs the received coinbase tx, against block header merkle_root
client extracts witness_commitment from cb_tx
1. if there is no witness_commitment,
  - client Satoshi-SPVs the txid, using merkle, block_height, block_hash, pos, against block header merkle_root
  - now client has an SPV guarantee that block only contains non-segwit txs, and tx in particular is also non-segwit (wtxid==txid) and it was mined in this block
2. if there is a witness_commitment,
  - client must do witness-SPV, even if tx seems non-segwit (as e.g. malicious electrum server could have stripped away the witness)
  - client does witness-SPV using wmerkle and pos, against witness_commitment
  - client must check that length of wmerkle matches length of cb_proof. This ensures there are the same number of levels in the wtxid merkle tree and in the txid merkle tree. (the two trees should have the same number of leaves, but this seems hard to check as a light client) This is intended as some protection against merkle-tree-construction weaknesses (cf. banning of 64 byte transactions and similar).
now client has an SPV guarantee that wtxid was mined in this block
- or if some check failed, then the client can connect to another server

in src/rpc/txoutproof.cpp:124 in 3abfd6d3d9

119             }
120             if (ntxFound != setTxids.size()) {
121                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Not all transactions found in specified or retrieved block");
122             }
123 
124+            if (!any_witness) prove_witness = false;

SomberNight commented at 0:50 am on July 1, 2025:

My cpp is beyond rusty, so maybe I misunderstand, but note that just because the tx to be proved does not have a witness, we would still want a witness-SPV proof for that tx.

The attack vector is that a malicious electrum server can notify the client about a tx touching their address and strip away the witness. That is, the actual tx has a witness, but the server sends it to the client using the old serialization format without it. Then when the client asks for the SPV proof, the server (still maliciously) would provide the old Satoshi-style SPV proof against the block header merkle root.

The only way the client can tell that this attack is happening is by Satoshi-SPV-ing the coinbase tx, extracting the witness_commitment from it, and then doing witness-SPV on the tx.

Basically whether to do witness-SPV or satoshi-SPV on a tx, the deciding factor is whether the coinbase contains a witness_commitment or not. If the client wants guarantees about the witness, it is immaterial whether the purported tx actually has a witness or not, the client must do witness-SPV if there is a witness_commitment. And as an extension of this logic, even though bitcoind knows for a fact whether the tx has a witness, to prove it to the client, it must provide the segwit-style witness-SPV proof.

Sjors commented at 5:37 pm on July 1, 2025: member

Concept ACK. @SomberNight it’s still useful if you open an issue to track the reason you need this, just in case this implementation PR doesn’t make.

in src/consensus/validation.h:164 in 605b372b1f outdated

160@@ -161,12 +161,12 @@ static inline int64_t GetTransactionInputWeight(const CTxIn& txin)
161 }
162 
163 /** Compute at which vout of the block's coinbase transaction the witness commitment occurs, or -1 if not found */
164-inline int GetWitnessCommitmentIndex(const CBlock& block)
165+inline int GetWitnessCommitmentIndex(const CTransaction& tx)

Sjors commented at 5:39 pm on July 1, 2025:

605b372b1f62c4cd59d8056f0a6a1baf17c24014 nit: maybe rename tx to coinbase

in src/consensus/validation.h:189 in 605b372b1f outdated

180@@ -181,4 +181,12 @@ inline int GetWitnessCommitmentIndex(const CBlock& block)
181     return commitpos;
182 }
183 
184+inline int GetWitnessCommitmentIndex(const CBlock& block)
185+{
186+    if (block.vtx.empty()) {
187+        return NO_WITNESS_COMMITMENT;
188+    }
189+    return GetWitnessCommitmentIndex(*block.vtx[0]);

Sjors commented at 5:41 pm on July 1, 2025:

605b372b1f62c4cd59d8056f0a6a1baf17c24014: *Assert(block)

luke-jr commented at 7:44 pm on July 7, 2025:

But block isn’t a pointer…?

in src/consensus/validation.h:186 in 605b372b1f outdated

180@@ -181,4 +181,12 @@ inline int GetWitnessCommitmentIndex(const CBlock& block)
181     return commitpos;
182 }
183 
184+inline int GetWitnessCommitmentIndex(const CBlock& block)
185+{
186+    if (block.vtx.empty()) {

Sjors commented at 5:43 pm on July 1, 2025:

605b372b1f62c4cd59d8056f0a6a1baf17c24014: to avoid confusion between an empty block and a pre-segwit block, could comment:

0// Block without coinbase

luke-jr commented at 7:45 pm on July 7, 2025:

This is just a move, so seems out of scope

in src/merkleblock.cpp:57 in 3abfd6d3d9

53@@ -49,6 +54,21 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std:
54             vMatch.push_back(false);
55         }
56         vHashes.push_back(hash);
57+        if (prove_witness && i) {

Sjors commented at 5:50 pm on July 1, 2025:

3abfd6d3d99bbcceb09162e83d8f9f9769bd4036:

0if (prove_witness) {
1  if (i == 0) {
2    // generation tx has null wtxid
3    wtxids.emplace_back();
4  } else {
5    wtxids.push_back(block.vtx[i]->GetWitnessHash());
6  }

in src/merkleblock.h:131 in 3abfd6d3d9

127@@ -128,6 +128,9 @@ class CMerkleBlock
128     /** Public only for unit testing */
129     CBlockHeader header;
130     CPartialMerkleTree txn;
131+    bool m_prove_gentx;

Sjors commented at 5:52 pm on July 1, 2025:

3abfd6d3d99bbcceb09162e83d8f9f9769bd4036: what does this do?

luke-jr commented at 7:47 pm on July 7, 2025:

An attempt to have a somewhat optional extension, but given the need to prove wtxid for non-witness txs, maybe not the best approach.

Sjors commented at 5:55 pm on July 1, 2025: member

Took an initial look at the code, but got a bit confused. Will check again later.

bigshiny90 commented at 1:40 pm on July 2, 2025: none

@SomberNight @luke-jr

I’m redoing this comment showing the proposed code changes, which are based on @SomberNight’s comments. I’m not sure IF these are all the fixes needed, just making the changes based on the comments so far. Let me know if anyone wants this branch pushed for the PR.

added a few references in the comments, showing the code/code pattern i used for the witness/nonce/commitment code.

Changes:

1. Fixed witness commitment verification in `src/rpc/txoutproof.cpp`

 0// In verifytxoutproof RPC, around line 220
 1const auto wtxids = [&]() -> std::unordered_map<unsigned int, uint256> {
 2    if (!merkleBlock.m_gentx) return {};
 3    if (merkleBlock.m_gentx->GetHash() != vMatch[0]) return {};
 4    const int witness_commit_outidx = GetWitnessCommitmentIndex(*merkleBlock.m_gentx);
 5    if (witness_commit_outidx == NO_WITNESS_COMMITMENT) return {};
 6    
 7    std::vector<uint256> wtxid_matches;
 8    std::vector<unsigned int> wtxid_indexes;
 9    const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
10    
11    // ref. validation.cpp, CheckWitnessMalleation
12    const auto& witness_stack{merkleBlock.m_gentx->vin[0].scriptWitness.stack};
13    // Check it's valid (1 element, 32 bytes)
14    if (witness_stack.size() != 1 || witness_stack[0].size() != 32) {
15       return {}; // invalid
16    }
17
18    uint256 hash_witness = wtxid_root;
19    CHash256().Write(hash_witness).Write(witness_stack[0]).Finalize(hash_witness);
20
21    if (memcmp(hash_witness.begin(), &merkleBlock.m_gentx->vout[witness_commit_outidx].scriptPubKey[6], 32)) {
22       // i left the output to be the full scriptPubKey here
23       LogPrintf("%s vs %s DEBUG", hash_witness.GetHex(), HexStr(MakeUCharSpan(merkleBlock.m_gentx->vout[witness_commit_outidx].scriptPubKey)));
24       return {};
25    }
26    
27    // ... rest of function

2. prove_witness should be true if it has a witness commitment in `gettxoutproof`

Force witness proofs for blocks that have witness commitments:

0// Around line 125
1// Security: If the block has a witness commitment, set prove_witness to true
2// (ref. validation.cpp, CheckWitnessMalleation)
3if (!prove_witness && GetWitnessCommitmentIndex(block) != NO_WITNESS_COMMITMENT) {
4    prove_witness = true;
5}

3. Updated test in `test/functional/rpc_txoutproof.py`

Handle the new return object format when verify_witness=True:

0# When verify_witness=True, returns object instead of array
1witness_result = self.nodes[0].verifytxoutproof(self.nodes[0].gettxoutproof([txid1, txid2], prove_witness=True), verify_witness=True)
2assert_equal(list(witness_result['txids'].keys()), txlist)

Key fix details:

The witness commitment code matches the pattern used in validation.cpp’s CheckWitnessMalleation().

rpc_txoutproof.py test passes successfully with these changes.

bigshiny90 commented at 1:55 pm on July 2, 2025: none

Changes are in my branch: https://github.com/bigshiny90/bitcoin/tree/fix-pr-32844-witness

RPC/txoutproof: Support including (and verifying) proofs of wtxid #32844