873beca6d [tests] Rename NodeConn and NodeConnCB (John Newbery)
Pull request description:
Final step in #11518
NodeConn -> P2PConnection
NodeConnCB -> P2PInterface
This is basically just a rename. Should be an easy review.
Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
ff8a9b0 [tests] Add combinedlogslen argument to test_runner.py (John Newbery)
dba94ea [tests] [travis-ci] Move Travis functional test log post processing to test_runner (John Newbery)
bba1c54 [tests] Improve logging shutdown and add hint for combine_logs (John Newbery)
Pull request description:
Replaces #11779 . Notes from that PR:
> Currently, when a functional test fails, the debug logs are printed sequentially to the travis log. This makes debugging race conditions based on the travis log hard. Instead, all logs events should be combined and sorted by their timestamp, then appended to the travis log.
@MarcoFalke
Tree-SHA512: 56c80067d6a2c92f7e6a35e3ae5160637a0de052d9da593c7be6e02233544a93c66d62456f903f85e2edc09e31ab4bdafd1aed1d9897ae48c634f82631f856f7
e9dfa9bcc [tests] Move version message sending from NodeConn to NodeConnCB (John Newbery)
dad596fc3 [tests] Make NodeConnCB a subclass of NodeConn (John Newbery)
e30d40438 [tests] Move only: move NodeConnCB below NodeConn (John Newbery)
4d5059856 [tests] Tidy up mininode (John Newbery)
f2ae6f32a [tests] Remove mininode periodic (half-hour) ping messages (John Newbery)
ec59523c5 [tests] Remove rpc property from TestNode in p2p-segwit.py. (John Newbery)
Pull request description:
This is the final step in #11518, except for possibly renaming - for motivation, please see that PR.
If this is merged, then migrating the test framework from asyncore to asyncio should be easier (I say should because I haven't dug too deeply into what would be required).
Requesting review from @ryanofsky , since he always has good feedback on these refactor PRs, and I'd appreciate his take on this refactor. Note particularly that I've reverted the change suggested here: https://github.com/bitcoin/bitcoin/pull/11182#discussion_r148859555 . The idea, as always, is to present a simple interface to the test writer.
Tree-SHA512: 94dd467a13ec799b101108cf47d4dccb6f6240b601e375e3d785313333bbb389c26072a50759aca663bbf3d6c8b867b99e36ae8800ab8ea115e0496c151926ce
Instead of calling combine_logs from test_runner when the environment variable
PYTHON_DEBUG is set, add a combinedlogslen argument. This makes it easier
to call when running locally, and allows a different length of combined logs
to be printed.
This commit moves the logic that sends a version message
on connection from NodeConn to NodeConnCB. NodeConn should
not be aware of the semantics or meaning of the P2P payloads.
This makes NodeConnCB a subclass of NodeConn, and
removes the need for the client code to know
anything about the implementation details of NodeConnCB.
NodeConn can now be swapped out for any other implementation
of a low-level connection without changing client code.
9d42cc333 [tests] address review comments (John Newbery)
f0c4ab9a7 [tests] fix flakiness in sendheaders.py (John Newbery)
25fd6e2c2 [tests] refactor check_last_announcement() in sendheaders.py (John Newbery)
f39d4bbd1 [tests] tidy up BaseNode in sendheaders.py (John Newbery)
2613c545f [tests] fix flake8 warnings in sendheaders.py (John Newbery)
Pull request description:
This PR should fix the intermittent failure of sendheaders.py described in #11673. The first three commits are tidying up and refactoring the file. The final commit _fix flakiness in sendheaders.py_ fixes the intermittent failures. The commit message for that commit describes the problems that are being fixed.
I think @laanwj @MeshCollider @MarcoFalke have seen these failures.
fixes#11673
Tree-SHA512: 278e1af85f2eae00f970f2d8ef33686dd52b4f62180dea4cfdaff7bcf3287c6f1c2930355d99461a12f0c51c4d42cc3b1cb3275174134028ca4d06ffc24c18dd
c1e5d40 Make debugging test crash easier (MeshCollider)
8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider)
9587a9c Default walletdir is wallets/ if it exists (MeshCollider)
d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider)
80c5cbc Add test for -walletdir (MeshCollider)
0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider)
Pull request description:
Closes#11348
Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists.
Includes tests and release notes. Things which might need to be considered more:
- there is no 'lock' on the wallets directory, which might be needed?
- because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir)
- jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in https://github.com/bitcoin/bitcoin/pull/11687
- doc/files.md needs updating (will do soon)
I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review.
Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
Currently it's possible to accidentally type e.g.
bitcoin-cli -getinfo getbalance
and get an answer which can be confusing; the trialing arguments are
just ignored.
To avoid this, throw an error if the user provides arguments to
`-getinfo`.
1135c79 [tests] Tidy up mininode.py module (John Newbery)
f9cd9b1 [tests] Move test_framework Bitcoin primitives into separate module (John Newbery)
Pull request description:
Second part of #11518.
Moves the primitive Bitcoin datastructures and message classes into their own module, and tidies up the mininode.py module.
- First commit is almost entirely move-only
- Second commit is mostly move-only, but also does a little tidying.
Tree-SHA512: 5d74802677f1ab788e43188653106a96fffd9ab1fe3aa6a4eb94e5807de5dd5c8ee212296f45e8d16c7e3d95cfc4891677e812b7944bd3ab604e04b3b88aa06e
Fixes to sources of intermittent failure in sendheaders.py
- at the start of test_null_locators(), a new block is generated and
then a getheaders is sent. check_last_accouncement() is called to assert
that the headers message is received. However, the new block triggers an
inv to be sent over both P2P connections, so there's a race. If the inv
is received at the wrong time, the test fails.
- test_null_locators() ends by sending a block to the node under test.
At the start of test_nonnull_locators(), a block is mined and
check_last_announcement() is called to assert that the inv received is
for the same block. That means there's a race: if the inv from the block
sent in test_null_locators() is received at the wrong time, the test
fails.
All calls of check_last_announcement() asserted that the return
value was True. Just assert inside the function instead. This
gives better debug information if the assert fails.
Also only check the contents of the most recent inv and header if
check_last_announcement() is called with the relevant argument.
28f8b66 Diagnose unsuitable outputs in lockunspent(). (Eelis)
Pull request description:
Fixes#2667.
This is a simplified version of pull request #3574, which was abandoned by its author.
I added some tests as well.
Tree-SHA512: e63e00dec8b1b232079380183805cb0b0b18c78ea6bea769837949aab984689d7f68b2ccfe66b1873517b040b9e616ce0eb058575c3d4382aa8c26eebcf1f14e
fafdad0d4 qa: Remove unused NodeConn members (MarcoFalke)
Pull request description:
* `ver_send` and `ver_recv` were completely unused
* `rpc` was only used once, in p2p-segwit. Imo better only pass it to the constructor in that single test
Tree-SHA512: 7f85554d6d0fd2096516ca3c608811d5370da66cde35d8031bdc921607a7a4efdb26355896012f75f713f8df09e28d46ba46be69fd96a5898fabb1a25cbcb8ad
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
5e0ba8f8c [wallet] getreceivedbyaddress should return error if address is not mine (John Newbery)
ea0cd24f7 [tests] Tidy up receivedby.py (John Newbery)
Pull request description:
Two commits:
- First commit tidies up the `receivedby.py` test (and speeds it up by factor of two)
- Second commit changes getreceivedbyaddress to return error if the address is not found in wallet, and adds test to `receivedby.py`
Tree-SHA512: e41342dcbd037a6b440cbe4ecd3b8ed589e18e477333f0d866f3564e948e0f5231e497d5ffb66da4e6680eb772d9f0cf839125098bb68b92d04a5ee35c6c0a81
11413646b [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c18171 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)
Pull request description:
Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.
First discussed in #10357 here: https://github.com/bitcoin/bitcoin/pull/10357#pullrequestreview-59963870
> ... we could simplify this (and possibly other) tests by just adding a way to know if a node is in IBD. I'd like to do that, but I'm not sure it makes sense to complicate this PR with discussion over how that information should be made available. Eg it's not clear to me that the notion of being in IBD is worth exposing to the casual user, versus a hidden rpc call or something, since the definition has changed over time, and may continue to change in the future. But I still do agree that at least for testing purposes it would be far simpler to expose the field somehow...
This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:
1. add a new debug RPC method that exposes `IBD` and potentially other information.
2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.
I quite like the idea of (3). Feedback on these and other approaches very much welcomed!
@sdaftuar @laanwj
Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
- Fix flake8 warnings
- Remove the useless get_sub_array_from_array() function
- Reduce runtime for receivedby.py by about half by only using two nodes
748157913 [tests] Make comp test framework more debuggable (John Newbery)
Pull request description:
We should remove the comparison test framework entirely (see #10603).
Until we do that, let's make it a bit more debuggable. Currently, if there's an assert in the framework, it's very difficult to track down where we are in the test generator. Make the logging a bit better to help with debugging.
Before this PR:
```
→ ./p2p-fullblocktest.py
2017-10-09 14:05:11.302000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testzdnax_yr
2017-10-09 14:05:11.557000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:11975
2017-10-09 14:05:11.712000 TestFramework.comptool (INFO): Test 1: PASS
2017-10-09 14:05:11.947000 TestFramework.comptool (INFO): Test 2: PASS
2017-10-09 14:05:12.057000 TestFramework.comptool (INFO): Test 3: PASS
2017-10-09 14:05:12.058000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "./p2p-fullblocktest.py", line 72, in run_test
self.test.run()
File "/home/ubuntu/bitcoin/test/functional/test_framework/comptool.py", line 306, in run
assert test_number != 4
AssertionError
2017-10-09 14:05:12.059000 TestFramework (INFO): Stopping nodes
2017-10-09 14:05:14.203000 TestFramework (WARNING): Not cleaning up dir /tmp/user/1000/testzdnax_yr
2017-10-09 14:05:14.204000 TestFramework (ERROR): Test failed. Test logging available at /tmp/user/1000/testzdnax_yr/test_framework.log
```
With this PR:
```
→ ./p2p-fullblocktest.py
2017-10-09 14:03:54.069000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testuey7t3tf
2017-10-09 14:03:54.329000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:11783
2017-10-09 14:03:54.383000 TestFramework.comptool (INFO): Running test 1: ./p2p-fullblocktest.py line 184
2017-10-09 14:03:54.496000 TestFramework.comptool (INFO): Running test 2: ./p2p-fullblocktest.py line 193
2017-10-09 14:03:54.758000 TestFramework.comptool (INFO): Running test 3: ./p2p-fullblocktest.py line 205
2017-10-09 14:03:54.867000 TestFramework.comptool (INFO): Running test 4: ./p2p-fullblocktest.py line 208
2017-10-09 14:03:54.867000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "./p2p-fullblocktest.py", line 72, in run_test
self.test.run()
File "/home/ubuntu/bitcoin/test/functional/test_framework/comptool.py", line 309, in run
assert test_number != 4
AssertionError
2017-10-09 14:03:54.868000 TestFramework (INFO): Stopping nodes
2017-10-09 14:03:56.950000 TestFramework (WARNING): Not cleaning up dir /tmp/user/1000/testuey7t3tf
2017-10-09 14:03:56.950000 TestFramework (ERROR): Test failed. Test logging available at /tmp/user/1000/testuey7t3tf/test_framework.log
```
Tree-SHA512: 5525958b0098d661c281bd955c92c72bf81359464376e96d44e6c88f18aea57ba08270ecd564edda4b47d674e3b27d20e5b1060544bf8dd5c6a68103d2bb35b8
725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky)
3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo)
Pull request description:
Nowhere else in the protocol do we send headers which are for
blocks we have not fully validated except in response to getheaders
messages with a null locator. On my public node I have not seen any
such request (whether for an invalid block or not) in at least two
years of debug.log output, indicating that this should have minimal
impact.
Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a
fb00c45c3 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aabbd [tests] Remove support for p2p alert messages (John Newbery)
c0b127470 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e301c [tests] Remove dead code from mininode.py (John Newbery)
Pull request description:
This is the first part of #11518. It removes a ~150 lines of unused code from the mininode module:
- remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
- remove support for pre-BIP31 ping messages
- remove support for alert message
- explicitly don't support p2p versions lower than 60001
Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)
Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0
bc9c0a7 Improve wallet-accounts test (Russell Yanofsky)
Pull request description:
Add comments and
- Verify sending to a account causes getaccountaddress to generate new addresses.
- Verify sending to a account causes getreceivedbyaccount to return amount received.
- Verify ways setaccount updates the accounts of existing addresses.
Tree-SHA512: 4facc8d10fb23847081e8081839d4050faa855cca40f34012b82297bf1a8cfdb975b26314bc518dc6b3f5997452a7acf309f9627e0dff359347c628b37014bfa
The mininode module includes code to support p2p versions below
60001. However, the test_framework does not support versions
of Bitcoin Core before V0.13.0. Remove code supporting
p2p versions before 60001 (which has never been run).
BIP31 support was added to Bitcoin Core in version 0.6.1. Our test
framework is incompatible with Bitcoin Core versions that old, so remove
all special logic for handling pre-BIP31 pings.
