Skip to content

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Dec 2, 2017

Builds on #11772, #11773 and #11817. Please review those PRs first.

Final step in #10603.

  • First commit removes bip9-softforks.py. bip9-sofforks.py was intended to be a generic test for versionbits deployments. However, it only tests CSV activation and was not updated to test segwit activation. CSV activation is tested by bip68-112-113-p2p.py, so this test is duplicated effort. Rather than try to update it to use the BitcoinTestFramework, just remove it. (see Test of BIP9 fork activation of mtp, csv, sequence_lock btcdrak/bitcoin#8 for previous discussion around the redundancy of bip9-softforks.py)
  • Second commit removes the now unused BitcoinComparisonFramework class and the comptool and blockstore modules.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed just the two removal commits, assuming all the other commits will go away when this is rebased.

  • utACK 5e483e1eae34083ebfe7bc89d21644fa2c8f4389 [tests] Remove Comparison Test Framework
  • utACK 8be729761f794d92b5bec2262ebee9fea35e518a [tests] Remove bip9-softforks.py

@jnewbery jnewbery force-pushed the remove_comp_framework branch from 5e483e1 to e6b6ca1 Compare March 29, 2018 15:50
@jnewbery
Copy link
Contributor Author

Rebased to remove merged commits.

bip9-sofforks.py was intended to be a generic test for versionbits
deployments. However, it only tests CSV activation and was not updated
to test segwit activation. CSV activation is tested by
bip68-112-113-p2p.py, so this test is duplicated effort.

Rather than try to update it to use the BitcoinTestFramework, just
remove it.
@jnewbery jnewbery force-pushed the remove_comp_framework branch from e6b6ca1 to 2382bca Compare April 2, 2018 17:46
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 2, 2018

Rebased. This should be a very easy review, since it's just removing dead code.

@sdaftuar
Copy link
Member

sdaftuar commented Apr 2, 2018

utACK! Thanks for sticking with this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is this line supposed to go away?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot! Added that back, and removed the documentation for test_framework/blockstore.py

@jnewbery jnewbery force-pushed the remove_comp_framework branch from 2382bca to 9c92c8c Compare April 2, 2018 18:04
@maflcko
Copy link
Member

maflcko commented Apr 2, 2018

utACK 9c92c8c

@maflcko maflcko merged commit 9c92c8c into bitcoin:master Apr 5, 2018
maflcko pushed a commit that referenced this pull request Apr 5, 2018
…TestFramework]

9c92c8c [tests] Remove Comparison Test Framework (John Newbery)
e80c640 [tests] Remove bip9-softforks.py (John Newbery)

Pull request description:

  Builds on #11772, #11773 and #11817. Please review those PRs first.

  Final step in #10603.

  - First commit removes bip9-softforks.py.  bip9-sofforks.py was intended to be a generic test for versionbits deployments. However, it only tests CSV activation and was not updated to test segwit activation. CSV activation is tested by bip68-112-113-p2p.py, so this test is duplicated effort. Rather than try to update it to use the BitcoinTestFramework, just remove it. (see btcdrak#8 for previous discussion around the redundancy of bip9-softforks.py)
  - Second commit removes the now unused BitcoinComparisonFramework class and the comptool and blockstore modules.

Tree-SHA512: 4bb7196d521048b3b8ba95c87dde73005a1ac73d29ccbb869f11ce9a71089686e7eacd7335337853041dfbd3a5b110172b105adbada58779814d4db22b1376f5
@jnewbery jnewbery deleted the remove_comp_framework branch April 5, 2018 14:23
codablock added a commit to dashpay/dash that referenced this pull request Jan 14, 2020
Backport bitcoin#11818: I accidentally [deliberately] killed it [the ComparisonTestFramework]
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request May 17, 2021
…son test framework

48e5d54 [Tests][Refactoring] Remove 'magic bytes' in p2p_invalid_* tests (random-zebra)
fcda6cc [QA][Cleanup] Retire the Comparison Test Framework (random-zebra)
3633e42 [Tests] Fix and resurrect feature_block.py (random-zebra)
77645bf [BUG][Tests] Add on_getblocks to P2PDataStore interface (random-zebra)
030517b [Refactoring] pass next block height to IncrementExtraNonce (random-zebra)
0f0c97c [BUG] Fix block with invalid PoW in zerocoin_rejection_tests (random-zebra)
b19bee9 [Refactoring][BUG] Never change chain-params after setup in unit tests (random-zebra)
810a880 [Tests] Update PIVX specific constants bits/block-size/block-version (random-zebra)
d0777a7 [Refactoring] Add fPowNoRetargeting consensus param + fix regtest diff (random-zebra)
93fcc2e [Params] Fix regtest coinbase - mine it with lower diff (random-zebra)
4cf7222 [Tests] Refactor wallet_sapling_transactions_validations_tests (random-zebra)
44a1c44 [Refactor] Option to mine mempool txes in TestChainSetup::CreateBlock (random-zebra)
0b670c8 [BUG] Properly recompute coinbase and sapling root hash in CreateBlock (random-zebra)
f5f72ca [Trivial] Update some state logs (tx oversize and inputs missing/spent) (random-zebra)
9d552f9 [Refactor][Tests] Do not change chain when not needed (random-zebra)
bd6e591 [Refactoring] remove SaplingActive check in GUI/RPC (random-zebra)
b2410ae [BUG] Fix assertion in error in assert_debug_log (random-zebra)

Pull request description:

  Follow-up to #2346.

  The original goal here was to remove the `ComparisonTestFramework`, following bitcoin#11818.

  But, before doing that, had to update the old `feature_block` test, which was still using the framework, even though the test was disabled.
  This is one of the most important functional tests, as it checks all block-related consensus rules, so, just keeping it disabled was not an option.

  Aside from being very long (1200+ lines), the test was taking ages to mine any single block.
  This is because it relies on `next_block` doing the actual proof of work (`CBlock::solve`), given the regtest nBits (difficulty is fixed on regtest).

  Now, since all networks (mainnet/testnet/regtest) share the same genesis block, the regtest difficulty was way higher than it should have been. We never had issues with this, because we are bailing out early in `CheckProofOfWork`, essentially not performing any proof of work check on regtest.

  So, in order to fix `feature_block.py`, there were two options: either modify `next_block` to keep a fixed nonce, or update the regtest chain parameters, creating a new genesis block with low-diff nBits, and actually checking the proof of work.
  I chose the latter.

  This triggered the need to refactor some unit-tests that were changing chain params (from `MAIN` to `REGTEST`) without reloading the genesis block in the block index (which wasn't needed before, because, as explained, all networks had the same genesis).

  After this PR, the chain is set only inside the test fixtures, and is never changed inside the single test cases (so there is no need to reload the chain index during execution).

ACKs for top commit:
  furszy:
    ACK 48e5d54 .
  Fuzzbawls:
    ACK 48e5d54

Tree-SHA512: 213cad1be7eade941ec5be59fc0b8753eeb11aef55ae74052f59b6c668c4b8ab87efe77221545bdbb2d7b5beeb34b6123005ed2964def6673080cc809c508145
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants