Includes:
getaddressinfo report tr() descriptors as solvable (so that inferred descriptors are shown), despite not having signing code for them.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
611+ return std::nullopt;
612+ } else if (node.sub[0]->done && !node.sub[1]->done && !node.sub[1]->leaf && !node.sub[1]->sub[0] && !node.sub[1]->hash.IsNull() &&
613+ (CHashWriter{HASHER_TAPBRANCH} << node.sub[1]->hash << node.sub[1]->hash).GetSHA256() == node.hash) {
614+ // Special case to deal with duplicate subtrees: after exploring the left subtree (which holds actual data),
615+ // iterate through it again, but mark the right (unexplored, but implicitly identical) subtree as finished,
616+ // so that we don't end up in an infinite loop.
In 77466ae5c58b4a46e545d3d24521fa635f7b8fbe “Taproot descriptor inference”
It would be useful to explain in more detail why these conditions matter.
IIUC, this happens because in the section above, if we have duplicate subtrees, we end up making the same side of the tree when we have duplicates which means that the normal tree handling part of this loop runs out of nodes to look at on the other side of the branch.
However it is still not clear to me why !node.sub[1]->sub[0] is necessary.
Restructured things a bit by adding explicit “explored” and “inner” flags to the nodes. This avoids the need for testing the specifics of the unexplored child.
Also expanded the comment to explain this better.
Reviewed that the consensus commit (e3739fb3b042c3805b5b6357f6f79df787d11144) is a pure refactor.
Code review ACK 13c6ab699eb6906c6554b4319a377c33fa0d157f
1231 return key_provider;
1232 }
1233
1234+std::unique_ptr<PubkeyProvider> InferXOnlyPubkey(const XOnlyPubKey& xkey, ParseScriptContext ctx, const SigningProvider& provider)
1235+{
1236+ unsigned char full_key[33] = {0x02};
0x02 (and 0x03 below) and 33.
1218@@ -1217,44 +1219,68 @@ std::unique_ptr<DescriptorImpl> ParseScript(uint32_t& key_exp_index, Span<const
1219 return nullptr;
1220 }
1221
1222-std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider)
1223+std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider, bool xonly = false)
xonly argument to InferPubkey is never passed?
1870+}
1871+
1872+static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, const std::vector<unsigned char>& program, const uint256& tapleaf_hash)
1873+{
1874+ //! The internal pubkey (x-only, so no Y coordinate parity).
1875+ const XOnlyPubKey p{uint256(std::vector<unsigned char>(control.begin() + 1, control.begin() + TAPROOT_CONTROL_BASE_SIZE))};
control.size() before indexing into it (maybe same for program)
I think you lost the asserts in your rebase:
0assert(control.size() >= TAPROOT_CONTROL_BASE_SIZE);
1assert(program.size() >= uint256::size());
Addressed some comments, and also made this observable change:
0 if (permit_uncompressed || key.IsCompressed()) {
1 CPubKey pubkey = key.GetPubKey();
2 out.keys.emplace(pubkey.GetID(), key);
3- return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey);
4+ return std::make_unique<ConstPubkeyProvider>(key_exp_index, pubkey, ctx == ParseScriptContext::P2TR);
5 } else {
Which will make descriptors with a provided private key inside tr() be interpreted as the equivalent xonly pubkey rather than the compressed or uncompressed one.
It’s too hot here to review code
Same, I propose we postpone the feature freeze a bit. Not able to concentrate at the moment.
214+{
215+ /** The BIP341 internal key. */
216+ XOnlyPubKey internal_key;
217+ /** The Merkle root of the script tree (0 if no scripts). */
218+ uint256 merkle_root;
219+ /** Map from (script, leaf_version) to (sets of) control blocks. */
495 return *this;
496 }
497
498 WitnessV1Taproot TaprootBuilder::GetOutput() { return WitnessV1Taproot{m_output_key}; }
499+
500+TaprootSpendData TaprootBuilder::GetSpendData() const
IsComplete()?
1324+ std::unique_ptr<DescriptorImpl> subdesc;
1325+ if (leaf_ver == TAPROOT_LEAF_TAPSCRIPT) {
1326+ subdesc = InferScript(script, ParseScriptContext::P2TR, provider);
1327+ }
1328+ if (!subdesc) {
1329+ ok = false;
raw() (at least for leaf_ver == TAPROOT_LEAF_TAPSCRIPT)? That can wait for a followup though, since we can’t have tr() descriptors with raw leaves in the wallet anyway atm, and raw() is still limited to the top level.
tACK 8349355
However I glanced over InferTaprootTree. I think it would help to include tests for the various problems that function is checking for.
InferTaprootTree.
I think something went wrong in your rebase.
For example in the original https://github.com/bitcoin/bitcoin/commit/834935511c2c799c8cb2312d617dbed6336000c4 you changed the argument bool xonly = false to bool xonly in the ConstPubkeyProvider provider.
re-ACK d637a9b397816e34652d0c4d383308e39770737a
Rebase looks good.
re-utACK d637a9b
Much better
528+ if (!tweak || tweak->first != output) return std::nullopt;
529+ // If the Merkle root is 0, the tree is empty, and we're done.
530+ std::vector<std::tuple<int, CScript, int>> ret;
531+ if (spenddata.merkle_root.IsNull()) return ret;
532+
533+ /** Data structure to represent the nodes of the tree we're going to be build. */
to build
530+ std::vector<std::tuple<int, CScript, int>> ret;
531+ if (spenddata.merkle_root.IsNull()) return ret;
532+
533+ /** Data structure to represent the nodes of the tree we're going to be build. */
534+ struct TreeNode {
535+ /** Hash of this none, if known; 0 otherwise. */
node
545+ bool inner;
546+ /** Whether or not we have produced output for this subtree. */
547+ bool done = false;
548+ };
549+
550+ // Build tree from the provides branches.
provided
1846@@ -1847,16 +1847,14 @@ static bool ExecuteWitnessScript(const Span<const valtype>& stack_span, const CS
1847 return true;
1848 }
1849
1850-static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, const std::vector<unsigned char>& program, const CScript& script, uint256& tapleaf_hash)
1851+uint256 ComputeTapleafHash(uint8_t leaf_version, const CScript& script)
CScript here. While it is true that BIP-341 names this stack item “script”, this value is only interpreted as a CScript in the case of BIP-342 where the leaf_version is equal to TAPROOT_LEAF_TAPSCRIPT (modulo TAPROOT_LEAF_MASK). Otherwise it is simply a bytestring.
Labels
Wallet
RPC/REST/ZMQ
Consensus
Descriptors
Milestone
22.0