Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Deprecate rpcserialversion=0 #28448

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Sep 11, 2023

This option was introduced in #9194 to ease the transition to segwit; now that most libraries and apps have been updated it should no longer be necessary.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 11, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, Randy808, glozow
Concept ACK instagibbs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28438 (Use serialization parameters for CTransaction by ajtowns)
  • #27433 (getblocktemplate improvements for segwit and sigops by Sjors)

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.

@ajtowns
Copy link
Contributor Author

ajtowns commented Sep 11, 2023

If people do still need this behaviour in the future, a couple of possible solutions:

  • run an old version of bitcoind (ie, anything between 0.13.2 and 26.x) behind a current bitcoind, and use its RPC interface
  • use an intermediate tool to strip the witness data; a patch to bitcoin-util to support stripping witness data from a hex-encoded tx or block is at https://github.com/ajtowns/bitcoin/commits/202309-util-stripwit

Context for this PR is #28438.

@ajtowns ajtowns added this to the 26.0 milestone Sep 11, 2023
@maflcko
Copy link
Member

maflcko commented Sep 11, 2023

For reference: https://github.com/search?q=rpcserialversion%3D0&type=code

Also, I am not sure if this is needed in the context of #28438 (comment). It may be better to consider this unrelated from purely internal refactoring going on?

@ajtowns
Copy link
Contributor Author

ajtowns commented Sep 11, 2023

For reference: https://github.com/search?q=rpcserialversion%3D0&type=code

This seems to consist of:

  • reference to feature_segwit.py in forks of bitcoin
  • python-bitcoinlib release notes entry from prior to 0.9.0 in Dec 2017
  • opentimestamps-server README prior to April 2018
  • repos scraping github PRs (including this PR itself)
  • blockchain analysis tools that haven't been updated in years

Also some altcoin/layered coin things:

  • stacks/stx - https://stacks101.com/stx-mining/setup/bitcoin-node/ (segwit donation address? check! segwit support in the software? uhh...)
  • "lightning cash" altcoin (this seems to be a fork of bitcoin core 0.15 and uses other features that were deprecatedrpc at that time, so code changes here shouldn't affect it)
  • omnicore (I think this is similar, but has merged up to version 0.20)

@maflcko
Copy link
Member

maflcko commented Sep 12, 2023

review ACK 971bae9

Again, I don't think this is needed or required for #28438, but if there is no use case and user left, it may be best to remove. If there is a major use case or user left, it can be un-deprecated in the future, or an alternative way to achieve the same can be added.

@Randy808
Copy link
Contributor

Randy808 commented Sep 12, 2023

Code Review ACK 971bae9

@glozow
Copy link
Member

glozow commented Sep 12, 2023

ACK 971bae9, seems appropriate to remove. Thanks for looking at usage in #28448 (comment)

@instagibbs
Copy link
Member

concept ACK

@fanquake fanquake merged commit 578f50f into bitcoin:master Sep 12, 2023
15 checks passed
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants