Skip to content

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 26, 2020

This is dead code post-Segwit.

@promag
Copy link
Contributor

promag commented Jul 26, 2020

ACK

@maflcko
Copy link
Member

maflcko commented Oct 19, 2020

Concept ACK on removing unused code

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 10, 2021

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

Conflicts

No conflicts as of last run.

@jnewbery
Copy link
Contributor

This is based on a 2018 base commit. Needs to be rebased to pick up all of the new tests/changes since then.

@jnewbery
Copy link
Contributor

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?

@fanquake
Copy link
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 (8a9ffec).

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".

@luke-jr
Copy link
Member Author

luke-jr commented Jul 7, 2021

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 luke-jr force-pushed the gbt_rm_versionforce branch from 12068e4 to 7512bfe Compare July 7, 2021 00:46
@fanquake
Copy link
Member

fanquake commented Sep 2, 2021

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.

@maflcko
Copy link
Member

maflcko commented Sep 2, 2021

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.

@glozow
Copy link
Member

glozow commented Mar 8, 2022

Concept ACK

@fanquake fanquake added this to the 24.0 milestone Mar 11, 2022
@fanquake
Copy link
Member

Added to the 24.0 milestone.

@fanquake
Copy link
Member

fanquake commented Aug 9, 2022

@luke-jr do you want to rebase this, otherwise I can drop it back off the milestone.

@luke-jr luke-jr force-pushed the gbt_rm_versionforce branch from 7512bfe to 90a5dfa Compare August 9, 2022 22:15
@luke-jr
Copy link
Member Author

luke-jr commented Aug 9, 2022

#25153 should really have been reverted, but rebased anyway.

@achow101
Copy link
Member

ACK 90a5dfa

@fanquake fanquake merged commit 95d4744 into bitcoin:master Aug 17, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2022
…lity code

90a5dfa RPC/Mining: Clean out pre-Segwit miner compatibility code (Luke Dashjr)

Pull request description:

  This is dead code post-Segwit.

ACKs for top commit:
  achow101:
    ACK 90a5dfa

Tree-SHA512: 5970aa3548d2a7da7c6e83fb9b910529faab10251b115122cec833bb7d3a54c7cb0714c1a873807be04c7817bb827c7ece1e20e8fa4c907aa58688487d0ec44d
@bitcoin bitcoin locked and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants