This PR:
PushWarnings in RPC callsimportdescriptors and by a new warnings field on getdescriptorinfo (which also exposes the existing miniscript older() warnings)Cloning internal to musig() is not warned: both musig(A,B)/<0;1>/* and musig(A/<1;2>,B) derive a distinct aggregate key per branch, so nothing observable is reused.
closes #35985
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fametrano |
| Stale ACK | jeanpablojp |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
wallet_importdescriptors to separate run_test() contents into specific functions by nebula-21)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/32101345101/job/95602400388</sub>
<sub>LLM reason (✨ experimental): CI failed due to a GCC 12 internal compiler error (segmentation fault) while compiling src/rpc/blockchain.cpp.</sub>
Edit: Looks like the ICE was due to a corrupt VM/hardware
tACK 13ef865f84fc5d0f7073e11a6b4167b2dcd45e92
Built and ran the unit tests plus rpc_getdescriptorinfo.py and wallet_importdescriptors.py. I dropped each of the four multipath_cloned = true lines one at a time and each one fails on a different descriptor in the new unit test, and reverting only the PushWarnings dedup fails both functional tests.
1353 | + auto descs = Parse(desc, keys, error); 1354 | + BOOST_REQUIRE_MESSAGE(!descs.empty(), desc + ": " + error); 1355 | + for (const auto& d : descs) { 1356 | + const auto warnings = d->Warnings(); 1357 | + if (expect_warning) { 1358 | + BOOST_REQUIRE_MESSAGE(warnings.size() == 1U, desc);
nit: this only allows one warning. I hit both with wsh(and_v(v:pk(A/<0;1>/*),and_v(v:pk(B/0/*),older(65536)))), which nothing covers at the moment. Worth adding one?
Ping @pythcoiner this has been in need of a rebase for a while. Are you still working on this change?
@sedited, this is still on my todo-list (but not at the top atm tbh)
rebased on master @jeanpablojp thanks for the review, added your descriptor to the test, and modified the check to count only expected warnings
tACK 93673215444ab07b64c39213c5eceb3bb979652e
I compared the new warning with key reuse computed outside Core. bip32-descriptors, a library I maintain, expanded 291 multipath descriptors. They cover multi, sortedmulti, multi_a, tr() trees, miniscript and musig(), with ranged, fixed and plain single-path keys. For each expansion it collected the keys in its scripts plus the taproot internal key, at indexes 0 to 2 where the descriptor is ranged. In 272 descriptors two expansions share a key. The warning fires on 270 of them and on none of the other 19. The two it misses clone no single-path key: each shared key comes from two multipath expressions on the same xpub. deriveaddresses matches the library's addresses on all 291. importdescriptors warns exactly where getdescriptorinfo does. With the tr() internal-key flag removed, 17 more descriptors disagree, so the comparison catches a missing flag.