The flag is problematic for many reasons:
Fix all issues by removing it.
If there is a use-case, likely a per-RPC flag can be added, if needed.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ajtowns, TheCharlatan |
| Concept ACK | fanquake, kristapsk |
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.
It is deprecated
It’s only been deprecated in master for a couple of months; better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they’re still relying on this functionality?
Otherwise, changes here look like what I’d expect.
If there is a use-case, likely a per-RPC flag can be added, if needed.
Could give bitcoin-util the capability to strip witness data from a block/tx and leave it up to users who need it, rather than making the RPC API more complex for what’s presumably a very rare use case.
better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they’re still relying on this functionality?
Happy to wait, but there shouldn’t be any merge conflicts, so it should be trivial to just type git revert cleanly in the rare case that the deprecation will be undone. If the deprecation isn’t undone, I don’t see a need to delay for another release, because anyone affected can stay at 26.0, for as long as they need to change their code.
The deprecation was covered in https://bitcoinops.org/en/newsletters/2023/09/20/ and 26.0 was released a few weeks ago. Unless anyone heard someone complain, this seems good to move forward now?
Given we’re doing a short cycle, probably should either merge this soon for 27.0 or defer it to 28.0 and merge it shortly after branch off. No opinion either way on that personally.
crACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
lgtm ACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
If there is a use-case, likely a per-RPC flag can be added, if needed.
