Backport orphan TX tests and dependent PRs#3127
Merged
UdjinM6 merged 9 commits intodashpay:developfrom Oct 7, 2019
Merged
Conversation
Member
|
Looks good 👍 I will review later! |
Author
|
Test failure should be fixed by #3129. I'll rebase this PR on develop after that is merged. |
|
Needs rebase |
codablock
force-pushed
the
pr_backport_orphantx_tests
branch
from
e162d79 to
983e81f
Compare
Author
|
Rebased on develop |
UdjinM6
requested changes
Oct 2, 2019
codablock
force-pushed
the
pr_backport_orphantx_tests
branch
from
38de024 to
d69bca5
Compare
UdjinM6
dismissed
their stale review
Hmmm... Looks like it timed out even with my suggestions applied https://travis-ci.org/dashpay/dash/jobs/592557866#L2562
|
I think we need (at least) bitcoin#13048 to fix tests. EDIT: and maybe we should also bump some timeouts, see bitcoin@fa05d52#diff-3c4697a632e54ae2c6e68e0159ec2234R1184 and bitcoin@fa502cb#diff-3c4697a632e54ae2c6e68e0159ec2234R1264 |
32ae82f [tests] use TestNode p2p connection in tests (John Newbery) 5e5725c [tests] Add p2p connection to TestNode (John Newbery) b86c1cd [tests] fix TestNode.__getattr__() method (John Newbery) Pull request description: Final two steps of bitcoin#10082 : Adding the "mininode" P2P interface to `TestNode` This PR adds the mininode P2P interface to `TestNode`. It simplifies the process for opening a P2P connection to the node-under-test from this: ```python node0 = NodeConnCB() connections = [] connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0)) node0.add_connection(connections[0]) ``` to this: ```python self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB) ``` The first commit adds the infrastructure to `test_node.py`. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR. Tree-SHA512: 44f1a6320f44eefc70489ae8350c6a16ad1a6035e4b9b7bafbdf19f5905ed0e2db85beaaf4758eec3059dd89a375a47a45352a029f39f57a86ab38a9ae66650e
faaa7db qa: Only allow disconnecting all NodeConns (MarcoFalke) Pull request description: Disconnecting the connection with `index=0` makes no sense when there are more than one connections, as the list "rotates around" and populates index 0 after `del`. Just disconnect all NodeConns in any case. Tree-SHA512: e5cf540823fccb31634b5a11501f54222be89862e80ccafc28bc06726480f8d2153b8c1b6f859fa6a6d087876251d48a6c6035bccdaaf16831e300bc17ff613d
5c8ff26 [tests] Add NetworkThread assertions (John Newbery) 34e08b3 [tests] Fix network threading in functional tests (John Newbery) 74e64f2 [tests] Use network_thread_start() in tests. (John Newbery) 5fc6e71 [tests] Add network_thread_ utility functions. (John Newbery) Pull request description: Add assert that only one NetworkThread exists at any time in functional tests, and fix cases where that wasn't true. fixes bitcoin#11776 Tree-SHA512: fe5d1c59005f94bf66e11bb23ccf274b1cd9913741b56ea11dbcd21db4cc0b53b4413c0c4c16dbcd6ac611adad5e5cc2baaa39720598ce7b6393889945d06298
…stFramework 95e2e9a [tests] Change invalidtxrequest to use BitcoinTestFramework (John Newbery) 359d067 [tests] Fix flake8 warnings in invalidtxrequest (John Newbery) c32cf9f [tests] Add P2PDataStore class (John Newbery) cc046f6 [tests] Reduce NodeConn connection logging from info to debug (John Newbery) Pull request description: Next step in bitcoin#10603 - first commit changes log level for an internal log from INFO to DEBUG. (Not really related, but I started finding the INFO level logging annoying when debuging test failures) - second commit introduces a `P2PStub` class - a subclass of `NodeConnCB` which has its own block and tx store and responds appropriately to getdata requests. Not all the functionality is used in `invalidtxrequest.py`, but will be used in `invalidblockrequest.py` and `p2p-fullblocktest` when those are changed to use `BitcoinTestFramework` - third commit tidies up `invalidtxrequest.py` - fourth commit removes usage of `ComparisonTestFramework` Tree-SHA512: f3085c73c15d6ce894e401490bce8a7fa7cf52b0c9d135ff7e351f1f6f517c99accab8588fcdc443f39ea8315329aaabd66b2baa32499df5a774737882030373
Until the renaming to P2PInterface is backported.
…nTestFramework e97b113 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery) 2b7064e [tests] Fix flake8 warnings in invalidblockrequest (John Newbery) 54b8c58 [test] Fix nits leftover from 11771 (Conor Scott) Pull request description: Builds on bitcoin#11771. Please review that PR first Next step in bitcoin#10603. - first commit tidies up invalidblockrequest.py - second commit removes usage of ComparisonTestFramework Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456
codablock
force-pushed
the
pr_backport_orphantx_tests
branch
from
d69bca5 to
d21031d
Compare
Author
|
I cherry picked bitcoin#13048 and rebased on develop to bring in the optimizations in prevector and sha256 hashing. These should speed up p2p-fullblocktest, and maybe it's not even required then to bump timeouts. |
|
Trying the same in my local branch and it still times out sometimes... :/ Thought I might dig a bit deeper and see what other patches were there and found this bitcoin#13517 but it requires bitcoin#11791 which in its turn requires bitcoin#11648 and this one also does not seem to be cleanly mergeable without some other backports... so I stopped. It kind of looks like we are starting to backport 0.16 to fix it and I'm no longer sure that the orphan test we were aiming to backport initially worths creating a mess thanks to multiple out-of-sequence merges. I think we should maybe postpone this. |
… times it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...is created by individual transactions to 2 places (but call only once in each): - ConnectBlock ( before calculated fees per txs twice ) - AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated fees per tx one extra time ) Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2) For more motivation: ~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~ jtimon/bitcoin@0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments EDIT: partially replaces dashpay#6445 Near-Bugfix as pointed out in bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346
codablock
force-pushed
the
pr_backport_orphantx_tests
branch
from
d21031d to
30767ff
Compare
Author
|
@UdjinM6 agree, the amount of out-of-band backports required to support the changes in p2p-fullblocktest.py got too much already. I've removed the changes (especially bitcoin#11773) to p2p-fullblocktest.py now so that this PR only focuses on the changes needed for the orphan TX tests (should have done that in the beginning...but I realized too late that I backported too much). |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
See individual commits.
The purpose of this PR is to bring in orphan TX tests into develop. This required to backport a few PRs before actually being able to backport bitcoin#13003.
I later realized that backporting bitcoin#11773 and probably bitcoin#11772 was not needed, but now the work is done and I'd like to include this here as well.
Backporting the orphan tests revealed a bug that is fixed by backporting bitcoin#8498. Without this fix, fee validation would happen before
nValueIn < value_outvalidation, causing rejection due to too low fees (actually, negative fees) instead of rejection and banning due to an invalid TX.