#26201 removed taproot from getdeploymentinfo (not yet in a release, slated for v32), which it turns out Lnd relies on: https://github.com/lightningnetwork/lnd/pull/10683
Expand the release doc note to recommend the same solution they used: check for TAPROOT in the script_flags array. This was added in v31.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35183.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, sedited |
| Stale ACK | ferminquant |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
We only added the script_flags array in v31.0, so it may be a bit annoying for RPC clients to have to use getdeploymentinfo for v30 and older. Since #26201 isn't in a release yet, we could temporarily put "taproot" back under deployments with a deprecation notice. Perhaps that's useful if Lnd is not the only client with this issue (since they've already dealt with it).
script_flags was indeed added in 4577fb2b1e098c3f560b1ff50a37ebfef2af5f32 v31
However, I don't understand why any client would need to check for taproot. Our own wallet and mempool assumes that it is active since genesis, so what is different about lightning wallets that they'd assume otherwise?
ACK 3a3623a111a6b66faeab6d6544ff860e1434e31f
Reviewed the release note change and checked the linked context. script_flags was added in v31, #26201 is targeted for v32, and the linked LND change uses script_flags as the compatibility path for the removed deployments.taproot field. Also ran git diff --check and the markdown/trailing/doc lints.
So I tend toward Approach NACK, per my previous comment. It should say that the deployment is buried and can be assumed to be active on main.
0 | @@ -1,4 +1,8 @@ 1 | RPC 2 | --- 3 | 4 | -- 'taproot' has been removed from 'getdeploymentinfo' because its historical activation height is no longer used anywhere in the codebase. (#26201) 5 | +- 'taproot' has been removed from 'getdeploymentinfo' because its historical 6 | + activation height is no longer used anywhere in the codebase. 7 | + Applications that rely on `deployments.taproot` to detect Taproot support 8 | + should instead check for `TAPROOT` in the `script_flags` array returned by
should either assume Taproot to be always active (since Bitcoin Core v24.0), or check for `TAPROOT` in the `script_flags` array returned by
Took this suggestion.
I don't think docs can "force" anyone. If they did, then you are "forcing" them to use the script_flags array.
5 | +- 'taproot' has been removed from 'getdeploymentinfo' because its historical 6 | + activation height is no longer used anywhere in the codebase. 7 | + Applications that rely on `deployments.taproot` to detect Taproot support 8 | + should either assume Taproot to be always active (since Bitcoin Core v24.0), 9 | + or check for `TAPROOT` in the `script_flags` array returned by 10 | + `getdeploymentinfo`. This is available since Bitcoin Core v31. (#26201)
`getdeploymentinfo` (since Bitcoin Core v31.0). (#26201)
nit: Either replace This with The array, or inline the condition right after?
Thanks, that's indeed more clear now that there's two suggestions.
production-ready enterprise-grade lgtm
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
production-ready enterprise-grade lgtm
Should be good for Microsoft certification now.
Stakeholder-aligned SLA-compliant production-ready enterprise-grade lgtm ACK f24a7b5f759920f62db1a131a9a9d31408edd0b2
ACK f24a7b5f759920f62db1a131a9a9d31408edd0b2
Milestone
32.0