[Tests] Resurrect feature_block and kill the p2p comparison test framework #2360
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You deserve some beers! 🍻 🍻 |
1 task
random-zebra
force-pushed
the
202104_kill-comparison-framework
branch
3 times, most recently
from
61e1947 to
368a402
Compare
random-zebra
force-pushed
the
202104_kill-comparison-framework
branch
from
368a402 to
9df921a
Compare
Author
|
Rebased on master, adding some more refactoring to address #2364 (review) |
random-zebra
added a commit
that referenced
this pull request
May 5, 2021
…SG usage for dynamic messages. 8ff6fa2 [test] validation_block_tests: New ASSERT_WITH_MSG to print dynamic error messages. (furszy) Pull request description: Bug discovered checking GA failing reason in #2314 --> [job](https://github.com/PIVX-Project/PIVX/pull/2314/checks?check_run_id=2509761057) Essentially, `BOOST_ASSERT_MSG` only prints static string messages. As we are inputting a dynamic message, the value isn't being printed if the test fails. So, have added a new function `ASSERT_WITH_MSG(cond, msg)` to support dynamic error messages. ACKs for top commit: random-zebra: ACK 8ff6fa2 and merging... (this means another rebase for #2360) Tree-SHA512: a47c8f94a497696a5bbb2b09b01f8dd1711b898a3d363211d12c79fd92b67a4f4d255a2c97ae192e17ccab706a604e16140a18bc7ea3f3539e8fad4559351902
random-zebra
force-pushed
the
202104_kill-comparison-framework
branch
4 times, most recently
from
11cb351 to
264399b
Compare
Author
|
Rebased. |
random-zebra
force-pushed
the
202104_kill-comparison-framework
branch
from
264399b to
0730891
Compare
Author
|
Rebased. |
In TestChainSetup::CreateBlock, the miner could have been credited with the fees spent by included mempool txes. Since those transactions are removed, we need to restore the coinbase out amount to the block value. Further, since the block could have included shield txes from the mempool, now removed, and/or shield txes passed from the caller, now included, we need to update the sapling root.
Extend its fixture from TestChain100Setup, thus avoid changing chain-params during the test case, and simplify the code.
Otherwise the block index would be no longer valid, should be discarded, and a new genesis block reloaded.
Without this, send_blocks_and_test() fails to send more than one block, as the GETBLOCKS replies from the node are not passed to on_getdata.
Nuke - ComparisonTestFramework - comptool - BlockStore - TxStore
random-zebra
force-pushed
the
202104_kill-comparison-framework
branch
from
0730891 to
48e5d54
Compare
Fuzzbawls
approved these changes
May 17, 2021
furszy
added a commit
that referenced
this pull request
May 20, 2021
…able block 2605f5b [BUG][Validation] Set DoS score for blocks with invalid PoW (random-zebra) a6d74e7 [BUG][P2P] Update mapBlockSource before processing new received block (random-zebra) Pull request description: Built on top of: - [x] #2360 Fix a couple bugs discovered with `feature_block`. Nodes not being disconnected after sending a block rejected during `ConnectBlock`, as the block source is not updated yet, and nodes not being disconnected after sending a block with invalid PoW (this only affects regtest chain). ACKs for top commit: furszy: Nice find 👌 , ACK 2605f5b Fuzzbawls: ACK 2605f5b Tree-SHA512: 165de5a1a3c8d26e2182d4e098004a18a84bf083e10d5b7b5529ecc229b4638d18fc6469f3b30ad1f9ceb797b5406657844cb7c0b6bf321a582c7aac9c6c2c24
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_blocktest, 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_blockdoing 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 modifynext_blockto 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
MAINtoREGTEST) 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).