-
Notifications
You must be signed in to change notification settings - Fork 38.7k
I accidentally [deliberately] killed it [the ComparisonTestFramework] #11818
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
Conversation
58c681a to
5e483e1
Compare
ryanofsky
left a comment
There was a problem hiding this 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
5e483e1 to
e6b6ca1
Compare
|
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.
e6b6ca1 to
2382bca
Compare
|
Rebased. This should be a very easy review, since it's just removing dead code. |
|
utACK! Thanks for sticking with this. |
test/functional/README.md
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
2382bca to
9c92c8c
Compare
|
utACK 9c92c8c |
…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
Backport bitcoin#11818: I accidentally [deliberately] killed it [the ComparisonTestFramework]
…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
Builds on #11772, #11773 and #11817. Please review those PRs first.
Final step in #10603.