bitcoin / bitcoin

Bitcoin Core integration/staging tree
https://bitcoincore.org/en/download
MIT License
79.74k stars 36.45k forks source link

docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md #30279

Open benma opened 5 months ago

benma commented 5 months ago

Bitcoin Core uses BIP-386 for tr(...) descriptors, and in the case of tr(KEY), the produced key is tweaked according to BIP-341:

https://github.com/bitcoin/bips/blob/85cda4e225b4d5fd7aff403f69d827f23f6afbbc/bip-0341.mediawiki?plain=1#L156

In the current descriptors.md, it is documented that tr(KEY) uses KEY as the internal key, with no further comment:

https://github.com/bitcoin/bitcoin/blob/a7bc9b76e73f04dfe4d6ba42033fe38659090e8b/doc/descriptors.md?plain=1#L82

By looking only at descriptors.md without knowledge or reference to BIP-386, one can interpret tr(KEY) to use KEY directly without tweaking.

Ideally the docs should be expanded to reference BIP-386.

achow101 commented 5 months ago

I thought the phrase "internal key" is unambiguous to mean that the key will need tweaking, as that is how BIP 341 defines that phrase.

It might make sense to remove descriptors.md since it's ostensibly superseded by the BIPs too.

benma commented 5 months ago

I for one was confused. Imo being explicit is better than being implicit. "internal key" is ambiguous to me, as it means it is untweaked, but it does not mean it must be tweaked. BIP-341 merely says "should".

Removing the descriptors.md in in favor of BIPs is not optimal, because:

I think the best would be to complete the docs and link to the relevant BIPs. In this particular issue, linking to BIP-386 for tr(...) descriptors and mentioning that tr(KEY) uses the tweak described in BIP-341 would make the docs here much more useful.

Other parts are also not up to date, like the support for miniscript expressions for SCRIPT. There it would also be great to expand the docs and link to https://bitcoin.sipa.be/miniscript/ (afaik there is no BIP for that).

pythcoiner commented 5 months ago

There it would also be great to expand the docs and link to https://bitcoin.sipa.be/miniscript/ (afaik there is no BIP for that).

There is re recent PR in BIP repo.

sipa commented 5 months ago

I agree with @benma. I think we should make the descriptors.md file more explicit, but also link to the relevant BIPs now.