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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32844.
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.
Reviewers, this pull request conflicts with the following ones:
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.
Possible typos and grammar issues:
No other typos were found.
drahtbot_id_4_m
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 {};
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
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:
wtxidblock_heightblock_hashpos (index of tx in block)merkle or wmerkle
merkle is the Satoshi-SPV proof, in case there is no witness commitment in the blockwmerkle is the witness-SPV proof otherwisecb_tx, the generation/coinbase txcb_proof, the Satoshi-SPV proof for the generation txObviously 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):
tx, and wants to verify whether it has been minedbc.tx.get_merkle_witness(txid)wtxidcb_tx, cb_proof, block_height, block_hash
cb_tx
merkle, block_height, block_hash, pos, against block header merkle_roottx in particular is also non-segwit (wtxid==txid) and it was mined in this blocktx seems non-segwit (as e.g. malicious electrum server could have stripped away the witness)wmerkle and pos, against witness_commitmentwmerkle 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).wtxid was mined in this block
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;
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.
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)
tx to coinbase
gentx (the coinbase is the input only)
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]);
*Assert(block)
block isn’t a pointer…?
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()) {
605b372b1f62c4cd59d8056f0a6a1baf17c24014: to avoid confusion between an empty block and a pre-segwit block, could comment:
0// Block without coinbase
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) {
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 }
127@@ -128,6 +128,9 @@ class CMerkleBlock
128 /** Public only for unit testing */
129 CBlockHeader header;
130 CPartialMerkleTree txn;
131+ bool m_prove_gentx;
Sorry, mixed that up with have_witness_proof.
This is to explicitly specify that the proof is proving the generation tx itself, not just using it to prove the witness root.