[tests] Change feature_block.py to use BitcoinTestFramework #11773
Conversation
JohnVonNeumann
left a comment
JohnVonNeumann
left a comment
There was a problem hiding this comment.
Just some basic review of code standards/conventions. Mostly applies to the refactored code, new code looks to have done the job from the get go.
There was a problem hiding this comment.
Unsure if you guys adhere directly to PEP8 but this comment looks like it could be removed, seems redundant.
There was a problem hiding this comment.
Would imagine this would be best moved inline as opposed to on it's own line to keep convention with inline comments like on line 32 and 29.
There was a problem hiding this comment.
Would assume this block comment should be moved into a docstring block like in test/functional/test_framework/mininode.py P2PStub class. Seems like the standard are great in there, I'm assuming that's down to this section being a refactor.
test/functional/invalidtxrequest.py
Outdated
There was a problem hiding this comment.
Newly added whitespace can be removed.
|
Thanks for your input @JohnVonNeumann. Three of your nits are on comments that are already in the code, and the last is to revert a pep8 fix (pep8 states that 'inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space.') the invalidtxrequest and invalidblockrequest refactors are covered by #11771 and #11772. If you have functional review of those tests, can you leave comments in those PRs? |
|
Is it typically best off to leave preexisting nits for other PRs? Is that something I could fix myself and submit as a PR? Would that be the typical way to address it? I assume you look to keep the PRs clean otherwise people would just keep bringing up little things and it would be difficult to tell what was actually being added in the PR itself? Apologies on the last PEP8 spacing comment too! I missed that part when reading the docs! Will have a look at the other PRs and comment in those. Thanks for the feedback as well. EDIT: Also sorry for not seeing that you had asked for #11771 and #11772 to be reviewed first, I didn't read the PR notes thoroughly, will keep an eye on them better from now on. |
I usually clean up style nits as I go, and precede functional-change commits with tidy-up commits as I have in this PR. The nits you've raised are more your personal preference than following any particular style convention, so I don't think there's any benefit for you to open a PR for them. |
|
Ok this makes sense, although I disagree that all my comments were related directly to my personal preference, for me it was just about keeping the standards throughout the code, for example with the redundant inline comment, I don't see any of them throughout the newly written code, so it would make sense to not have them in the old code. However, you are probably right, and I'll happily defer to your experience. I'll try and make my reviews more valuable. Cheers |
jnewbery
force-pushed
the
refactor_p2pfullblocktest
branch
from
36cfdb9 to
d0c2ef8
Compare
|
This requires rebase, but I'll leave it as it is until #11771 is reviewed/merged. |
jnewbery
force-pushed
the
refactor_p2pfullblocktest
branch
2 times, most recently
from
54779f9 to
13ed1eb
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Skipped first two commits which are from #11771 and not up to date. Reviewed remaining individual commits:
- utACK 13ed1ebe08d100500a8f5311c0f91fe4d8f680db [tests] Improve assert message when wait_until() fails
- utACK 1e5da2e20d655d602e62ed87a80df0f23bbe7a34 [tests] Change p2p-fullblocktest to use BitcoinTestFramework
- utACK 63782cd7c7e864f73b2fb715e24513d3d3117214 [tests] Add logging to p2p-fullblocktest.py
- utACK a9554710e65484d7973b706829be67faff53c357 [tests] Tidy up p2pfullblocktest
- utACK 40f7f6b99698552e71537c97e76c2011ebaa5626 [tests] Fix flake8 warnings in p2p-fullblocktest
test/functional/p2p-fullblocktest.py
Outdated
There was a problem hiding this comment.
In commit "Tidy up p2pfullblocktest"
Can you note that this option was removed in the commit message (and maybe say why it wasn't useful?)
There was a problem hiding this comment.
Yep. Added to commit message.
jnewbery
force-pushed
the
refactor_p2pfullblocktest
branch
from
13ed1eb to
c10fc8e
Compare
|
Rebased now that #11771 has been merged. |
jnewbery
force-pushed
the
refactor_p2pfullblocktest
branch
2 times, most recently
from
c9d15e4 to
061f4d8
Compare
jnewbery
changed the title
jnewbery
changed the title
|
ACK |
|
utACK 061f4d8cfba58d299dd913bd96f88bddebae8276 Thanks for a very nice cleanup! Looking forward to seeing this merged! |
jnewbery
force-pushed
the
refactor_p2pfullblocktest
branch
from
061f4d8 to
ba87be8
Compare
- move all helper methods to the end - remove block, create_tx and create_and_sign_tx shortcuts - remove --runbarelyexpensive option, since it defaults to True and it's unlikely that anyone ever runs the test with this option set to false.
|
Test re-ACK 265d7c4 |
|
Ready for merge? |
|
Needs more (re) acks
…On Tue, Mar 27, 2018, 09:21 jamesob ***@***.***> wrote:
Ready for merge?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11773 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGGmv32N2D39OikZv_aEbDkLMcUm6iy6ks5tijzTgaJpZM4Qr7m3>
.
|
|
@ryanofsky @practicalswift - care to reACK? |
| self.blocks[block_number] = block | ||
| return block | ||
|
|
||
| def reconnect_p2p(self): |
There was a problem hiding this comment.
I'm pretty sure this functionality gets repeated in several other tests. If so, we should move this somewhere more general and consolidate usages later on.
There was a problem hiding this comment.
Sure, but let's save that for a future PR.
|
4 ACKs. Ready for merge @MarcoFalke ? |
|
utACK 265d7c4 |
|
@laanwj Could you please merge this, since I am using my travel laptop for the next few days? |
…work 265d7c4 [tests] Improve assert message when wait_until() fails (John Newbery) ebf053a [tests] Change feature_block.py to use BitcoinTestFramework (John Newbery) fc02c12 [tests] Add logging to feature_block.py (John Newbery) 3898c4f [tests] Tidy up feature_block.py (John Newbery) 5cd01d2 [tests] Fix flake8 warnings in feature_block.py (John Newbery) Pull request description: Next step in #10603. - first three commits tidy up feature_block.py - fourth commit removes usage of ComparisonTestFramework Longer term, it would be better to separate net_processing testing from validation testing, but I think this is still a useful PR, since it moves us away from the comparison test framework. Tree-SHA512: d0bb3ad22ad0aa1222877f4212bff075f9ce358e99c69c26d9913e4b346d931b8380e744434a9f6f37812c352cdaa75791691565bfeb18afcb619c06c6ca32a3
|
It looks like ebf053a is causing feature_block.py to use about 5GB of RSS memory instead of about 1GB, haven't had time to look into quite why yet. It's causing MemoryError exceptions on my 4GB vm :( |
…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
| assert_greater_than(attempts, attempt) | ||
| assert_greater_than(timeout, time.time()) | ||
| predicate_source = inspect.getsourcelines(predicate) | ||
| logger.error("wait_until() failed. Predicate: {}".format(predicate_source)) |
There was a problem hiding this comment.
This results in a spurious error message when wait_until is used and failure is expected, for instance in the p2p_segwit.py test where we advertise a txid from a not-NODE_WITNESS peer and ensure that a subsequence announcement from a NODE_WITNESS peer does not result in a duplicate getdata (while the first request is outstanding).
…nTestFramework (#3277) * [tests] Change feature_block.py to use BitcoinTestFramework * [tests] Fix flake8 warnings in feature_block.py * [tests] Tidy up feature_block.py - move all helper methods to the end - remove block, create_tx and create_and_sign_tx shortcuts - remove --runbarelyexpensive option, since it defaults to True and it's unlikely that anyone ever runs the test with this option set to false. * [tests] Add logging to feature_block.py * [tests] Improve assert message when wait_until() fails * Merge bitcoin#13048: [tests] Fix feature_block flakiness c1d7420 [tests] Fix feature_block flakiness (John Newbery) Pull request description: feature_block.py occasionally fails on Travis. I believe this is due to a a race condition when reconnecting to bitcoind after a subtest that expects disconnection. If the test runs ahead and sends the INV for the subsequent test before we've received the initial sync getheaders, then we may end up sending two headers messages - one as a response to the initial sync getheaders and one in response to the INV getheaders. If both of those headers fail validation with a DoS score of 50 or higher, then we'll unexpectedly be disconnected. There is only one validation failure that has a DoS score bewteen 50 and 100, which is high-hash. That's why the test is failing immediately after the "Reject a block with invalid work" subtest. Fix is to wait for the initial getheaders from the peer before we start populating our blockstore. That way we won't have any invalid headers to respond to it with. Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f * Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE We'll undo this after the next commit. This avoids merge many conflicts and makes reviewing easier. * Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE * Use DoS score of 100 for bad-blk-sigops This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py * Use allowOptimisticSend=true when sending reject messages This fixes test failures in p2p-fullblocktest.py which expects reject messages to be sent/received before connections get closed. * Fix p2p-fullblocktest.py - CBlock and friends are still in test_framework.mininode - "-whitelist" causes connections to not be dropped, which in turn causes sync_blocks with reconnect=True to fail - "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False - Dash already bans when a header is received which is a child of an invalid header, causing block requests to never happen * Backport missing changes from bitcoin#13003 bitcoin#13003 was backported out of order which causes missed changes. * Bump p2p-fullblocktest timeouts * Increase RPC timeout in p2p-fullblocktest.py Co-authored-by: John Newbery <[email protected]> Co-authored-by: MarcoFalke <[email protected]>
…nTestFramework (dashpay#3277) * [tests] Change feature_block.py to use BitcoinTestFramework * [tests] Fix flake8 warnings in feature_block.py * [tests] Tidy up feature_block.py - move all helper methods to the end - remove block, create_tx and create_and_sign_tx shortcuts - remove --runbarelyexpensive option, since it defaults to True and it's unlikely that anyone ever runs the test with this option set to false. * [tests] Add logging to feature_block.py * [tests] Improve assert message when wait_until() fails * Merge bitcoin#13048: [tests] Fix feature_block flakiness c1d7420 [tests] Fix feature_block flakiness (John Newbery) Pull request description: feature_block.py occasionally fails on Travis. I believe this is due to a a race condition when reconnecting to bitcoind after a subtest that expects disconnection. If the test runs ahead and sends the INV for the subsequent test before we've received the initial sync getheaders, then we may end up sending two headers messages - one as a response to the initial sync getheaders and one in response to the INV getheaders. If both of those headers fail validation with a DoS score of 50 or higher, then we'll unexpectedly be disconnected. There is only one validation failure that has a DoS score bewteen 50 and 100, which is high-hash. That's why the test is failing immediately after the "Reject a block with invalid work" subtest. Fix is to wait for the initial getheaders from the peer before we start populating our blockstore. That way we won't have any invalid headers to respond to it with. Tree-SHA512: dc17d795fcfaf0f8c0bf1e9732b5e11fbc8febbfafba4c231b7c13a5404a2c297dcd703a7a75bc7f353c893e12efc87f424f2201abd47ba5268af32d4d2e841f * Temporarely rename MAX_BLOCK_SIZE -> MAX_BLOCK_BASE_SIZE We'll undo this after the next commit. This avoids merge many conflicts and makes reviewing easier. * Rename MAX_BLOCK_BASE_SIZE back to MAX_BLOCK_SIZE * Use DoS score of 100 for bad-blk-sigops This was accidently changed to 10 while backporting bitcoin#7287 and causes test failures in p2p-fullblocktest.py * Use allowOptimisticSend=true when sending reject messages This fixes test failures in p2p-fullblocktest.py which expects reject messages to be sent/received before connections get closed. * Fix p2p-fullblocktest.py - CBlock and friends are still in test_framework.mininode - "-whitelist" causes connections to not be dropped, which in turn causes sync_blocks with reconnect=True to fail - "bad-cb-amount" does not cause a ban in Dash, so reconnect must be False - Dash already bans when a header is received which is a child of an invalid header, causing block requests to never happen * Backport missing changes from bitcoin#13003 bitcoin#13003 was backported out of order which causes missed changes. * Bump p2p-fullblocktest timeouts * Increase RPC timeout in p2p-fullblocktest.py Co-authored-by: John Newbery <[email protected]> Co-authored-by: MarcoFalke <[email protected]>
11 participants
Next step in #10603.
Longer term, it would be better to separate net_processing testing from validation testing, but I think this is still a useful PR, since it moves us away from the comparison test framework.