Skip to content

Backport orphan TX tests and dependent PRs #3127

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.

Sign up for GitHub

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

Jump to bottom
Merged
UdjinM6 merged 9 commits into from
Oct 7, 2019
Merged

Backport orphan TX tests and dependent PRs #3127

UdjinM6 merged 9 commits into from
Oct 7, 2019

Conversation

@codablock
Copy link

@codablock codablock commented Sep 30, 2019

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_out validation, causing rejection due to too low fees (actually, negative fees) instead of rejection and banning due to an invalid TX.

@codablock codablock added this to the 14.1 milestone Sep 30, 2019
@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Sep 30, 2019

Looks good 👍 I will review later!

@codablock
Copy link
Author

codablock commented Oct 1, 2019

Test failure should be fixed by #3129. I'll rebase this PR on develop after that is merged.

UdjinM6
UdjinM6 reviewed Oct 1, 2019
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 few nits, see below

PastaPastaPasta
PastaPastaPasta reviewed Oct 1, 2019
View reviewed changes
@UdjinM6
Copy link

UdjinM6 commented Oct 2, 2019

Needs rebase

@codablock codablock force-pushed the pr_backport_orphantx_tests branch from e162d79 to 983e81f Compare October 2, 2019 09:21
@codablock
Copy link
Author

codablock commented Oct 2, 2019

Rebased on develop

UdjinM6
UdjinM6 requested changes Oct 2, 2019
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, tests time out because of missing sync_masternodes, see below

@codablock codablock force-pushed the pr_backport_orphantx_tests branch from 38de024 to d69bca5 Compare October 2, 2019 13:26
UdjinM6
UdjinM6 previously approved these changes Oct 2, 2019
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@UdjinM6
Copy link

UdjinM6 commented Oct 3, 2019 •
edited
Loading

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

MarcoFalke and others added 6 commits October 3, 2019 14:04
@codablock
Merge bitcoin#11182: [tests] Add P2P interface to TestNode
7c163b1 
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
@laanwj @codablock
Merge bitcoin#11641: qa: Only allow disconnecting all NodeConns
6e6e950 
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
@laanwj @codablock
Merge bitcoin#11849: [tests] Assert that only one NetworkThread exists
f4ea836 
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
@laanwj @codablock
Merge bitcoin#11771: [tests] Change invalidtxrequest to use BitcoinTe…
01c138e 
…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
@codablock
Use NodeConnCB as base for P2PDataStore
573d1da 
Until the renaming to P2PInterface is backported.
@codablock
Merge bitcoin#11772: [tests] Change invalidblockrequest to use Bitcoi…
3e72a09 
…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 codablock force-pushed the pr_backport_orphantx_tests branch from d69bca5 to d21031d Compare October 3, 2019 12:10
@codablock
Copy link
Author

codablock commented Oct 3, 2019

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.

@UdjinM6
Copy link

UdjinM6 commented Oct 3, 2019 •
edited
Loading

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.

laanwj and others added 3 commits October 3, 2019 17:04
@laanwj @codablock
Merge bitcoin#13003: qa: Add test for orphan handling
ed65d51 
fa02c5b qa: Clarify documentation for send_txs_and_test (MarcoFalke)
fadfbd3 qa: Add test for orphan handling (MarcoFalke)

Pull request description:

Tree-SHA512: e0932d6bd03c73e3113f5457a3ffa3bbfc7b6075dfca8de95224d9df875e60ca6eb15cd8baa226f13de965483006559556191630a83c3bb431e79c53a85ef73f
@laanwj @codablock
Merge bitcoin#8498: Near-Bugfix: Optimization: Minimize the number of…
8bcba5d 
… 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
Remove unnecessary time imports
30767ff
@codablock codablock force-pushed the pr_backport_orphantx_tests branch from d21031d to 30767ff Compare October 3, 2019 15:12
@codablock
Copy link
Author

codablock commented Oct 3, 2019

@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).

UdjinM6
UdjinM6 approved these changes Oct 3, 2019
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@UdjinM6 UdjinM6 mentioned this pull request Oct 3, 2019
Merged
@codablock codablock mentioned this pull request Oct 4, 2019
Merged
nmarley
nmarley approved these changes Oct 7, 2019
View reviewed changes
Copy link

@nmarley nmarley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@UdjinM6 UdjinM6 merged commit 47c7f42 into dashpay:develop Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

+1 more reviewer

@nmarley nmarley nmarley approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

15

Development

Successfully merging this pull request may close these issues.

5 participants

@codablock @PastaPastaPasta @UdjinM6 @nmarley @laanwj