RPC/Mining: Clean out pre-Segwit miner compatibility code #19391

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:gbt_rm_versionforce changing 1 files +0 −16

luke-jr commented at 9:24 PM on June 26, 2020: member

This is dead code post-Segwit.
DrahtBot added the label Mining on Jun 26, 2020
DrahtBot added the label RPC/REST/ZMQ on Jun 26, 2020
promag commented at 10:54 PM on July 26, 2020: member

ACK
MarcoFalke commented at 9:39 AM on October 19, 2020: member

Concept ACK on removing unused code
DrahtBot cross-referenced this on Mar 10, 2021 from issue Genericide BIP9 in variable/type names and comments by luke-jr
DrahtBot commented at 11:08 AM on March 10, 2021: contributor



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



Conflicts

No conflicts as of last run.
jnewbery commented at 12:16 PM on March 10, 2021: member

This is based on a 2018 base commit. Needs to be rebased to pick up all of the new tests/changes since then.
jnewbery commented at 12:49 PM on March 10, 2021: member

Code looks safe, but should be rebased on a recent base commit to ensure all new tests/checks run successfully.

Should https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes be updated to say that maxversion no longer has meaning?
luke-jr closed this on Mar 10, 2021
luke-jr reopened this on Mar 10, 2021
DrahtBot cross-referenced this on Mar 22, 2021 from issue Implement BIP8 lockinontimeout by achow101
fanquake added the label Waiting for author on Apr 12, 2021
fanquake commented at 9:02 AM on April 12, 2021: member

but should be rebased on a recent base commit to ensure all new tests/checks run successfully.

Agree. This branch is currently based off (8a9ffec0a257da659ba54c5073bfc820986ae9c1).

Should https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes be updated to say that maxversion no longer has meaning?

This also still needs a response.

Added "Waiting for author".
DrahtBot cross-referenced this on Apr 14, 2021 from issue rpc: Add RPCContext by promag
DrahtBot cross-referenced this on Apr 14, 2021 from issue [Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl
DrahtBot added the label Needs rebase on Apr 17, 2021
MarcoFalke removed the label Waiting for author on Apr 17, 2021
luke-jr commented at 12:46 AM on July 7, 2021: member

Rebased due to conflicts, but note

Code looks safe, but should be rebased on a recent base commit to ensure all new tests/checks run successfully.

That's not a reason to rebase. That's a flaw in QA infra. It should be merging to master before running tests/checks.
luke-jr force-pushed on Jul 7, 2021
DrahtBot removed the label Needs rebase on Jul 7, 2021
fanquake commented at 4:39 AM on September 2, 2021: member

Concept ACK

That's not a reason to rebase. That's a flaw in QA infra.

There's a myriad of reasons to keep development branches rebased up and/or up to date.

If I'm a dev wanting to just simply checkout and test this code, i.e git fetch origin pull/19391/head:19391 && git checkout 19391, not only should I not have to worry about merging it into master myself, but, having a commit here based on a branch from years ago means it's going to bust up my caches when compiling, have missing tests, lack support for fuzzers & other tooling that's been added in the interim, depends will be outdated, so if I'm building against that I'll be testing against old / potentially broken dependencies. I also shouldn't need to replicate the QA infra locally to test things fairly extensively.
MarcoFalke commented at 6:49 AM on September 2, 2021: member

going to bust up my caches

Personally I rebase/merge every change I test on master (even if it is just a single commit back) to rule out any unintended cache mishaps and at the same time check for silent merge conflict.

If there is no reason to rebase, I think the approach @luke-jr is taking seems fine.
luke-jr cross-referenced this on Mar 7, 2022 from issue miner: always assume we can build witness blocks by glozow
glozow commented at 1:57 PM on March 8, 2022: member

Concept ACK
DrahtBot cross-referenced this on Mar 11, 2022 from issue CChainState -> Chainstate by jamesob
fanquake added this to the milestone 24.0 on Mar 11, 2022
fanquake commented at 2:51 PM on March 11, 2022: member

Added to the 24.0 milestone.
DrahtBot cross-referenced this on May 19, 2022 from issue scripted-diff: Use getInt<T> over get_int/get_int64 by MarcoFalke
DrahtBot added the label Needs rebase on May 19, 2022
fanquake commented at 8:59 AM on August 9, 2022: member

@luke-jr do you want to rebase this, otherwise I can drop it back off the milestone.
RPC/Mining: Clean out pre-Segwit miner compatibility code 90a5dfa509
luke-jr force-pushed on Aug 9, 2022
luke-jr commented at 10:16 PM on August 9, 2022: member

#25153 should really have been reverted, but rebased anyway.
DrahtBot removed the label Needs rebase on Aug 9, 2022
achow101 commented at 3:34 PM on August 15, 2022: member

ACK 90a5dfa5098f142ba8b7b2f20eac31f4095ca583
fanquake merged this on Aug 17, 2022
fanquake closed this on Aug 17, 2022
sidhujag referenced this in commit 3c9d789e34 on Aug 17, 2022
bitcoin locked this on Aug 17, 2023