-
Notifications
You must be signed in to change notification settings - Fork 716
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
[Tests] Resurrect feature_block and kill the p2p comparison test framework #2360
[Tests] Resurrect feature_block and kill the p2p comparison test framework #2360
Conversation
You deserve some beers! 🍻 🍻 |
61e1947
to
368a402
Compare
368a402
to
9df921a
Compare
Rebased on master, adding some more refactoring to address #2364 (review) |
…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
11cb351
to
264399b
Compare
Rebased. |
264399b
to
0730891
Compare
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
0730891
to
48e5d54
Compare
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.
Awesome work! 🍻
ACK 48e5d54 .
what a long long test..
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.
ACK 48e5d54
…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
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 modifynext_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
toREGTEST
) 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).