d280617bf5 [qa] Add a test for merkle proof malleation (Suhas Daftuar)
ed82f17000 have verifytxoutproof check the number of txns in proof structure (Gregory Sanders)
Pull request description:
Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/
This change would at least allow `verifytxoutproof` to properly validate that the proof matches a known block, with known number of transactions any time after the full block is processed. This should neuter the attack entirely.
The negative is that a header-only processed block/future syncing mode would cause this to fail until the node has imported the data required.
related: #13451
`importprunedfunds` needs this check as well. Can expand it to cover this if people like the idea.
Tree-SHA512: 0682ec2b622a38b29f3f635323e0a8b6fc071e8a6fd134c954579926ee7b516e642966bafa667016744ce49c16e19b24dbc8801f982a36ad0a6a4aff6d93f82b
09c6699900 [qa] Handle disconnect_node race (Suhas Daftuar)
Pull request description:
Several tests call disconnect_nodes() on each node-pair in rapid
succession, resulting in a race condition if a node disconnects a peer
in-between the calculation of the nodeid's to disconnect and the
invocation of the disconnectnode rpc call. Handle this.
Tree-SHA512: 3078cea0006fcb507c812004a777c505eb1e9dda7c6df12dbbe72395a73ff6f6760f597b6492054f5487b34534417ddef5fbad30553c135c288c4b7cfce79223
Several tests call disconnect_nodes() on each node-pair in rapid
succession, resulting in a race condition if a node disconnects a peer
in-between the calculation of the nodeid's to disconnect and the
invocation of the disconnectnode rpc call. Handle this.
By default, libc will print fatal errors to /dev/tty instead of stderr.
Adding the LIBC_FATAL_STDERR_ to the environment variables allows
us to catch libc errors in stderr and test for them.
fac1e1f qa: Remove unused option --srcdir (MarcoFalke)
Pull request description:
The `srcdir` option was both unused and misleading; It should have been called `builddir`. So remove it.
Tree-SHA512: 2c24dcf2aa82219158b8cbbf03dd3f0f51f805f1f5f670faa1fd59e5a8d60fda120ffddadeccb058d8d3f20583b4952be7afd2df6bbefb9367d35c0f0a9fda3c
891beb0 [test] fundrawtransaction: lock watch-only shared address (Karl-Johan Alm)
Pull request description:
`self.nodes[0]` creates an address which is watch-only-shared with `self.nodes[3]`. If `nodes[0]` spends the associated UTXO during any of its sends later, the watchonly test will fail, as `nodes[3]` now has insufficient funds.
I ran into this in #12257 and this commit is in that PR as well, but I figured I'd split it out (and remove from there once/if merged).
Tree-SHA512: d04a04b1ecebe82127cccd47c1b3de311bf07f4b51dff80db20ea2f142e1d5c4a85ed6180c5c0b081d550e238c742e119b953f60f487deac5a3f3536e1a8d9fe
fa811b0 qa: Normalize executable location (MarcoFalke)
Pull request description:
This removes the need to override the executable locations by just reading them from the config file. Beside making the code easier to read, running individual test on Windows is now possible by default (without providing further command line arguments).
Note: Of course, it is still possible to manually specify the location through the `BITCOIND` environment variable, e.g. `bitcoin-qt`.
Tree-SHA512: bee6d22246796242d747120ca18aaab089f73067de213c9111182561985c5912228a0b0f7f9eec025ecfdb44db031f15652f30d67c489d481c995bb3232a7ac7
80a5e59 [qa] Attach node index to test_node AssertionError and print messages (James O'Beirne)
Pull request description:
In the midst of fighting with https://github.com/bitcoin/bitcoin/pull/12873 it became apparent that there're a number of assertions and print statements which are emitted by test nodes but don't identify the node in question. This change makes debugging a bit easier by adding identifying information to non-logger test_node-related error messages.
Tree-SHA512: 7cc86f2c81f4b3fdba15ec9a2d21a84c4b083629e845e82288087c3affbbdc5c68e74067621856cc97fe84fbc8cb4f5ca4977a51ef381e5d74515df8eb001239
c25321f Add config changes to release notes (Anthony Towns)
5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns)
005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns)
608415d [tests] Unit tests for network-specific config entries (Anthony Towns)
68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns)
d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns)
8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns)
30f9407 [tests] Unit tests for config file sections (Anthony Towns)
95eb66d ArgsManager: support config file sections (Anthony Towns)
4d34fcc ArgsManager: drop m_negated_args (Anthony Towns)
3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns)
Pull request description:
The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) discussed allowing options to bitcoin to have some sensitivity to what network is in use. @theuni suggested having sections in the config file:
<cfields> an alternative to that would be sections in a config file. and on the
cmdline they'd look like namespaces. so, [testnet] port=5. or -testnet::port=5.
This approach is (more or less) supported by `boost::program_options::detail::config_file_iterator` -- when it sees a `[testnet]` section with `port=5`, it will treat that the same as "testnet.port=5". So `[testnet] port=5` (or `testnet.port=5` without the section header) in bitcoin.conf and `-testnet.port=5` on the command line.
The other aspect to this question is possibly limiting some options so that there is no possibility of accidental cross-contamination across networks. For example, if you're using a particular wallet.dat on mainnet, you may not want to accidentally use the same wallet on testnet and risk reusing keys.
I've set this up so that the `-addnode` and `-wallet` options are `NETWORK_ONLY`, so that if you have a bitcoin.conf:
wallet=/secret/wallet.dat
upnp=1
and you run `bitcoind -testnet` or `bitcoind -regtest`, then the `wallet=` setting will be ignored, and should behave as if your bitcoin.conf had specified:
upnp=1
[main]
wallet=/secret/wallet.dat
For any `NETWORK_ONLY` options, if you're using `-testnet` or `-regtest`, you'll have to add the prefix to any command line options. This was necessary for `multiwallet.py` for instance.
I've left the "default" options as taking precedence over network specific ones, which might be backwards. So if you have:
maxmempool=200
[regtest]
maxmempool=100
your maxmempool will still be 200 on regtest. The advantage of doing it this way is that if you have `[regtest] maxmempool=100` in bitcoin.conf, and then say `bitcoind -regtest -maxmempool=200`, the same result is probably in line with what you expect...
The other thing to note is that I'm using the chain names from `chainparamsbase.cpp` / `ChainNameFromCommandLine`, so the sections are `[main]`, `[test]` and `[regtest]`; not `[mainnet]` or `[testnet]` as might be expected.
Thoughts? Ping @MeshCollider @laanwj @jonasschnelli @morcos
Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db
9db48c5634 tests: Remove redundant bytes² (practicalswift)
Pull request description:
This is a follow-up to #12993. As @jnewbery noted `bytes()` is idempotent.
Tree-SHA512: 0eb25e0c2c46f9abaac30f964c5eb422bece1414c840a717d86794424294cb19d995a6db7c8df2a2f4ec84776b05274a637f2c111738f397051f510e57184752
75d0e4c544 [qa] Delete cookie file before starting node (Suhas Daftuar)
Pull request description:
When a node is restarted during a test after an unclean shutdown (such
as with -dbcrashratio), it's possible an old cookie file was left
behind. This can cause a race condition when restarting the node, where
the test framework might try to connect using credentials from the
old cookie file, just as the node will generate new credentials and
overwrite the old file.
Delete any such cookie file if present prior to startup.
Tree-SHA512: ae1e8bf8fd20e07c32b0715025693bb28b0e3dd34f328cae4346abf579b0c97b5db1c02782e1c46b7a3b6058d268b6d46b668e847658a6eed0be857ffb0d65dc
If a cookie file exists in a datadir prior to node startup, it must have
been leftover from a prior unclean shutdown. As bitcoind will overwrite
it anyway, delete it before starting up to prevent the test framework
from inadvertently trying to connect using stale credentials.
c9cce0a Tests: Add Metaclass for BitcoinTestFramework (Will Ayd)
Pull request description:
BitcoinTestFramework instructs developers in its docstring to override
`set_test_params` and `run_test` in subclasses while being sure NOT to
override `__init__` and `main` . This change adds a metaclass to ensure
that developers adhere to that protocol, raising a ``TypeError`` in
instances where they have not.
closes#12835
Tree-SHA512: 5a47a7ead1f18361138cad4374747c4a8f29d25506f7b2c2a8c1c966a0b65e5ccf7317f9a078df8680fdab5d3fb71fee46a159c9f381878a3683c1e9f874abbe
e36a0c0 [qa] Ensure bitcoind processes are cleaned up when tests end (Suhas Daftuar)
Pull request description:
When tests fail (such as due to a bug in the test, race condition, etc), it's possible that we could follow code paths that bypass our normal node shutdown that occurs in `TestNode.stop_node`. Add a destructor to `TestNode` that cleans this up.
Tree-SHA512: 72e04bc21462ebd0cb346fd1fe0540da454acfbad41923a0b06ea2317e9045b68e58f9adb02d8200891aca89a9d03a022eb72282aeb31a3b3afe7c6843a4b450
self.nodes[0] creates an address which is watch-only-shared with self.nodes[3]. If nodes[0] spends the associated UTXO during any of its sends later, the watchonly test will fail, as nodes[3] now has insufficient funds.
Note that this also adds a new find_vout_for_address function to the test framework.
BitcoinTestFramework instructs developers in its docstring to override
`set_test_params` and `run_test` in subclasses while being sure NOT to
override `__init__` and `main` . This change adds a metaclass to ensure
that developers adhere to that protocol, raising a ``TypeError`` in
instances where they have not.
closes#12835
9c92c8c827 [tests] Remove Comparison Test Framework (John Newbery)
e80c640d78 [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 https://github.com/btcdrak/bitcoin/pull/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
faace13868 qa: Match full plain text by default (MarcoFalke)
Pull request description:
Instead of escaping all full plain text error strings, just compare their strings by default.
Tree-SHA512: 42e28f55105eb947ac6af6ce4056f0ec0f701d85f1c2a38b35ab777bbdf2296bdb79639c345621b8adc03a98b28c7630ded9a67b8b04a48e2c3a49d598ecdcd7
The new P2PDataStore class was sending full blocks in headers messages,
which meant that calls to send_blocks_and_test() would blow up memory if
called with a large number of blocks. Fix that by only sending headers
in headers messages.
b55555d rpc: Add testmempoolaccept (MarcoFalke)
Pull request description:
To check if a single raw transaction makes it into the current transaction pool, one had to call `sendrawtransaction`. However, on success, this adds the transaction to the mempool with no easy way to undo.
The call `testmempoolaccept` is introduced to provide a way to solely check the result without changing the mempool state.
Tree-SHA512: 5afd9311190135cee8fc1f229c7d39bf893f1028f29e28d34f70df820198ff97b4bf86b41cbbd6e6c36a5c30073cefa92d541c74a4939c7a2a6fa283dfd41b63
ea04bf7862 Enable flake8 warning F841 ("local variable 'foo' is assigned to but never used") (practicalswift)
169f3e8637 Remove assigned but never used local variables (practicalswift)
Pull request description:
Remove assigned but never used local variables. Enable Travis checking for unused local variables.
Tree-SHA512: d6052ec9044c5d1f03d874ea3c8addd5a156779213ef9200f89d3ae53230f2fd1691aff405c3dae14178e5ef09912c4432e92f606ef4a5220ed9daa140cdee81
8394300859 [Tests] fix a typo in TestNode.assert_start_raises_init_error() (Roman Zeyde)
Pull request description:
`self.wait_util_stopped()` should be `self.wait_until_stopped()`.
Also, use a specific Exception subclass for indicating node failure to start (instead of using `AssetionError` and an `except Exception` clause).
Following https://travis-ci.org/bitcoin/bitcoin/jobs/359066226#L2726 and depending on #12806 (which fixes the root cause of the Travis test failure).
Tree-SHA512: 7bd5a95586a412472ef9dffdb086789d7275ddaf862724e21cebb3418d0c97e6d89b4d1a58375e42114060d028403d6eab89e3a1e9a833ffe8dadf3439ab1fe2
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
c8330d4 qa: Use node.datadir instead of tmpdir in test framework (MarcoFalke)
Pull request description:
Commit c53c9831ee introduced the utility function `get_datadir_path`, however not all places in the code use this util function. Using the util function everywhere makes it easier to review pull requests related to the datadir.
This commit replaces datadir path creation with the `datadir` member of `TestNode`, which itself uses `get_datadir_path`.
Tree-SHA512: c75707ab7149d732a6d56152a5813138a33459d3d07577b60b89f2a207c83b7663fac5f203593677c9892d1c23a5eba4bd45c5c4ababf040d720b437240fcddf
fae1374 qa: Allow for partial_match when checking init error (MarcoFalke)
5812273 [Tests] Require exact match in assert_start_raises_init_eror() (John Newbery)
0ec08a6 [Tests] Move assert_start_raises_init_error method to TestNode (John Newbery)
Pull request description:
Extracted from #12379, because the changes are important on their own.
This allows for exact testing, since the match can be specified with a strict regex. Internal details (such as exact formatting of the error message) can still be fuzzed away by regex wildcards.
Tree-SHA512: 605d2c9c42362a32d42321b066637577a026d0bb8cfc1c9f5737a4ca6503ffe85457a5122cea6e1101053ccc6c8aa1bbae3602e1fa7d2988bf7d5c275f412f66
4d9b4256d8 Fix typos (Dimitris Apostolou)
Pull request description:
Unfortunately I messed up my repo while trying to squash #12593 so I created a PR with just the correct fixes.
Tree-SHA512: 295d77b51bd2a9381f1802c263de7ffb2edd670d9647391e32f9a414705b3c8b483bb0e469a9b85ab6a70919ea13397fa8dfda2aea7a398b64b187f178fe6a06
Several test scripts define a subclass of P2PInterface called TestNode.
This commit renames those to TestP2PConn since we already have a
TestNode class in the test framework.
-BEGIN VERIFY SCRIPT-
sed -i s/TestNode/TestP2PConn/ test/functional/*py test/functional/test_framework/comptool.py
_END VERIFY SCRIPT-
fa23105 qa: Cache only chain and wallet for regtest datadir (MarcoFalke)
Pull request description:
mempool.dat should be empty and I don't see a need to copy it around when restoring from the cache.
Tree-SHA512: f11ab69732db4dee0e9a0900570464e49085532b0cebc963877057112a7b985c477da3d32eb2093daabac9ada9e73b7c49881681ec5efa6101919b0af76001cf
fa1436c42 [qa] util: Remove unused sync_chain (MarcoFalke)
Pull request description:
The util function `sync_blocks` already checks for equal chains, so we can remove the unused `sync_chain`.
Also cleaned up the errors that are printed in case of timeout:
```
AssertionError: Block sync timed out:
'72a3a3e9dcfd0a09204c3447af0f481d19641eeadbe6a91b8e680ed614bc7712'
'5032af4ae22ae7a21afdc9d9f516877309c4dd8ef2ecadb9354be7088439b4a6'
'5032af4ae22ae7a21afdc9d9f516877309c4dd8ef2ecadb9354be7088439b4a6'
```
and
```
AssertionError: Mempool sync timed out:
{'c2af943d9b321c36e0f5a153a9d3d8b11bdd46ceb28e38f5fd2c722e3edb3563'}
set()
set()
```
Tree-SHA512: cb4ad30e3e3773072f59afa2c81cfa27bd8f389a93f02acb919990298627fcfbaa53a3d3944d827cc8a5d009871e42a47ea09e9bb93e85c22e3af6a24a574e5d
* Z is the zone designator for the zero UTC offset.
* T is the delimiter used to separate date and time.
This makes it clear for the end-user that the date/time logged is
specified in UTC and not in the local time zone.
fa3528a85b qa: Fix some tests to work on native windows (MarcoFalke)
Pull request description:
This allows some more tests to be run natively on Windows
Tree-SHA512: 8097a82dc046be9f6bb0da634758c9afef7836960ca7a1f88f9acab9512dbf7bc26525b515faae407edab4620846cce2b427940298f822e250f23f924b4c7591
b156ff7c3 [tests] bind functional test nodes to 127.0.0.1 (Sjors Provoost)
Pull request description:
Replaces #12200 which broke `rpc_bind.py`.
Prevents OSX firewall allow-this-application-to-accept-inbound-connections permission popups and is generally safer.
To prevent binding to `127.0.0.1`, set `self.bind_to_localhost_only = False`.
cc @jnewbery
Tree-SHA512: 5e700124c91bd0cbdee83ca44910071d71d61d8842334755b685d14fbff6454d75de1ea7de67340370386f58b41361e80e90bb4dca5c4d5992f9d2b27985f999
Prevents OSX firewall allow-this-application-to-accept-inbound-connections
permission popups and is generally safer.
To prevent binding to 127.0.0.1, set self.bind_to_localhost_only = False.
fa41d68a2 qa: Fix python TypeError in script.py (MarcoFalke)
Pull request description:
`__repr__` returns string, so don't mix it with byte strings.
This fixes
```
TypeError: %b requires a bytes-like object, or an object that implements __bytes__, not 'str'
Tree-SHA512: fac06e083f245209bc8a36102217580b0f6186842f4e52a686225111b0b96ff93c301640ff5e7ddef6a5b4f1689071b16a9a8dc80f28e2b060ddee29edd24ec7
eb91835 Add setter for g_initial_block_download_completed (Jonas Schnelli)
3f56df5 [QA] add NODE_NETWORK_LIMITED address relay and sync test (Jonas Schnelli)
158e1a6 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
fa999af [QA] Allow addrman loopback tests (add debug option -addrmantest) (Jonas Schnelli)
6fe57bd Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD (Jonas Schnelli)
31c45a9 Accept addresses with NODE_NETWORK_LIMITED flag (Jonas Schnelli)
Pull request description:
Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD.
Accept and relay NODE_NETWORK_LIMITED peers in addrman.
Tree-SHA512: 8a238fc97f767f81cae1866d6cc061390f23a72af4a711d2f7158c77f876017986abb371d213d1c84019eef7be4ca951e8e6f83fda36769c4e1a1d763f787037
face7220b7 qa: Move common args to bitcoin.conf (MarcoFalke)
Pull request description:
Beside removing duplicates of the same args in the code, this actually helps with debugging after a test failure.
For example, `bitcoin-qt` has `server` turned off, so you'd have to turn it on every time, if you wanted to debug a temporary test datadir created by the test framework.
Also, `keypool` would fill up if you forget to specify `-keypool=1`.
Tree-SHA512: 996bec8738dc0fce7297ab1fc5b4fbe3aa31b9b6241f8c4e685db728925363ccaca6145ad1edc6bee2f360e02ac4b9a53fcdff74eb79de90793393742d52b559
42e1b5d [test] Round target fee to 8 decimals in assert_fee_amount (Karl-Johan Alm)
Pull request description:
The output would produce arbitrary number of decimal points, sometimes resulting in 9 decimals:
```
AssertionError: Fee of 0.00000415 BTC too low! (Should be 0.000006175 BTC)
```
The above looks like the expected fee is 6175 sats when in reality it's 618.
Tree-SHA512: ddbff2926a88890d6e34a58db36f0b15a917a80064be6e40e9bcbec3f05ae6202d02adcd7873733945b043fa121d4a56dd930446ec368078fe1935cbfff898ce
The output would produce arbitrary number of decimal points, sometimes resulting in 9 decimals:
AssertionError: Fee of 0.00000415 BTC too low! (Should be 0.000006175 BTC)
The above looks like the expected fee is 6175 sats when in reality it's 618.
Splits signrwatransaction into a wallet version (signrawtransactionwithwallet) and
non-wallet version (signrawtransactionwithkey). signrawtransaction is marked as DEPRECATED
and will call the right signrawtransaction* command as per the parameters in order to
maintain compatibility.
Updated signrawtransactions test to use new RPCs
b22cce014 scripted-diff: validateaddress to getaddressinfo in tests (Andrew Chow)
b98bfc5ed Create getaddressinfo RPC and deprecate parts of validateaddress (Andrew Chow)
1598f3230 [rpc] Move DescribeAddressVisitor to rpc/util (John Newbery)
39633ecd5 [rpc] split wallet and non-wallet parts of DescribeAddressVisitor (John Newbery)
Pull request description:
This PR makes a new RPC command called `getaddressinfo` which relies on the wallet. It contains all of `validateaddress`'s address info stuff. Those parts in `validateaddress` have been marked as deprecated. The tests have been updated to use `getaddressinfo` except the `disablewallet` test which is the only test that actually uses `validateaddress` to validate an address.
Tree-SHA512: ce00ed0f2416200b8de1e0a75e8517c024be0b6153457d302c3879b3491cce28191e7c29aed08ec7d2eeeadc62918f5c43a7cb79cd2e4b6d9291bd83ec31c852
Change all instances of validateaddress to getaddressinfo since it seems that
no test actually uses validateaddress for actually validating addresses.
-BEGIN VERIFY SCRIPT-
find ./test/functional -path '*py' -not -path ./test/functional/wallet_disable.py -not -path ./test/functional/rpc_deprecated.py -not -path ./test/functional/wallet_address_types.py -exec sed -i'' -e 's/validateaddress/getaddressinfo/g' {} \;
-END VERIFY SCRIPT-
65682da [tests] bind functional test nodes to 127.0.0.1 (Sjors Provoost)
Pull request description:
Prevents OSX firewall allow-this-application-to-accept-inbound-connections permission popups and is generally safer.
To test, make an arbitrary whitespace change to `src/bitcoind.cpp` and recompile. This normally resets the firewall's memory.
Easiest way to reproduce a popup without running the test suite:
```sh
src/bitcoind -regtest -bind=127.0.0.1 # No popup
src/bitcoind -regtest # Popup
```
Tree-SHA512: ffa92f148a2ead2ceca978c285882979639be23eb31ad6a27aa81df9fdddba5d493719c92c09a351a81d638f6f739c351a721e42168d77ead60abe074df773d6
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 #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
faefd29 qa: Prepare functional tests for Windows (MarcoFalke)
Pull request description:
* Pass `sys.executable` when calling a python script via the subprocess
module
* Don't remove the log file while it is still open and written to
* Properly use os.pathsep and os.path.sep when modifying the PATH
environment variable
* util-tests: Use os.path.join for Windows compatibility
Ref: #8227
Tree-SHA512: c507a536af104b3bde4366b6634099db826532bd3e7c35d694b5883c550920643b3eab79c76703ca67e1044ed09979e855088f7324321c8d52112514e334d614
* Pass `sys.executable` when calling a python script via the subprocess
module
* Don't remove the log file while it is still open and written to
* Properly use os.pathsep and os.path.sep when modifying the PATH
environment variable
* util-tests: Use os.path.join for Windows compatibility
fae7b14a04 qa: Make TestNodeCLI command optional in send_cli (MarcoFalke)
ffffb10a9f qa: Rename cli.args to cli.options (MarcoFalke)
Pull request description:
Makes the `command` optional, since there are valid bitcoin-cli calls that have no `command`:
* `bitcoin-cli -?`
* `bitcoin-cli -getinfo`
* ...
Also, rename self.args to self.options, since that is the name in the `bitcoin-cli -help` documentation.
Tree-SHA512: f49c06024e78423301d70782946d47c0fb97a26876afba0a1f71ed329f5d7124aee4c2df520c7af74079bf9937851902f7be9c54abecc28dc29274584804d46c
Also warn if bitcoind is configured to use a relative -datadir path.
Specifying paths relative to the current working directory in a daemon process
can be dangerous, because files can fail to be located even if the
configuration doesn't change, but the daemon is started up differently.
Specifying a relative -datadir now adds a warning to the debug log. It would
not be backwards-compatible to forbid relative -datadir paths entirely, and it
could also be also inconvenient for command line testing.
Specifying a relative -walletdir now results in a startup error. But since the
-walletdir option is new in 0.16.0, there should be no compatibility issues.
Another reason not to use working directory paths for -walletdir specifically
is that the default -walletdir is a "wallets" subdirectory inside the datadir,
so it could be surprising that setting -walletdir manually would choose a
directory rooted in a completely different location.
5fecd84 [tests] Remove redundant import in blocktools.py test (Anthony Towns)
9b20bb4 [tests] Check tests conform to naming convention (Anthony Towns)
7250b4e [tests] README.md nit fixes (Anthony Towns)
82b2712 [tests] move witness util functions to blocktools.py (John Newbery)
1e10854 [tests] [docs] update README for new test naming scheme (John Newbery)
Pull request description:
Splitting #11774 into two parts -- this part updates the README with the proposed naming convention, and adds some checks to test_runner.py that the number of tests violating the naming convention doesn't increase too much. Idea is this part of the change should not introduce merge conflicts or require much rebasing, so reviews of the complicated bits won't become invalidated too often; while the second part will just be file renames, which will require regular rebasing and will introduce merge conflicts with pending PRs, but can be merged later, and should also be much easier to review, since it will only include relatively trivial changes.
Tree-SHA512: b96557d41714addbbfe2aed62fb5a48639eaeb1eb3aba30ac1b3a86bb3cb8d796c6247f9c414c4695c4bf54c0ec9968ac88e2f88fb62483bc1a2f89368f7fc80
a14dbff39e Allow multiwallet.py to be used with --usecli (Russell Yanofsky)
f6ade9ce1a [tests] allow tests to be run with --usecli (John Newbery)
ff9a363ff7 TestNodeCLI batch emulation (Russell Yanofsky)
ca9085afc5 Prevent TestNodeCLI.args mixups (Russell Yanofsky)
fcfb952bca Improve TestNodeCLI output parsing (Russell Yanofsky)
Pull request description:
Lack of test coverage was pointed out by @jnewbery in https://github.com/bitcoin/bitcoin/pull/11687#discussion_r158133900
Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02
b224a47a1 Add address_types test (Pieter Wuille)
7ee54fd7c Support downgrading after recovered keypool witness keys (Pieter Wuille)
940a21932 SegWit wallet support (Pieter Wuille)
f37c64e47 Implicitly know about P2WPKH redeemscripts (Pieter Wuille)
57273f2b3 [test] Serialize CTransaction with witness by default (Pieter Wuille)
cf2c0b6f5 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille)
37c03d3e0 Support P2WPKH addresses in create/addmultisig (Pieter Wuille)
3eaa003c8 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille)
30a27dc5b Expose method to find key for a single-key destination (Pieter Wuille)
985c79552 Improve witness destination types and use them more (Pieter Wuille)
cbe197470 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille)
0c8ea6380 Abstract out IsSolvable from Witnessifier (Pieter Wuille)
Pull request description:
This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089.
Two new configuration options are added:
* `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`.
* `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used.
All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version.
The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key.
To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:
* All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date.
* All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software.
* All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work.
These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented.
`dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now.
Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959
test_framework accepts a new --usecli parameter. Running the test with
this parameter will cause all RPCs to be sent through bitcoin-cli rather
than directly over http. By default, individual test cases do not
support --usecli, and self.supports_cli must be set to True in the
set_test_params method.
We can make supports_cli default to True in future once we know which
tests will fail with use_cli.
Change TestNodeCLI.__call__() to return a new instance instead of modifying the
existing instance. This way, it's possible to create different cli objects that
have their own options (for example -rpcwallet options to connect to different
wallets), and options set for a single call (`node.cli(options).method(args)`)
will no longer leak into future calls.
Add network thread_start(), network_thread_running() and
network_thread_join() utility functions in mininode.py and use
network_thread_running() in network thread assertions.
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
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.
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
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
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
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
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.
p2p connections can now be added to TestNode instances.
This commit also updates the example test to use the new
p2p interface in TestNode to demonstrate usage.
A future commit will update the existing tests to use p2p through the
TestNode.
fafa003 qa: Remove never used return value of sync_with_ping (MarcoFalke)
fa9de37 qa: Make tmpdir option an absolute path (MarcoFalke)
Pull request description:
This should fix issues with the multiwallet test and its symlinks
when the tmpdir is a relative path.
Rather than fixing os.symlink to work with paths relative to a
directory descriptor, which does not work on Windows, normalize
the path instead.
Tree-SHA512: 189690f3d065ea2f0f48e06775c86d513d0916c7c86312432e8e16df160e65539e288c2bd53d49a4180735fa940f6fcd52b506ccd7d9815651a9b1a69850dda6
