3a7e79478a test: retry when write to a socket fails on macOS (Ivan Metlushko)
8cf9d15b82 test: use pgrep for better compatibility (Ivan Metlushko)
Pull request description:
Rationale: a few minor changes to make experience of running tests on macOS a bit better
1.`pidof` is not available on BSD/macOS, while `pgrep` is present on BSD, Linux and macOS
2. Add retry as a workaround for a weird behavior when writing to a socket (https://bugs.python.org/issue33450). Stacktrace attached
Man pages:
https://www.freebsd.org/cgi/man.cgi?query=pgrep&apropos=0&sektion=1&manpath=FreeBSD+6.0-RELEASE&arch=default&format=htmlhttps://man7.org/linux/man-pages/man1/pgrep.1.html
Related to #19281
Stacktrace example:
```
...
33/161 - feature_abortnode.py failed, Duration: 63 s
stdout:
2020-06-11T10:46:43.947000Z TestFramework (INFO): Initializing test directory /var/folders/2q/d5w9zh614r7g5c8r74ln3g400000gq/T/test_runner_₿_🏃_20200611_174102/feature_abortnode_128
2020-06-11T10:46:45.199000Z TestFramework (INFO): Waiting for crash
2020-06-11T10:47:15.921000Z TestFramework (INFO): Node crashed - now verifying restart fails
2020-06-11T10:47:47.068000Z TestFramework (INFO): Stopping nodes
[node 1] Cleaning up leftover process
stderr:
Traceback (most recent call last):
File "/Users/xxx/Projects/bitcoin/test/functional/feature_abortnode.py", line 50, in <module>
AbortNodeTest().main()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
exit_code = self.shutdown()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 266, in shutdown
self.stop_nodes()
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 515, in stop_nodes
node.stop_node(wait=wait)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_node.py", line 318, in stop_node
self.stop(wait=wait)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
self.__conn.request(method, path, postdata, headers)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1107, in request
self._send_request(method, url, body, headers)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1152, in _send_request
self.endheaders(body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1103, in endheaders
self._send_output(message_body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 936, in _send_output
self.send(message_body)
File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 908, in send
self.sock.sendall(data)
OSError: [Errno 41] Protocol wrong type for socket
```
ACKs for top commit:
laanwj:
ACK 3a7e79478a
Tree-SHA512: fefbe40ce94ab29f18bbbed2a434194b1384ffa5279b1d04db7a3708e3dd422bd9e450f1db3f95a1a851fac5a626ab533c6ebcfd7ede96f8ccae9e6f3e9fff92
If the socket is tearing down macOS will return EPROTOTYPE instead of EPIPE.
Because python doesn't handle this internally we have to do a workaround and retry the request.
See https://bugs.python.org/issue33450
56010f9256 test: hoist p2p values to test framework constants (Jon Atack)
75447f0893 test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack)
57960192a5 test: refactor test_large_inv() into 3 tests with common method (Jon Atack)
e2b21d8a59 test: add p2p_invalid_messages logging (Jon Atack)
9fa494dc09 net: update misbehavior logging for oversized messages (Jon Atack)
Pull request description:
...seen while reviewing #19264, #19252, #19304 and #19107:
in `net_processing.cpp`
- make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages
in `p2p_invalid_messages`
- add missing logging
- improve assertions/message sends, move cleanup disconnections outside the assertion scopes
- split a slowish 3-part test into 3 order-independent tests
- add a few p2p constants to the test framework
ACKs for top commit:
troygiorshev:
reACK 56010f9256
MarcoFalke:
ACK 56010f9256 🎛
Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
cc84460c16 test: move sync_blocks and sync_mempool functions to test_framework.py (Roy Shao)
Pull request description:
This PR moves `sync_blocks` and `sync_mempool` out from `test_framework/util.py` to `test_framework/test_framework.py` so they can take contextual information of test framework into account.
* Change all reference callers to call functions from `test_framework.py`
* Remove `**kwargs` which is not used
* Take into account of `timeout_factor` when respecting timeout in function implementations.
* Pass all tests by running `./test/functional/test_runner.py`
fixes#18930
ACKs for top commit:
MarcoFalke:
ACK cc84460c16 , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫
Tree-SHA512: a79b2a3fa842fc26a7aacb834bb2aea88b3049916c0b754e60002a77ce94bb5954e0ea3b436bf268e9295efb62d721dfef263a09339a55c684ac3fda388c275e
9a40cfc558 [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb414e [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d2e1 [test] logging and style followups for bloomfilter tests (gzhao408)
Pull request description:
Followup to #19083 which adds bloomfilter-related tests.
1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/19083#discussion_r437383989). And clean up any redundant `wait_until`s in the functional tests.
2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](https://github.com/bitcoin/bitcoin/pull/19083#pullrequestreview-428955784)
ACKs for top commit:
jonatack:
Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
MarcoFalke:
ACK 9a40cfc558🐂
Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
-Waiting is important to avoid race conditions,
especially if testing peer info through rpc later.
-Wait for mininodes to be disconnected only, even
though it's more complex, because we may still want
to be connected to test nodes.
62068381a3 [tests] Make mininode_lock non-reentrant (John Newbery)
c67c1f2c03 [tests] Don't call super twice in P2PTxInvStore.on_inv() (John Newbery)
9d80762fa0 [tests] Don't acquire mininode_lock twice in wait_for_broadcast() (John Newbery)
edae6075aa [tests] Only acquire lock once in p2p_compactblocks.py (John Newbery)
Pull request description:
There's no need for mininode_lock to be reentrant.
Use a simpler non-recursive lock.
ACKs for top commit:
MarcoFalke:
ACK 62068381a3😃
jonatack:
ACK 62068381a3
Tree-SHA512: dcbc19e6c986970051705789be0ff7bec70c69cf76d5b468c2ba4cb732883ad512b1de5c3206c2eca41fa3f1c4806999df4cabbf67fc3c463bb817458e59a19c
34e641a564 test: Remove unnecessary disconnect_nodes call in rpc_psbt.py (Danny Lee)
e6e7abd51a test: remove redundant two-way disconnect_nodes calls (Danny Lee)
a9bd1f9adf test: warn if nodes not connected before disconnect_nodes (Danny Lee)
Pull request description:
There's no harm in calling `disconnect_nodes` for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.
In addition, since `disconnect_nodes` works bidirectionally, I removed all instances of this pattern:
```
disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)
```
ACKs for top commit:
MarcoFalke:
review ACK 34e641a564👔
amitiuttarwar:
ACK 34e641a564. Thanks for this test improvement!
Tree-SHA512: 344855ceb46c012d43c13d7c09f44d32dcb7645706d10ae1e4645d9edca54c6c6c13fee26b79480755cdfcdf39b4b5770b36bb03ce71ba002d5be8a27fe008af
7daffc6a90 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
Migrates the CScriptNum decode tests into a unit test, and moved some
changes made in #14816. Made possible by the integration of
test_framework unit testing in #18576. Further extends the original
test with larger ints, similar to the scriptnum_tests.cpp file. Adds
test to blocktools.py testing fn create_coinbase() with CScriptNum
decode.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
Mypy is used in lint-python.sh to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
Useful resources:
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
9e36067d8c [test] Add test for cfilters. (Jim Posen)
11106a4722 [net processing] Message handling for getcfilters. (Jim Posen)
e535670726 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5 [refactor] Pass CNode and CConnman by reference (John Newbery)
Pull request description:
Support `getcfilters` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
Empact:
re-Code Review ACK 9e36067d8c
MarcoFalke:
re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
jkczyz:
ACK 9e36067d8c
fjahr:
Code review ACK 9e36067d8c
Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887
60ed33904c tests: implement base58_decode (10xcryptodev)
Pull request description:
implements TODO: def base58_decode
ACKs for top commit:
ryanofsky:
Code review ACK 60ed33904c. Just suggested changes since last review. Thank you for taking suggestions!
Tree-SHA512: b3c06b4df041a6d88033cd077a093813a688e42d0b9aa777c715e5fd69cfba7b1bf984428bd98417d3c15232d3d48bc9c163317564f9e1d562db6611c21e2c10
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
fa80b4788b test: Remove global wait_until from p2p_getdata (MarcoFalke)
999922baed test: Default mininode.wait_until timeout to 60s (MarcoFalke)
fab47375fe test: pep-8 p2p_getdata.py (MarcoFalke)
Pull request description:
Using the global wait_until makes it impossible to adjust the timeout based on the hardware the test is running on.
Fix that by using the mininode member function.
So for example, `./test/functional/p2p_getdata.py --timeout-factor=0.04` gives a timeout of 2.4 seconds.
ACKs for top commit:
laanwj:
ACK fa80b4788b
Tree-SHA512: ebb1b7860a64451de2b8ee9a0966faddb13b84af711f6744e8260d7c9bc0b382e8fb259897df5212190821e850ed30d4d5c2d7af45a97f207fd4511b06b6674a
5308c97cca [test] Add test for cfheaders (Jim Posen)
f6b58c1506 [net processing] Message handling for getcfheaders. (Jim Posen)
3bdc7c2d39 [doc] Add comment for m_headers_cache (John Newbery)
Pull request description:
Support `getcfheaders` requests when `-peerblockfilters` is set.
Does not advertise compact filter support in version messages.
ACKs for top commit:
jkczyz:
ACK 5308c97cca
MarcoFalke:
re-ACK 5308c97cca , only change is doc related 🗂
theStack:
ACK 5308c97cca🚀
Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7
fad798be76 test: Default --previous-releases to false if dir is empty (MarcoFalke)
faf1c3cc58 test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option (MarcoFalke)
Pull request description:
The "auto-detection" feature is kept in place, but making it an option allows to properly document it. For example, on my machine I get:
```
$ ./test/functional/wallet_disable.py --help | grep previous-releases
--previous-releases Force test of previous releases (default: False)
ACKs for top commit:
Sjors:
re-tACK fad798b
Tree-SHA512: a7377d0d5378be0a50be278d76396cc403583617b5fc43467773eee706df698acf3f4e67651491183b9b43a8e1816b052e4c17b90272b7ec4b6ac134ad811400
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f
amitiuttarwar:
ACK 651f1d816f🎉
MarcoFalke:
Review ACK 651f1d816f
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
4a614ff88a test: explicit imports from test_framework.messages in p2p_invalid_messages.py (Sebastian Falbesoner)
b35e1d2471 test: add inventory type constant MSG_CMPCT_BLOCK (Sebastian Falbesoner)
eeaaa58d2c test: replace inv type magic numbers by constants (Sebastian Falbesoner)
Pull request description:
Many functional tests still use magic numbers for inventory types, either passed to the `CInv` constructor or for comparing the `type` member of `CInv`. This PR replaces all of those by constants in the module `test_framework.messages` that have been introduced in commit c32cf9f622: `MSG_TX` (1) or `MSG_BLOCK` (2).
It also introduces a new constant `MSG_CMPCT_BLOCK` (naming as in `src/protocol.h`) and uses it to replace the remaining magic numbers.
The occurences of the magic numbers were identified through `grep`ing for `CInv(` and `type ==`. The idea was first to create a scripted-diff, but since also adding missing `import`s is needed, this would be non-trivial. Besides, also some unneeded comments like `# 2 == "Block"` could be removed.
ACKs for top commit:
gzhao408:
ACK [`4a614ff`](4a614ff88a)
Tree-SHA512: 4ba4fdef9f3eef7fd5ac72cb03ca3524863d1ae292161c550424a4c1047283fa2d2e7e03017d1fbae3652b3cb14f08b8d4b368403f3f209993aef3f2e2b22784
- mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata),
so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete
('unbroadcast' = False) otherwise the state may change in between calls
- update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list
of txids to complete initial broadcast
- make mempool_packages.py wait because it compares entries using getrawmempool and
getmempoolentry
Modifies the existing --factor flag to --timeout-factor to better express intent.
Adds rules to disable timeout if --timeout-factor is set to 0.
Modfies --timeout-factor help doc to inform users about this feature.
8a22fd0114 avoided os-dependant path (Ferdinando M. Ametrano)
Pull request description:
The current code fails on windows because of the forward slashes; using os.path.join solves the problem and it is in general more robust
ACKs for top commit:
MarcoFalke:
ACK 8a22fd0114
Tree-SHA512: 813f27aea33f97c8afac52e716a55fc5d7fb69621023aba99d40df7e1d145e0ec8d1eee49ddd403b219bf0e0e168e0e987b05c78eaef611f744d99bf2fc8bc91
faa26d3744 test: Remove RPCOverloadWrapper boilerplate (MarcoFalke)
Pull request description:
There are too many wrappers in test_node already, so at least the code that implements the wrappers should be as minimal as possible.
ACKs for top commit:
laanwj:
code review ACK faa26d3744
Tree-SHA512: 94e593907de22187524e2445afb3101e40b3b599d4b4015aa8c6ca902d7586ff9daf520828759029d199a3af79e61b96b490a822a5a193ac7bf946beacb11a24
23083856a5 [test] Add test for cfcheckpt (Jim Posen)
f9e00bb25a [net processing] Message handling for getcfcheckpt. (Jim Posen)
9ccaaba11e [init] Add -peerblockfilters option (Jim Posen)
Pull request description:
Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set.
`NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients.
ACKs for top commit:
jonatack:
Code review re-ACK 23083856a5 the only change since my review @ 967e2b1 is an update required for #16224 that was merged yesterday.
fjahr:
re-ACK 23083856a5
jkczyz:
re-ACK 23083856a5
ariard:
re-Code Review ACK 2308385
clarkmoody:
Tested ACK 23083856a
MarcoFalke:
re-ACK 23083856a5🌳
theStack:
ACK 23083856a5
Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
2742c34286 test: add factor option to adjust test timeouts (Harris)
Pull request description:
This PR adds a new option **factor** that can be used to adjust timeouts in various functional tests.
Several timeouts and functions from `authproxy`, `mininode`, `test_node` and `util` have been adapted to use this option. The factor-option definition is located in `test_framework.py`.
Fixes https://github.com/bitcoin/bitcoin/issues/18266
Also Fixes https://github.com/bitcoin/bitcoin/issues/18834
ACKs for top commit:
MarcoFalke:
Thanks! ACK 2742c34286
Tree-SHA512: 6d8421933ba2ac1b7db00b70cf2bc242d9842c48121c11aadc30b0985c4a174c86a127d6402d0cd73b993559d60d4f747872d21f9510cf4806e008349780d3ef
de8905adf2 test: use unittest and test_runner for test framework unit testing (Gloria Zhao)
Pull request description:
Proposal for unit testing on test_framework functions:
1. Use the python `unittest` library. Don't use test_framework to test itself.
2. Put the tests inside the same file as the functions they are testing.
3. Call the tests from `test_runner.py`. To include more Test Framework tests, add the filename to the list `TEST_FRAMEWORK_MODULES`. Don't add new files or change the list of accepted script prefixes.
Makes these changes for `bn2vch` (followup to [this comment](https://github.com/bitcoin/bitcoin/pull/18378#pullrequestreview-377271264)).
ACKs for top commit:
jnewbery:
Tested ACK de8905adf2. Great stuff gzhao408 . Thanks for this!
Tree-SHA512: 91572d43e203a1864765b93a9472667994115ec38b271f2b2f9fcd0f0112b393fc24ba7d2371d5a34b0a6a4522f6b934fc5164363819aa7ed8d6c6c9a60cc101
66fe7b1a98 test: added test for upgradewallet RPC (Harris)
Pull request description:
This PR adds tests for the newly merged *upgradewallet* RPC.
Additionally, it expands `test_framework/util.py` by adding the function `adjust_bitcoin_conf_for_pre_17` to support nodes that don't parse configuration sections.
This test uses two older node versions, v0.15.2 and v0.16.3, to create older wallet versions to be used by `upgradewallet`.
Fixes https://github.com/bitcoin/bitcoin/issues/18767
Top commit has no ACKs.
Tree-SHA512: bb72ff1e829e2c3954386cc308842820ef0828a4fbb754202b225a8748f92d4dcc5ec77fb146bfd5484a5c2f29ce95adf9f3fb4483437088ff3ea4a8d2c442c1
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
223588b1bb Add a --descriptors option to various tests (Andrew Chow)
869f7ab30a tests: Add RPCOverloadWrapper which overloads some disabled RPCs (Andrew Chow)
cf06062859 Correctly check for default wallet (Andrew Chow)
886e0d75f5 Implement CWallet::IsSpentKey for non-LegacySPKMans (Andrew Chow)
3c19fdd2a2 Return error when no ScriptPubKeyMan is available for specified type (Andrew Chow)
388ba94231 Change wallet_encryption.py to use signmessage instead of dumpprivkey (Andrew Chow)
1346e14831 Functional tests for descriptor wallets (Andrew Chow)
f193ea889d add importdescriptors RPC and tests for native descriptor wallets (Hugo Nguyen)
ce24a94494 Add IsLegacy to CWallet so that the GUI knows whether to show watchonly (Andrew Chow)
1cb42b22b1 Generate new descriptors when encrypting (Andrew Chow)
82ae02b165 Be able to create new wallets with DescriptorScriptPubKeyMans as backing (Andrew Chow)
b713baa75a Implement GetMetadata in DescriptorScriptPubKeyMan (Andrew Chow)
8b9603bd0b Change GetMetadata to use unique_ptr<CKeyMetadata> (Andrew Chow)
72a9540df9 Implement FillPSBT in DescriptorScriptPubKeyMan (Andrew Chow)
84b4978c02 Implement SignMessage for descriptor wallets (Andrew Chow)
bde7c9fa38 Implement SignTransaction in DescriptorScriptPubKeyMan (Andrew Chow)
d50c8ddd41 Implement GetSolvingProvider for DescriptorScriptPubKeyMan (Andrew Chow)
f1ca5feb4a Implement GetKeypoolOldestTime and only display it if greater than 0 (Andrew Chow)
586b57a9a6 Implement ReturnDestination in DescriptorScriptPubKeyMan (Andrew Chow)
f866957979 Implement GetReservedDestination in DescriptorScriptPubKeyMan (Andrew Chow)
a775f7c7fd Implement Unlock and Encrypt in DescriptorScriptPubKeyMan (Andrew Chow)
bfdd073486 Implement GetNewDestination for DescriptorScriptPubKeyMan (Andrew Chow)
58c7651821 Implement TopUp in DescriptorScriptPubKeyMan (Andrew Chow)
e014886a34 Implement SetupGeneration for DescriptorScriptPubKeyMan (Andrew Chow)
46dfb99768 Implement writing descriptorkeys, descriptorckeys, and descriptors to wallet file (Andrew Chow)
4cb9b69be0 Implement several simple functions in DescriptorScriptPubKeyMan (Andrew Chow)
d1ec3e4f19 Add IsSingleType to Descriptors (Andrew Chow)
953feb3d27 Implement loading of keys for DescriptorScriptPubKeyMan (Andrew Chow)
2363e9fcaa Load the descriptor cache from the wallet file (Andrew Chow)
46c46aebb7 Implement GetID for DescriptorScriptPubKeyMan (Andrew Chow)
ec2f9e1178 Implement IsHDEnabled in DescriptorScriptPubKeyMan (Andrew Chow)
741122d4c1 Implement MarkUnusedAddresses in DescriptorScriptPubKeyMan (Andrew Chow)
2db7ca765c Implement IsMine for DescriptorScriptPubKeyMan (Andrew Chow)
db7177af8c Add LoadDescriptorScriptPubKeyMan and SetActiveScriptPubKeyMan to CWallet (Andrew Chow)
78f8a92910 Implement SetType in DescriptorScriptPubKeyMan (Andrew Chow)
834de0300c Store WalletDescriptor in DescriptorScriptPubKeyMan (Andrew Chow)
d8132669e1 Add a lock cs_desc_man for DescriptorScriptPubKeyMan (Andrew Chow)
3194a7f88a Introduce WalletDescriptor class (Andrew Chow)
6b13cd3fa8 Create LegacyScriptPubKeyMan when not a descriptor wallet (Andrew Chow)
aeac157c9d Return nullptr from GetLegacyScriptPubKeyMan if descriptor wallet (Andrew Chow)
96accc73f0 Add WALLET_FLAG_DESCRIPTORS (Andrew Chow)
6b8119af53 Introduce DescriptorScriptPubKeyMan as a dummy class (Andrew Chow)
06620302c7 Introduce SetType function to tell ScriptPubKeyMans the type and internal-ness of it (Andrew Chow)
Pull request description:
Introducing the wallet of the glorious future (again): native descriptor wallets. With native descriptor wallets, addresses are generated from descriptors. Instead of generating keys and deriving addresses from keys, addresses come from the scriptPubKeys produced by a descriptor. Native descriptor wallets will be optional for now and can only be created by using `createwallet`.
Descriptor wallets will store descriptors, master keys from the descriptor, and descriptor cache entries. Keys are derived from descriptors on the fly. In order to allow choosing different address types, 6 descriptors are needed for normal use. There is a pair of primary and change descriptors for each of the 3 address types. With the default keypool size of 1000, each descriptor has 1000 scriptPubKeys and descriptor cache entries pregenerated. This has a side effect of making wallets large since 6000 pubkeys are written to the wallet by default, instead of the current 2000. scriptPubKeys are kept only in memory and are generated every time a descriptor is loaded. By default, we use the standard BIP 44, 49, 84 derivation paths with an external and internal derivation chain for each.
Descriptors can also be imported with a new `importdescriptors` RPC.
Native descriptor wallets use the `ScriptPubKeyMan` interface introduced in #16341 to add a `DescriptorScriptPubKeyMan`. This defines a different IsMine which uses the simpler model of "does this scriptPubKey exist in this wallet". Furthermore, `DescriptorScriptPubKeyMan` does not have watchonly, so with native descriptor wallets, it is not possible to have a wallet with both watchonly and non-watchonly things. Rather a wallet with `disable_private_keys` needs to be used for watchonly things.
A `--descriptor` option was added to some tests (`wallet_basic.py`, `wallet_encryption.py`, `wallet_keypool.py`, `wallet_keypool_topup.py`, and `wallet_labels.py`) to allow for these tests to use descriptor wallets. Additionally, several RPCs are disabled for descriptor wallets (`importprivkey`, `importpubkey`, `importaddress`, `importmulti`, `addmultisigaddress`, `dumpprivkey`, `dumpwallet`, `importwallet`, and `sethdseed`).
ACKs for top commit:
Sjors:
utACK 223588b1bb (rebased, nits addressed)
jonatack:
Code review re-ACK 223588b1bb.
fjahr:
re-ACK 223588b1bb
instagibbs:
light re-ACK 223588b
meshcollider:
Code review ACK 223588b1bb
Tree-SHA512: 59bc52aeddbb769ed5f420d5d240d8137847ac821b588eb616b34461253510c1717d6a70bab8765631738747336ae06f45ba39603ccd17f483843e5ed9a90986
Test the test_framework, but don't use test_framework objects or functions to test itself
Use python unittest library and put test_framework's unit tests inside their respective files
Add the filename to TEST_FRAMEWORK_MODULES in test_runner
Aggregate all test_framework tests into one TestSuite to run before the functional tests in test_runner
Delete framework_test_script, move test_bn2vch to script.py and add to TEST_FRAMEWORK_MODULES in test_runner
as defined in PEP 3135:
"The new syntax:
super()
is equivalent to:
super(__class__, <firstarg>)
where __class__ is the class that the method was defined in, and <firstarg> is
the first parameter of the method (normally self for instance methods, and cls
for class methods)."
8f5dc8800a test: display command line options passed to send_cli() in debug log (Jon Atack)
Pull request description:
as per https://github.com/bitcoin/bitcoin/pull/18691#discussion_r411382589, and revert two cli calls changed in #18691 from rpc commands back to command line options (these were the only occurrences).
ACKs for top commit:
MarcoFalke:
ACK 8f5dc8800a
Tree-SHA512: fcb3eca00aa4099066028c90d5e50a02e074366e09a17f5f5b937d9f7562dd054ff65681aa0ad4c94f6de1e98b1e2b9ac4cd084ddc297010253989a80483b1b9
Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.
Some tests are modified to work with --descriptors and run with that
option in test_runer:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py
* wallet_keypool_topup.py
* wallet_labels.py
* wallet_avoidreuse.py
RPCOverloadWrapper overloads some deprecated or disabled RPCs with
an implementation using other RPCs to avoid having a ton of code churn
around replacing those RPCs.
9f5608c289 test: check for matching object hashes in wait_for_getdata (Danny Lee)
Pull request description:
Previously, `wait_for_getdata` only looked for the presence of a recent `"getdata"` message. Additionally checking the object hashes inside the message should make tests involving `wait_for_getdata` more robust.
`p2p_sendheaders.py` already overrides `wait_for_getdata` do this check; we can use the same approach consistently across all tests that call `wait_for_getdata`.
This PR is progress towards #18614 , but closing that issue would also involve some additional changes to `wait_for_getheaders`.
ACKs for top commit:
theStack:
ACK 9f5608c289🍻
Tree-SHA512: 8e7f95881c19631db014d4bb2399fea0d14686a32542f6ca3b60809744b0d684eac4e4c107c87143991f3cd0c2d4ab09d0c17486239768a9b40bee25f2e4d54a
fac2fc4dd8 test: Increase debugging to hunt down mempool_reorg intermittent failure (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 4094b44afaa623e58b69f8d0332e60f0150b9ae2fd8bb265210d85546d887672ab8a3435cd9b086be14f69ab5b17e0f9fae06bd8aec1e7947ca766dd72b577c4
9df32e820d scripted-diff: test: replace command with msgtype (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to https://github.com/bitcoin/bitcoin/pull/18533, which changed the naming of `strCommand` to `msg_type` in the network processing code. The same approach is done here for the function test framework, to get rid of the wrong "command" terminology for network mesage types. (Commands are usually used in the CLI or RPC context, so using the same name in the network message context would only be confusing.)
The commit was created through the following steps:
1. search for all occurences of the string "command" within the folder `test/functional`
```git grep -i command test/functional > command_finds```
2. manually sort out all false-positives, i.e. occurences of "command" which describe commands in the correct sense (mostly CLI or RPC related, also some with Socks5)
3. put the remaining occurences into a scripted-diff (a quite simple one, actually) that renames "command" to "msgtype" in the concerned files.
The name `msgtype` was intentionally chosen without the underscore `_` as classes beginning with `msg_` define concrete types of messages.
ACKs for top commit:
MarcoFalke:
ACK 9df32e820d . Makes sense that tests use the same naming as Bitcoin Core. See `NetMsgType` here: https://doxygen.bitcoincore.org/namespace_net_msg_type.html
Tree-SHA512: cd0ee08a382910b7f10ce583acdaf4f8a39f9ba4a22434a914415727eedd98bac538de9bf6633574d5eb86f62558bc8dcb638a3289d99b04f8481f34e7a9a0c7
fa03713e13 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) (MarcoFalke)
Pull request description:
actually (?) fix#18561
See most recent traceback https://travis-ci.org/github/bitcoin/bitcoin/jobs/674668692#L7062
I believe the reason the error is still there is that ConnectionResetError is derived from OSError:
ConnectionResetError(ConnectionError(OSError))
And IOError is an alias for OSError since python 3.3, see https://docs.python.org/3/library/exceptions.html#IOError
So fix that by renaming IOError to the alias OSError and move the less specific catch clause down a few lines.
ACKs for top commit:
jonatack:
ACK fa03713e13
Tree-SHA512: 6e5b214ed9101bf8ebe7472dcc1f9e9d128e2575c93ec00c8d0774ae1a9b52a8c2a653a45a0eab8d881570b08dd5ffeddf5aca88a10438c366e1f633253cb0b5
fa32097541 test: Create cached blocks not in the future (MarcoFalke)
Pull request description:
This avoids test failures when tests assume blocks are not from the future, like in wallet_dump: https://cirrus-ci.com/task/6607130193035264?command=ci#L3306
ACKs for top commit:
jonatack:
ACK fa32097541
Tree-SHA512: 60b6882e0e1df8c5d67f034533407a45d3685983891b67ff4631072bfd0a93a325c7ca18758d7a2df252e4fcdb7c87321cb1e84458b22782e57e719eec634c22
This is the functional test framework pendant for
7777e3624f, which renamed "strCommand" with
"msg_type" in the network processing code.
-BEGIN VERIFY SCRIPT-
# Rename in test framework
sed -i 's/command/msgtype/g' ./test/functional/test_framework/messages.py ./test/functional/test_framework/mininode.py
# Rename in individual tests
sed -i 's/command/msgtype/g' ./test/functional/p2p_invalid_messages.py ./test/functional/p2p_leak.py
-END VERIFY SCRIPT-
fab9899204 test: Try once more when RPC connection fails on Windows (MarcoFalke)
faa655731e test: Document why connection is re-constructed on windows (MarcoFalke)
fa9f4f663c test: Remove python 3.4 workaround (MarcoFalke)
fae760f2b2 cirrus: Bump freebsd to 12.1 (MarcoFalke)
Pull request description:
Fixes: #18548
ACKs for top commit:
hebasto:
ACK fab9899204, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: c4e9ed8d995b63a820ca66984f152ac216c83ba1f318b61b15c6d375c0e936c08f6bc3d38c255dddf3ee8952f848c7ababf684854e07a7c1b1d8504e6b7208ba
The interfaces for the methods wait_for_header() and wait_for_merkleblock() are
changed to take a hex string instead of an integer, improving type safety and
removing the burden from the caller to always do the transformation via
`int(...)`. As suggested by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/18593#discussion_r407062253
Implements the missing initialization/serialization methods for
msg_merkleblock, based on the already present class CMerkleBlock.
Also changes the method wait_for_merkleblock() to be more precise by waiting
for a merkleblock with a specified blockhash instead of an arbitrary one.
In the BIP37 test p2p_filter.py, this new method is used to make the test of
receiving merkleblock and tx if a filter is set to be more precise, by checking
if they also arrive in the right order.
fac3716b09 test: check that peer is connected when calling sync_* (MarcoFalke)
Pull request description:
Without a connection there is no way to sync, so we can fail early and don't have to wait for the timeout
ACKs for top commit:
jonatack:
ACK fac3716b09
Tree-SHA512: 12f771473c23e152dae4bfb201fadb2c3530cf439de64fea07d048734614543080a5d05c9c36e6e398c6a69c8279f609d34706599571814172a11bcfbea4a3b9
fa15699969 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa15699969 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
3ed772d221 [tests] remove bignum.py (John Newbery)
f950ec2520 [tests] remove bn2bin() (John Newbery)
3b9b38579c [tests] remove bn_bytes() function (John Newbery)
a760aa14a9 [tests] remove mpi2vch() function (John Newbery)
9a60bef50d [tests] don't encode the integer size in bignum (John Newbery)
1dc68aee66 [tests] add function comments to bignum (John Newbery)
f31fc0e92e [tests] fix flake8 warnings in script.py and bignum.py (John Newbery)
Pull request description:
Only one function is imported in script.py. Just move that function to script.py and remove the bignum.py module.
Remove unused functionality and fix some flake8 warnings along the way.
Top commit has no ACKs.
Tree-SHA512: 015f543ab545b5d5451896e2751d9c19334d9155b03faacd2023781e89833a2440f7f28741e9a8ac49badd9cdc012cbb6e038cdcdebeefaf9cb9d461c0689157
b951b0973c on startup, write config options to debug.log (Larry Ruane)
Pull request description:
When a developer is examining `debug.log` after something goes wrong, it's often useful to know the exact options the failing instance of `bitcoind` was started with. Sometimes the `debug.log` file is all that's available for the analysis. This PR logs the `bitcoin.conf` entries and command-line arguments to `debug.log` on startup.
ACKs for top commit:
MarcoFalke:
ACK b951b0973c🐪
jonatack:
ACK b951b0973c reviewed diff, re-code review, built, ran tests, launched bitcoind and reviewed debug log output, verified value of `str` debug log in the added unit test.
Tree-SHA512: bbca4fb3d49f99261758302bde0b8b67300ccc72e7380b01f1f66a146ae8a008a045df0ca5ca9664caff034d0ee38ea7ef38a50f38374525608c07ba52790358
8925df86c4 doc: update release notes (Jon Atack)
8bb405bbad test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f1 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4.
meshcollider:
Code review ACK 8925df86c4
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
- change the value returned in the RPC getaddressinfo `labels` field to an array
of label name strings
- deprecate the previous behavior of returning a JSON hash structure containing
label `name` and address `purpose` key/value pairs
- update the relevant tests
b6f9e3576a test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/17675#issuecomment-563188648.
ACKs for top commit:
practicalswift:
ACK b6f9e3576a assuming Travis is happy too -- diff looks correct :)
MarcoFalke:
> ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
5db506ba59 tests: Add option --valgrind to run nodes under valgrind in the functional tests (practicalswift)
Pull request description:
What is better than fixing bugs? Fixing entire bug classes of course! :)
Add option `--valgrind` to run the functional tests under Valgrind.
Regular functional testing under Valgrind would have caught many of the uninitialized reads we've seen historically.
Let's kill this bug class once and for all: let's never use an uninitialized value ever again. Or at least not one that would be triggered by running the functional tests! :)
My hope is that this addition will make it super-easy to run the functional tests under Valgrind and thus increase the probability of people making use of it :)
Hopefully `test/functional/test_runner.py --valgrind` will become a natural part of the pre-release QA process.
**Usage:**
```
$ test/functional/test_runner.py --help
…
--valgrind run nodes under the valgrind memory error detector:
expect at least a ~10x slowdown, valgrind 3.14 or
later required
```
**Live demo:**
First, let's re-introduce a memory bug by reverting the recent P2P uninitialized read bug fix from PR #17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have").
```
$ git diff
diff --git a/src/consensus/validation.h b/src/consensus/validation.h
index 3401eb64c..940adea33 100644
--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};
class TxValidationState : public ValidationState {
private:
- TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
+ TxValidationResult m_result;
public:
bool Invalid(TxValidationResult result,
const std::string &reject_reason="",
```
Second, let's test as normal without Valgrind:
```
$ test/functional/p2p_segwit.py -l INFO
2019-11-28T09:30:42.810000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__fc8q3qo
…
2019-11-28T09:31:57.187000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
…
2019-11-28T09:32:08.265000Z TestFramework (INFO): Tests successful
```
Third, let's test with `--valgrind` and see if the test fail (as we expect) when the unitialized value is used:
```
$ test/functional/p2p_segwit.py -l INFO --valgrind
2019-11-28T09:32:33.018000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_gtjecx2l
…
2019-11-28T09:40:36.702000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
2019-11-28T09:40:37.813000Z TestFramework (ERROR): Assertion failed
ConnectionRefusedError: [Errno 111] Connection refused
```
ACKs for top commit:
MarcoFalke:
ACK 5db506ba59
jonatack:
ACK 5db506ba59
Tree-SHA512: 2eaecacf4da166febad88b2a8ee6d7ac2bcd38d4c1892ca39516b6343e8f8c8814edf5eaf14c90f11a069a0389d24f0713076112ac284de987e72fc5f6cc3795
A BitcoinTestFramework child class which can be imported by an external user or
project. TestShell.setup() initiates an underlying BitcoinTestFramework object
with bitcoind subprocesses, rpc interfaces and test logging.
TestShell.shutdown() safely tears down the BitcoinTestFramework object.
This ensures TestFramework default parameters are set before setup is called. A
child class will therefore have access to defaults when overriding setup.
In order for BitcoinTestFramework to correctly restart after shutdown, the
previous logging handlers need to be removed, or else logging will continue in
the previous temp directory. "Flush" ensures buffers are emptied, and "close"
ensures file handler close logging file.
Setup and shutdown code now moved into dedicated methods. Test "success" is
added as a BitcoinTestFramework member, which can be accessed outside of main.
Argument parsing also moved into separate method and called from main.
The asyncio.new_event_loop() instance is now removed from the NetworkThread
class during shutdown. This enables a NetworkThread instance to be restarted
after being closed. The current NetworkThread class guards against an existing
new_event_loop during initialization.
The function implementing segwit v0 signature hash was originally named
SegwitVersion1SignatureHash() (presumably before segwit v0 was named
segwit v0). Rename it to SegwitV0SignatureHash().
Also rename SignatureHash() to LegacySignatureHash() for disambiguation.
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see src/validation.cpp:MemPoolAccept::PreChecks(...)),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
21 <21-byte-long string of 'a' or 1s>
former commit messages, now squashed:
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for bumped scriptPubKey
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for dummy scriptPubKeys (b'a' * 35)
test: rbf, bip68: comment DUMMY_P2WPKH_SCRIPT constant, put into common (new) module
fa25f43ac5 p2p: Remove BIP61 reject messages (MarcoFalke)
Pull request description:
Reject messages (BIP 61) appear in the following settings:
* Parsing of reject messages (in case `-debug=net` is set, off by default). This has only been used for a single `LogPrint` call for several releases now. Such logging is completely meaningless to us and should thus be removed.
* The sending of reject messages (in case `-enablebip61` is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use `-printtoconsole` to have it as stream, or read from the `debug.log` file like our python function `assert_debug_log` in the test framework does)
Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.
ACKs for top commit:
jnewbery:
utACK fa25f43ac5
laanwj:
I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5.
ryanofsky:
Code review ACK fa25f43ac5
Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
ea4cc3a7b3 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b3
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented
fa69588537 test: Make PORT_MIN in test runner configurable (MarcoFalke)
Pull request description:
This is needed when some ports in the port range are used by other processes. Note that simply assigning the ports dynamically does not work:
* We spin up several nodes per test (each node gets its own port)
* We run several tests in parallel
So to avoid nodes from different tests colliding on ports, the port assignment must be deterministic (can not be dynamic).
Fixes: #10869
ACKs for top commit:
practicalswift:
ACK fa69588537 -- diff looks correct
promag:
ACK fa69588537.
Tree-SHA512: e79adb015e7de79064e2d14336c38bc9672bd779ad6c52917721897e73f617c39d32c068a369c26670002a6c4ab95a71ef3a6878ebdd9710e02f410e2f7bcd14
333317ce6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4 test: Pass down correct chain name in tests (MarcoFalke)
Pull request description:
To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them
ACKs for top commit:
Sjors:
Thanks for adding the node 1 example. Code review ACK 333317c
Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
fa734603b7 wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5 wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)
Pull request description:
Comes with a test to aid review. The test should fail without the fix to bitcoind
The following `CreateWalletFromFile` issues are fixed:
* `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
* `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation
ACKs for top commit:
promag:
ACK fa734603b7.
darosior:
ACK fa734603b7
meshcollider:
LGTM, code-read ACK fa734603b7
Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
e4f4ea47eb lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd867150 Avoid using mutable default parameter values (practicalswift)
Pull request description:
Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.
Examples of this gotcha caught during review:
* https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
* https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304
Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:
```
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})
```
In contrast to:
```
>>> def f(i, j=None, k=None):
... if j is None:
... j = []
... if k is None:
... k = {}
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})
```
The latter is typically the intended behaviour.
This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)
ACKs for top commit:
Sjors:
Oh Python... ACK e4f4ea47eb. Testing tip: swap the two commits.
Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
e78aaf41f4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfc [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f4 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f4 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
fa3c6575ca lint: Add false positive to python dead code linter (MarcoFalke)
fa25668e1c test: Test p2sh-witness and bech32 in wallet_import_rescan (MarcoFalke)
fa79af2989 test: Replace fragile "rng" with call to random() (MarcoFalke)
fac3dcf7d0 test: Generate one block for each send in wallet_import_rescan (MarcoFalke)
Pull request description:
This adds test coverage for segwit in the `wallet_import_rescan` test, among other cleanups.
ACKs for top commit:
jnewbery:
ACK fa3c6575ca
Tree-SHA512: 877741763c62c1bf9d868864a1e3f0699857e8c028e9fcd65c7eeb73600c22cbe97b7b51093737743d9e87bcb991c1fe1086f673e18765aef0fcfe27951402f0
c5b404e8f1 Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa3918 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
fa8489a155 test: Add test for BIP30 duplicate tx (MarcoFalke)
77770d95e2 test: Properly serialize BIP34 coinbase height (MarcoFalke)
Pull request description:
This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in https://github.com/bitcoin/bitcoin/pull/16333#issuecomment-508604071)
We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request.
Also, add a commit to fix the BIP34 test failures reported in https://github.com/bitcoin/bitcoin/pull/14633#issue-227712540
ACKs for top commit:
laanwj:
Code review ACK fa8489a155
Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
faf36838bd test: Avoid hardcoding the chain name in combine_logs (MarcoFalke)
fa8a1d7ba3 test: Adapt test framework for chains other than "regtest" (MarcoFalke)
68f546635d test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley)
Pull request description:
This is required for various work in progress:
* testchains #8994
* signet #16411
* some of my locally written tests
While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes.
ACKs for top commit:
jonatack:
ACK faf36838bd, ran tests and reviewed the code.
Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
bead32e31e Add release notes for DEFAULT_BLOOM change (Matt Corallo)
f27309f55c Move DEFAULT_PEERBLOOMFILTERS from validation.h to net_processing.h (Matt Corallo)
5efcb77283 Disable bloom filtering by default. (Matt Corallo)
Pull request description:
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
ACKs for top commit:
jnewbery:
ACK bead32e31e
kallewoof:
ACK bead32e31e
Tree-SHA512: ecd901898e8efe1a7c82b471af0acc2373c2282ac633eb58d9aae7c35deda1999d0f79fb0485e6cecbda7246aeda00206cd82c7fa36866e2ac64705ba93f9390
fa1d766717 tests: Make msg_block a witness block (MarcoFalke)
fa52eb55c9 test: Remove True argument to CBlock::serialize (MarcoFalke)
Pull request description:
Unnamed arguments are confusing as to what they mean without looking up the function signature.
Since segwit is active by default in regtest, and all blocks are serialized with witness (#15664), remove the argument `with_witness=True` from all calls to `CBlock::serialize` and `BlockTransactions::serialize`.
ACKs for commit fa1d76:
laanwj:
code-review ACK fa1d766717
Tree-SHA512: 2c550646f99c9ca86a223ca988c61a730f5e6646807adeaa7174fb2424a32cea3fef8bcd3e0b12e162e7ff192877d0c02fd0654df6ee1a9b821b065707c2dcbc
BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.
NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.
fa8ced32a6 doc: Mention blocksonly in reduce-traffic.md, unhide option (MarcoFalke)
fa320de79f test: Add test for p2p_blocksonly (MarcoFalke)
fa3872e7b4 test: Format predicate source as multiline on error (MarcoFalke)
fa1dce7329 net: Rename ::fRelayTxes to ::g_relay_txes (MarcoFalke)
Pull request description:
This is de-facto no longer hidden
ACKs for commit fa8ced:
jamesob:
utACK fa8ced32a6
Tree-SHA512: 474fbdee6cbd035ed9068a066b6056c1f909ec7520be0417820fcd1672ab3069b53f55c5147968978d9258fd3a3933fe1a9ef8e4f6e14fb6ebbd79701a0a1245
a407b6fdf3 [tests] Make random seed logged and settable (John Newbery)
Pull request description:
This allows tests which use randomness to be reproducibly run on failure.
ACKs for commit a407b6:
jonatack:
re-ACK a407b6fdf3
jb55:
great! utACK a407b6fdf3
Tree-SHA512: e1e89e6e76d11ddec71a8f0f077227e4b46303f80461b170900d3f95d4dcc4187b0d1decfd63562ea970aaaf530ef032a3e64ed1669aac29033d95161855fda3
This diff has been generated with the following script, but is better
reviewed without looking at the script.
# -BEGIN VERIFY SCRIPT-
echo "Use msg_witness_block everywhere, except for tests that require msg_block"
# This could be a separate commit, but it is combined with the
# following scripts to reduce the overall diff
sed -i -e 's/msg_block/msg_witness_block/g' ./test/functional/{feature_assumevalid,feature_cltv,feature_dersig,feature_versionbits_warning,p2p_fingerprint,p2p_sendheaders,p2p_unrequested_blocks,example_test,rpc_blockchain}.py
echo "Rename msg_block to msg_no_witness_block"
# Rename msg_block to msg_no_witness_block in all tests (not the
# framework)
sed -i -e 's/msg_block/msg_no_witness_block/g' $(git grep -l msg_block ./test/functional/*.py)
# Derive msg_no_witness_block from msg_block
# Make msg_block a witness block in messages.py
patch -p1 --fuzz 0 << EOF
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 00190e4cbd..e454ed5987 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1133 +1133 @@ class msg_block:
- return self.block.serialize(with_witness=False)
+ return self.block.serialize()
@@ -1155 +1155 @@ class msg_generic:
-class msg_witness_block(msg_block):
+class msg_no_witness_block(msg_block):
@@ -1158,2 +1158 @@ class msg_witness_block(msg_block):
- r = self.block.serialize()
- return r
+ return self.block.serialize(with_witness=False)
@@ -1445 +1444 @@ class msg_blocktxn:
- r += self.block_transactions.serialize(with_witness=False)
+ r += self.block_transactions.serialize()
@@ -1452 +1451 @@ class msg_blocktxn:
-class msg_witness_blocktxn(msg_blocktxn):
+class msg_no_witness_blocktxn(msg_blocktxn):
@@ -1456,3 +1455 @@ class msg_witness_blocktxn(msg_blocktxn):
- r = b""
- r += self.block_transactions.serialize()
- return r
+ return self.block_transactions.serialize(with_witness=False)
EOF
# Conclude rename of msg_block to msg_no_witness_block
sed -i -e 's/msg_witness_block/msg_block/g' $(git grep -l msg_witness_block)
# -END VERIFY SCRIPT-
Unnamed arguments are confusing as to what they mean without looking up
the function signature.
Since segwit is active by default in regtest, and all blocks are
serialized with witness (#15664, c459c5f), remove the argument
`with_witness=True` from all calls to `CBlock::serialize` and
`BlockTransactions::serialize`.
This diff has been created with a script, but is better reviewed without
a scripted diff.
sed -i --regexp-extended -e 's/block(_?[2a-z]*)\.serialize\([a-z_]*=?True/block\1.serialize(/g' $(git grep -l serialize ./test)
9f9db39041 QA/mininode: Send all headers upfront in send_blocks_and_test to avoid sending an unconnected one (Luke Dashjr)
Pull request description:
While this doesn't currently trigger any problems, the network protocol does expect headers to be sent connectable in normal circumstances, and if too many are sent out of order will disconnect the peer.
ACKs for commit 9f9db3:
Tree-SHA512: 25b88718e4ba3d31aed2de7ece23fab9a0737fd6536c5e618ea8eb5a3a217dab0dffaebc4892df7993bcea7efb7c4fb5085fabebe99535b8f7fdde3c19df54ff
a014373d81 QA: Avoid re-reading config.ini unnecessarily (Luke Dashjr)
Pull request description:
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
ACKs for commit a01437:
practicalswift:
utACK a014373d81
hebasto:
utACK a014373d81
Tree-SHA512: 36e3dda36cf4fae243feb1655da2891d1b78c86319be9bd9809c20054fa0cb75749370b05aa9d589a4dcab6322d8cdf4e874c5175144ed23ba63b2ed338538ca
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
fafe5f0d09 test: Remove unused imports (MarcoFalke)
fa16a09215 scripted-diff: use self.sync_* methods (MarcoFalke)
faf77f9b90 test: Pass self to test_simple_bumpfee_succeeds (MarcoFalke)
fa6dc7c5c3 test: Add BitcoinTestFramework::sync_* methods (MarcoFalke)
fafe008cb4 test: Pass at most one node group to sync_all (MarcoFalke)
fa4680ed09 scripted-diff: Rename sync_blocks to send_blocks to avoid name collisions and confusion (MarcoFalke)
Pull request description:
This adds methods to the test framework that can be called by just `self.sync_*()`.
This avoids having to import the underlying util method. Also, in the default case, where all nodes are synced this avoid having to pass `self.nodes` explicitly.
So the effective changes are:
```diff
@@
-from test_framework.util import sync_blocks, sync_mempools
@@
- sync_blocks(self.nodes)
+ self.sync_blocks()
@@
- sync_mempools(self.nodes)
+ self.sync_mempools()
ACKs for commit fafe5f:
promag:
utACK fafe5f0.
jonatack:
ACK fafe5f0d09, nice simplification.
Tree-SHA512: 5c81840edf9fb3c5de2d7bf95ca36a5a8d23567cd1479a0f4044547c2080e9a3c5cf375357bc8eebb5b68829be050a171ab2512cfd47b89feed51fe3bad2cd72
fae38c3dc6 doc: Fix all typos reported by codespell (MarcoFalke)
fa9058f0ed doc: Add release notes for 15629 (MarcoFalke)
fa4a922d78 qa: Add test for missing testnet section in conf file (MarcoFalke)
dddd6f0f58 init: Throw error when network specific config is ignored (MarcoFalke)
Pull request description:
This should have no effect on mainnet users, but simplifies testing, where config settings are currently ignored with only a warning. Fix this by making it an error.
Issues:
* bitcoin client 0.17.0 ignores wallet's name (file) #14523
* Can't set custom rpcport on testnet #13777
* ...
ACKs for commit fae38c:
Tree-SHA512: 2e209526898eea6e444c803ec2666989cee4ca137492d32984998733c50a70056cb54657df8dc3027a6a0612738a8afce0bc35824b868c5f22281e00e0188530
8f5d9431a Add regtests for HTTP status codes. (Daniel Kraft)
Pull request description:
This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success.
#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally.
ACKs for commit 8f5d94:
laanwj:
utACK 8f5d9431a3
promag:
utACK 8f5d943.
jonasschnelli:
utACK 8f5d9431a3
Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
This adds explicit tests for the returned HTTP status codes to
interface_rpc.py (for error cases) and the HTTP JSON-RPC client in
general for success.
PR 15381 brought up discussion about the HTTP status codes in general,
and the general opinion was that the current choice may not be ideal
but should not be changed to preserve compatibility with existing
JSON-RPC clients. Thus it makes sense to actually test the current
status to ensure this desired compatibility is not broken accidentally.
fa2cdc9ac2 test: Simplify create_cache (MarcoFalke)
fa25210d62 qa: Fix wallet_txn_doublespend issue (MarcoFalke)
1111aecbb5 qa: Always refresh stale cache to be out of ibd (MarcoFalke)
fab0d85802 qa: Remove mocktime unless required (MarcoFalke)
Pull request description:
When starting a test, we are always in IBD because the timestamps on cached blocks are in the past. Usually, we solve that by generating a block at the beginning of the test.
That is clumsy and might even lead to other problems such as #15360 and https://github.com/bitcoin/bitcoin/issues/14446#issuecomment-461926598
So fix that by getting rid of mocktime and always refreshing the last block of the cache when starting the test framework.
Should fix#14446
Tree-SHA512: 6af09800f9c86131349a103af617a54551f5f3f3260d38e14e3f30fdd3d91a0feb0100c56cbb12eae4aeac5571ae4b530b16345cbb831d2670237b53351a22c1
8e4b4f683a Address test todos by removing -txindex to nodes. Originally added when updating getrawtransaction to stop searching unspent utxos. (Amiti Uttarwar)
Pull request description:
Original todos added when removing getrawtransaction default behavior of searching unspent utxos.
Tree-SHA512: d080953c3b0d2e5dca2265a15966dc25985a614c9cc86271ecd6276178ce428c85e262c24df92501695c32fed7beec0339b989f03cce91b57fb2efba201b7809
e3e1a5631e [test] functional: set cwd of nodes to tmpdir (Sjors Provoost)
Pull request description:
Any process launched by bitcoind will have `self.datadir` as its `cwd`.
Tree-SHA512: 0b311643bb96c7dc2f693774620173243b3add40bf373284695af2f0071823b23485289fd2ffe152b7f63e0bfe989b16720077cfc2ce33905f9b8e7f2630f3c0
fa178a6385 [rpc] mining: Omit uninitialized currentblockweight, currentblocktx (MarcoFalke)
Pull request description:
Previously we'd report "0", which could be mistaken for a valid number. E.g. the number of transactions is 0 or the block weight is 0, whatever that means.
Tree-SHA512: ee94ab203a329e272211b726f4c23edec4b09c650ec363b77fd59ad9264165d73064f78ebb9e11b5c2c543b73c157752410a307655560531c7d5444d203aa0ea
b651ef7e1c submitheader: more directly test missing prev block header (Gregory Sanders)
1e7f741745 remove some magic mining constants in functional tests (Gregory Sanders)
Pull request description:
The fewer magic numbers the better.
Also more directly tested a `submitheader` case of bad previous blockhash.
Tree-SHA512: 52b01a6aa199fa909eea4e9e84409a901933e545724e33149cc4132c82168199fd678809b6d94d95c9ff6ad02238a9552363620d13b8beaa5d4b67ade9ef425c
13782b8ba8 docs: add perf section to developer docs (James O'Beirne)
58180b5fd4 tests: add utility to easily profile node performance with perf (James O'Beirne)
Pull request description:
Adds a context manager to easily (and selectively) profile node performance during functional test execution using `perf`.
While writing some tests, I encountered some odd bitcoind slowness. I wrote up a utility (`TestNode.profile_with_perf`) that generates performance diagnostics for a node by running `perf` during the execution of a particular region of test code.
`perf` usage is detailed in the excellent (and sadly unmerged) https://github.com/bitcoin/bitcoin/pull/12649; all due props to @eklitzke.
### Example
```python
with node.profile_with_perf("large-msgs"):
for i in range(200):
node.p2p.send_message(some_large_msg)
node.p2p.sync_with_ping()
```
This generates a perf data file in the test node's datadir (`/tmp/testtxmpod0y/node0/node-0-TestName-large-msgs.perf.data`).
Running `perf report` generates nice output about where the node spent most of its time while running that part of the test:
```bash
$ perf report -i /tmp/testtxmpod0y/node0/node-0-TestName-large-msgs.perf.data --stdio \
| c++filt \
| less
# To display the perf.data header info, please use --header/--header-only options.
#
#
# Total Lost Samples: 0
#
# Samples: 135 of event 'cycles:pp'
# Event count (approx.): 1458205679493582
#
# Children Self Command Shared Object Symbol
# ........ ........ ............... ................... ........................................................................................................................................................................................................................................................................
#
70.14% 0.00% bitcoin-net bitcoind [.] CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
|
---CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
70.14% 0.00% bitcoin-net bitcoind [.] CNetMessage::readData(char const*, unsigned int)
|
---CNetMessage::readData(char const*, unsigned int)
CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
35.52% 0.00% bitcoin-net bitcoind [.] std::vector<char, zero_after_free_allocator<char> >::_M_fill_insert(__gnu_cxx::__normal_iterator<char*, std::vector<char, zero_after_free_allocator<char> > >, unsigned long, char const&)
|
---std::vector<char, zero_after_free_allocator<char> >::_M_fill_insert(__gnu_cxx::__normal_iterator<char*, std::vector<char, zero_after_free_allocator<char> > >, unsigned long, char const&)
CNetMessage::readData(char const*, unsigned int)
CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
...
```
Tree-SHA512: 9ac4ceaa88818d5eca00994e8e3c8ad42ae019550d6583972a0a4f7b0c4f61032e3d0c476b4ae58756bc5eb8f8015a19a7fc26c095bd588f31d49a37ed0c6b3e
When running tests with --usecli, unify the conversion from argument objects to
strings using a new function arg_to_cli(). This fixes boolean arguments when
using named arguments.
Also use json.dumps() to get the string values for arguments that are dicts and
lists so that bitcoind's JSON parser does not become confused.
3c3e31c3a4 [tests] Add wallet-tool test (João Barbosa)
49d2374acf [tools] Add wallet inspection and modification tool (Jonas Schnelli)
Pull request description:
Adds an offline tool `bitcoin-wallet-tool` for wallet creation and maintenance.
Currently this tool can create a new wallet file, display information on an existing wallet, and run the salvage and zapwallettxes maintenance tasks on an existing wallet. It can later be extended to support other common wallet maintenance tasks.
Doing wallet maintenance tasks in an offline tool makes much more sense (and is potentially safer) than having to spin up a full node.
Tree-SHA512: 75a28b8a58858d9d76c7532db40eacdefc5714ea5aab536fb1dc9756e2f7d750d69d68d59c50a68e633ce38fb5b8c3e3d4880db30fe01561e07ce58d42bceb2b
Introduces `TestNode.profile_with_perf()` context manager which
samples node execution to produce profiling data.
Also introduces a test framework flag, `--perf`, which will run
perf on all nodes for the duration of a given test.
fab17e8272 test: Add basic test for BIP34 (MarcoFalke)
Pull request description:
BIP34 was disabled for testing, which explains why it had no test.
Fix that by enabling it and adding a test.
Tree-SHA512: 9cb5702d474117ce6420226eb93ee09d6fb5fc856fabc8b67abe56a088cd727674e0e5462000e1afa83b911374036f90abdbdde56a8c236a75572ed47e10a00f
59e387705c test: add invalid tx templates for use in functional tests (James O'Beirne)
Pull request description:
This change adds a list of `CTransaction`-generating templates which each correspond to a specific type of invalid transaction. We then use this list to test for a wider variety of invalid tx types in `p2p_invalid_tx.py` and `feature_block.py`.
Consolidating all invalid tx types will allow us to more easily cover all tx reject cases from a variety of tests without repeating ourselves. Validation logic doesn't differ much between mempool and block acceptance, but there *is* a difference and we should be sure we're testing both comprehensively.
Right now, I've only added templates covering the tx reject types listed below but if this approach seems worthwhile I will expand the list to be fully comprehensive.
```
bad-txns-in-belowout
bad-txns-inputs-duplicate
bad-txns-too-many-sigops
bad-txns-vin-empty
bad-txns-vout-empty
bad-txns-vout-negative
```
Tree-SHA512: 05407f4a953fbd7c44c08bb49bb989cefd39a2b05ea00f5b3c92197a3f05e1b302f789e33832445734220e1c333d133aba385740b77b84139b170c583471ce20
4999992c34 whitespace: Split ~300 char line into multiple ones (MarcoFalke)
fa71b38168 scripted-diff: Rename rpc_timewait to rpc_timeout (MarcoFalke)
fa3e5786d0 scripted-diff: Remove unused 'split' parameter to setup_network (MarcoFalke)
Pull request description:
This is a bugfix, since wallet_dump currently uses the wrong name:
18857b4c40/test/functional/wallet_dump.py (L89-L92)
Rename all to the same name with a scripted diff (and some unrelated cleanups).
Tree-SHA512: 338ddd20dae12e6cf7aa7adbcfb239cf648017a1572b373f8431fecb184bd2a65492846d81e75a023864d9e41c94afb53044c16b79651a5937d34a5a6b772f81
2474de0265 Fix running individually through test_runner.py, as suggested by @MarcoFalke (#14732) (Kristaps Kaupe)
Pull request description:
As suggested by @MarcoFalke. Resolves#14732.
Tree-SHA512: b4e400ba06075e218dbd97d0390845f1c55be42a2b6fd70513381318cfc2693473ba1d0f9d7f379a96939d1960b53801fad7c02e06bddc50c5a835ad024c37ef
fa4b8c90d3 test: add_nodes can only be called once after set_test_params (MarcoFalke)
faa831102a Revert "tests: Support calling add_nodes more than once" (MarcoFalke)
Pull request description:
Writing tests should be straightforward and with little side-effects as possible.
I don't see how this is needed and can not be achieved with `self.num_nodes` (and `self.extra_args` et al.)
Tree-SHA512: 83a67f2cba9d97e21d80847ff405a4633fcb0d5674486efa57ee1813e46efe8709ae0fb462b8339a01ebeca5c4f2d29ecb1807d648b8fd9ee8ce336b08d580a8
98a1846b00 tests: Support calling add_nodes more than once (Steven Roose)
Pull request description:
Ran into this while writing [a multi-chain test for Elements](https://github.com/ElementsProject/elements/pull/458) where I call this method more than once.
Tree-SHA512: f2d698fcb560552aa5d81a4c3fbf40b7269b228b34d85a118291649ef83f8c0a30cd82a28d418237b55893bcecd538046b704e64a4d8a41f2c0aef8033dc83e5
aaaa8eb1ed test: consensus: Check that final transactions are valid (MarcoFalke)
fae3617d79 test: Correctly deserialize without witness (MarcoFalke)
Pull request description:
There is no check that checks that final transactions are valid, i.e. the consensus rules could be changed (accidentally) with none of the tests failing.
Tree-SHA512: 48f4c24bfcc525ddbc1bfe8c37131953b464823428c1f7a278ba6d98b98827b6b84a8eb2b33396bfb5b8cc4012b7cc1cd771637f405ea20beddae001c22aa290
31926ee8cf [test] functional framework: add CScript hex() for Python 3.4 (Sjors Provoost)
74ce326831 [test] Travis: enforce Python 3.4 support in functional tests (Sjors Provoost)
Pull request description:
The minimum supported version of Python is 3.4 according to [dependencies.md](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md). This PR makes the Travis linter use this version in order to catch accidental use of modern syntax.
Tree-SHA512: 71b2c102be72b135a8ba049378d66875760f20a04a657102a399240c5c2b2ddbdfa7d5ab4c0c0242ecc3259e0ee8eb2273f331bc5eb824f4ae4c3cc58aea37ac
d6b3790d1a tests: check readability of cookie file (Chun Kuan Lee)
Pull request description:
This PR would wait until the `.cookie` file is readable
Possible fix no. 5 `PermissionError` in #14446
Tree-SHA512: e7055c7ca26a6eadbbe19e4eef08ffee61cd17de79b30af2f0d090f0ad81ca24815e3c7e034e5e30d47c580bb0b221b3955e9ff2fcec2274fbf7b9232ab0cdc7
28479f926f qa: Test bitcond shutdown (João Barbosa)
8d3f46ec39 http: Remove timeout to exit event loop (João Barbosa)
e98a9eede2 http: Remove unnecessary event_base_loopexit call (João Barbosa)
6b13580f4e http: Unlisten sockets after all workers quit (João Barbosa)
18e9685816 http: Send "Connection: close" header if shutdown is requested (João Barbosa)
02e1e4eff6 rpc: Add wait argument to stop (João Barbosa)
Pull request description:
Fixes#11777. Reverts #11006. Replaces #13501.
With this change the HTTP server will exit gracefully, meaning that all requests will finish processing and sending the response, even if this means to wait more than 2 seconds (current time allowed to exit the event loop).
Another small change is that connections are accepted even when the server is stopping, but HTTP requests are rejected. This can be improved later, especially if chunked replies are implemented.
Briefly, before this PR, this is the order or events when a request arrives (RPC `stop`):
1. `bufferevent_disable(..., EV_READ)`
2. `StartShutdown()`
3. `evhttp_del_accept_socket(...)`
4. `ThreadHTTP` terminates (event loop exits) because there are no active or pending events thanks to 1. and 3.
5. client doesn't get the response thanks to 4.
This can be verified by applying
```diff
// Event loop will exit after current HTTP requests have been handled, so
// this reply will get back to the client.
StartShutdown();
+ MilliSleep(2000);
return "Bitcoin server stopping";
}
```
and checking the log output:
```
Received a POST request for / from 127.0.0.1:62443
ThreadRPCServer method=stop user=__cookie__
Interrupting HTTP server
** Exited http event loop
Interrupting HTTP RPC server
Interrupting RPC
tor: Thread interrupt
Shutdown: In progress...
torcontrol thread exit
Stopping HTTP RPC server
addcon thread exit
opencon thread exit
Unregistering HTTP handler for / (exactmatch 1)
Unregistering HTTP handler for /wallet/ (exactmatch 0)
Stopping RPC
RPC stopped.
Stopping HTTP server
Waiting for HTTP worker threads to exit
msghand thread exit
net thread exit
... sleep 2 seconds ...
Waiting for HTTP event thread to exit
Stopped HTTP server
```
For this reason point 3. is moved right after all HTTP workers quit. In that moment HTTP replies are queued in the event loop which keeps spinning util all connections are closed. In order to trigger the server side close with keep alive connections (implicit in HTTP/1.1) the header `Connection: close` is sent if shutdown was requested. This can be tested by
```
bitcoind -regtest
nc localhost 18443
POST / HTTP/1.1
Authorization: Basic ...
Content-Type: application/json
Content-Length: 44
{"jsonrpc": "2.0","method":"stop","id":123}
```
Summing up, this PR:
- removes explicit event loop exit — event loop exits once there are no active or pending events
- changes the moment the listening sockets are removed — explained above
- sends header `Connection: close` on active requests when shutdown was requested which is relevant when it's a persistent connection (default in HTTP 1.1) — libevent is aware of this header and closes the connection gracefully
- removes event loop explicit break after 2 seconds timeout
Tree-SHA512: 4dac1e86abe388697c1e2dedbf31fb36a394cfafe5e64eadbf6ed01d829542785a8c3b91d1ab680d3f03f912d14fc87176428041141441d25dcb6c98a1e069d8
2012d4df2 Add CScriptNum decode python implementation in functional suite (Gregory Sanders)
Pull request description:
I needed this for reasons and thought it'd be good to upsteam it.
Tree-SHA512: 6ea89fa2a5f5a7759ba722f2b4ed5cd6423ebfff4e83ac8b8b5c935e6aa479684e626c5f41fa020816d2a9079a99af5564e30808594d5c13e3b51ec9b474926d
d20a9fa13d tests: add tests for invalid P2P messages (James O'Beirne)
62f94d39f8 tests: add P2PConnection.send_raw_message (James O'Beirne)
5aa31f6ef2 tests: add utility to assert node memory usage hasn't increased (James O'Beirne)
Pull request description:
- Adds `p2p_invalid_messages.py`: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages.
- Adds `TestNode.assert_memory_usage_stable`: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test.
- Adds `P2PConnection.send_raw_message`: which allows us to construct and send messages with tweaked headers.
Tree-SHA512: 720a4894c1e6d8f1551b2ae710e5b06c9e4f281524623957cb01599be9afea82671dc26d6152281de0acb87720f0c53b61e2b27d40434d30e525dd9e31fa671f
Adds a utility to get resident set size memory usage for a test
node and a context manager that allows assertions based upon
maximum memory use increase.
This requires a small changes to a few tests, but means that
deterministic addresses will always be imported (unless setup_nodes
behaviour is explicitly overridden).
c11875c590 Add segwit address tests for importmulti (MeshCollider)
201451b1ca Make getaddressinfo return solvability (MeshCollider)
1753d217ea Add release notes for importmulti segwit change (MeshCollider)
353c064596 Fix typo in test_framework/blocktools (MeshCollider)
f6ed748cf0 Add SegWit support to importmulti with some ProcessImport cleanup (MeshCollider)
Pull request description:
Add support for segwit to importmulti, supports P2WSH, P2WPKH, P2SH-P2WPKH, P2SH-P2WSH. Adds a new `witnessscript` parameter which must be used for the witness scripts in the relevant situations.
Also includes some tests for the various import types.
~Also makes the change in #14019 redundant, but cherry-picks the test from that PR to test the behavior (@achow101).~
Fixes#12253, also addresses the second point in #12703, and fixes#14407
Tree-SHA512: 775a755c524d1c387a99acddd772f677d2073876b72403dcfb92c59f9b405ae13ceedcf4dbd2ee1d7a8db91c494f67ca137161032ee3a2071282eeb411be090a
If a node has not announced a tx at all, then it should respond to
getdata messages for that tx with notfound, to avoid leaking tx
origination privacy.
Adds a generate() method to the TestNode class in the test framework.
This method intercepts calls to generate, imports a dewterministic
private key to the node and then calls generatetoaddress to generate the
block to that address.
Note that repeated calls to importprivkey for the same private keys are
no-ops, so it's fine to call the generate() method many times.
faa4043c66 qa: Run more tests with wallet disabled (MarcoFalke)
Pull request description:
Instead of skipping the whole test, only skip the wallet specific section of a test if the wallet is not compiled in. This is mostly an indentation change, so can be reviewed with `--ignore-all-space`.
Tree-SHA512: 5941a8b6b00dca5cf9438c5f6f010ba812115188a69e427d7ade4c1ab8cfe7a57c73daf52c66235dbb24b1cd9ab7c7a17c49bc23d931e041b605d79116a71f66
a2a04a5abb Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled (Luke Dashjr)
92af71cea9 configure: Make it possible to build only one of bitcoin-cli or bitcoin-tx (Luke Dashjr)
Pull request description:
Includes #5618 (which the reasons for rejecting no longer hold true)
Tree-SHA512: f30a8e4a2f70166b7cabef77c4674163b3a9da14c6a547d34f00d1056a19bf4d23e22851eea726fad2afc8735d5473ae91122c770b65ac3886663dc20e2c5b70
e460232876 Document fixed attribute behavior in critical test framework classes. (Justin Turner Arthur)
17b42f4122 Check for specific tx acceptance failures based on script signature (Justin Turner Arthur)
3a4449e9ad Strictly enforce instance attrs in critical functional test classes. (Justin Turner Arthur)
1d0ce94a54 Fix for incorrect version attr set on functional test segwit block. (Justin Turner Arthur)
ba923e32a0 test: Fix broken segwit test (practicalswift)
Pull request description:
No extra attributes will be able to be added to instances of the C++ class ports or of other critical classes without causing an exception. Helps prevent adding or depending on attributes that aren't in the intended object structure. It may prevent issues such as the one fixed in bitcoin/bitcoin#14300.
This request fixes the erroneous version attribute used in the p2p_segwit.py functional tests. This pull includes the commit from bitcoin/bitcoin#14300.
Tree-SHA512: 1b8c58e7aa0f71075ed5ff3e5be0a5182599108d8cd9bce682feac3b1842508124288e9335432b16a43f40f159c9710899e6d84af1b5868f48c947bc6f3e07ec
661ac15a4a appveyor: Run functional tests on appveyor (Chun Kuan Lee)
2148c36b6e tests: Make it possible to run functional tests on Windows (Chun Kuan Lee)
Pull request description:
This PR do the following things:
- Make functional tests compatible with Windows
- Print color output in functional tests for Windows 10
- Run util and functional tests on appveyor
- Do not run symlink tests on Windows
Note:
- The wallet_multiwallet.py fail is unrelated to the test framework, it's a bug related to c++ code or maybe dependencies. `bitcoind` would exit with 0xC0000005(Access violation) during shutdown occasionally. Disable this for now.
- Not using `--failfast` because this is still in experimental. We should track if there is any other error.
- Disable ZMQ tests because the python zmq library could cause access violation sometimes.
- Disable `feature_notifications` because Bitcoin Core handles the command in different thread, whicha can cause a race condition.
Tree-SHA512: b76db137d264e62a5c130e1cbca7a2ca002a7a0f4153fa0b92c1ea6c9c09ef0533e11c49bdbd566c472d8ff59f245758feb5e5a6ec6cb6bb66a1c67bab5fa48a
c1dde3a949 No longer shutdown after encrypting the wallet (Andrew Chow)
d7637c5a3f After encrypting the wallet, reload the database environment (Andrew Chow)
5d296ac810 Add function to close all Db's and reload the databae environment (Andrew Chow)
a769461d5e Move BerkeleyEnvironment deletion from internal method to callsite (Andrew Chow)
Pull request description:
This is the replacement for #11678 which implements @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/11678#pullrequestreview-76464511).
Shutting down the software was to prevent the BDB environment from writing unencrypted private keys to disk in the database log files, as was noted [here](https://bitcointalk.org/index.php?topic=51474.msg616068#msg616068). This PR replaces the shutdown behavior with a CDBEnv flush, close, and reopen which achieves the same effect: everything is cleanly flushed and closed, the log files are removed, and then the environment reopened to continue normal operation.
To ensure that no unencrypted private keys are in the log files after encrypting the wallet, I wrote [this script](https://gist.github.com/achow101/7f7143e6c3d3fdc034d3470e72823e9d) to pull private keys from the original wallet file and searches for these keys in the log files (note that you will have to change your file paths to make it work on your own machine).
As for concerns about private keys being written to slack space or being kept in memory, these behaviors no longer exist after the original wallet encryption PR and the shutting down solution from 2011.
cc @ryanofsky
Tree-SHA512: 34b894283b0677a873d06dee46dff8424dec85a2973009ac9b84bcf3d22d05f227c494168c395219d9aee3178e420cf70d4b3eeacc9785aa86b6015d25758e75
fa8433e379 qa: Remove unneded import_deterministic_coinbase_privkeys overwrite, add comments (MarcoFalke)
e413c2ddd1 qa: Fix codespell error and have lint-spelling error instead of warn (MarcoFalke)
Pull request description:
Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs.
Note that this change most likely requires the `./test/cache/` to be cleared.
Tree-SHA512: 9ce26036b0e10f0f888f66a1e50be6a357343f9ffb302ae24a7bb3df2f083a31702ef308b738a03b08a1b623aeddac5d6563dc1b15078c0357b7dafad7808ec3
fac9539836 qa: Run all tests even if wallet is not compiled (MarcoFalke)
faa669cbcd qa: Premine to deterministic address with -disablewallet (MarcoFalke)
Pull request description:
Currently the test_runner would exit if the wallet was not compiled into the Bitcoin Core executable. However, a lot of the tests run without the wallet just fine and there is no need to globally require the wallet to run the tests.
Tree-SHA512: 63177260aa29126fd20f0be217a82b10b62288ab846f96f1cbcc3bd2c52702437703475d91eae3f8d821a3149fc62b725a4c5b2a7b3657b67ffcbc81532a03bb
fac3e22b18 qa: Read reject reasons from debug log, not p2p messages (MarcoFalke)
Pull request description:
For local testing we don't need to rely on p2p messages just to assert a reject reason.
Replace reading p2p messages with reading from the debug log file.
Tree-SHA512: fa59598ecf5e00cfb420ef1892d90aa415501fd882e1c608894dc577b0d00e93a442326d3a9167fef77d26aafbe345b730b49109982ccad68a5942384564a90b
68400d8b96 tests: Use explicit imports (practicalswift)
Pull request description:
Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports.
Wildcard imports make it unclear which names are present in the namespace, confusing both readers and many automated tools.
An additional benefit of not using wildcard imports in tests scripts is that readers of a test script then can infer the rough testing scope just by looking at the imports.
Before this commit:
```
$ contrib/devtools/lint-python.sh | head -10
./test/functional/feature_rbf.py:8:1: F403 'from test_framework.util import *' used; unable to detect undefined names
./test/functional/feature_rbf.py:9:1: F403 'from test_framework.script import *' used; unable to detect undefined names
./test/functional/feature_rbf.py:10:1: F403 'from test_framework.mininode import *' used; unable to detect undefined names
./test/functional/feature_rbf.py:15:12: F405 bytes_to_hex_str may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:17:58: F405 CScript may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:25:13: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:26:31: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:26:60: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:30:41: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
./test/functional/feature_rbf.py:30:68: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
$
```
After this commit:
```
$ contrib/devtools/lint-python.sh | head -10
$
```
Tree-SHA512: 3f826d39cffb6438388e5efcb20a9622ff8238247e882d68f7b38609877421b2a8e10e9229575f8eb6a8fa42dec4256986692e92922c86171f750a0e887438d9
cf9ed307e6 qa: blocktools enforce named args for amount (MarcoFalke)
Pull request description:
Since #13669 changed some signatures, I think it might be worthwhile to enforce named args for primitive types such as amounts.
Tree-SHA512: 2733e7b6a20590b54bd54e81a09e3f5e2fadf4390bed594916b70729bcf485b048266012c1203369e0968032a2c6a2719107ac17ee925d8939af3df916eab1a6
fafe73a626 qa: Raise feature_help timeout to 5s (MarcoFalke)
faabd7bc47 qa: Use files for stdout/stderr to support Windows (MarcoFalke)
facb56ffaf qa: Run gen_rpcauth with sys.executable (MarcoFalke)
fada8966c5 qa: Close stdout and stderr file when node stops (MarcoFalke)
Pull request description:
### qa: Close stdout and stderr file when node stops
Since these files are potentially deleted by the test framework for cleanup, they should be closed first. Otherwise this will lead to errors on Windows when the tests finish successfully.
Side note: After the patch, it is no longer possible to reopen the file on Windows (see https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile)
### qa: Run gen_rpcauth with sys.executable
Similar to `test_runner.py`, the `sys.executable` needs to be passed down into subprocesses to pass on native Windows. (Should have no effect on Linux)
### qa: Use files for stdout/stderr to support Windows
It seems that using PIPE is not supported on Windows. Also, it is easier to just use the files that capture the stdout and stderr within the test node class.
Tree-SHA512: ec675012b10705978606b7fcbdb287c39a8e6e3732aae2fa4041d963a3c6993c6eac6a9a3cbd5479514e7d8017fe74c12235d1ed6fed2e8af8f3c71981e91864
fa85c985ed qa: Add p2p_invalid_locator test (MarcoFalke)
Pull request description:
Should not be merged *before* #13907
Tree-SHA512: a67ca407854c421ed20a184d0b0dc90085aed3e3431d9652a107fa3022244767e67f67e50449b7e95721f56906836b134615875f28a21e8a012eb22cfe6a66a5
fa5587fe71 qa: wait_for_verack by default (MarcoFalke)
Pull request description:
This removes the need to do so manually every time a connection is added.
Tree-SHA512: a46c92cb4df41e30778b42b9fd3dcbd8d2d82aa7503d1213cb1c1165034f648d8caee01c292e2d87d05b0f71696996eef5be8a753f35ab49e5f66b0e3bf29f21
3fc20632a3 qt: Set BLOCK_CHAIN_SIZE = 220 (DrahtBot)
2b6a2f4a28 Regenerate manpages (DrahtBot)
eb7daf4d60 Update copyright headers to 2018 (DrahtBot)
Pull request description:
Some trivial maintenance to avoid having to do it again after the 0.17 branch off.
(The scripts to do this are in `./contrib/`)
Tree-SHA512: 16b2af45e0351b1c691c5311d48025dc6828079e98c2aa2e600dc5910ee8aa01858ca6c356538150dc46fe14c8819ed8ec8e4ec9a0f682b9950dd41bc50518fa
fa67505e1e qa: Quote wallet name for rpc path (MarcoFalke)
Pull request description:
When using external multiwallets they are specified by their full path which might contain non-ascii characters (e.g. umlauts or emojis).
Fix this by url-quoting the path.
Tree-SHA512: 7cc66514579d9f602f88a6817c5ab43a44c6d3711df452dc904173f0bc34e2c0b2c34877497f06b61f6720c532fa183053f54622dc454e316c89cee7eaa72463
https://github.com/bitcoin/bitcoin/pull/13715 introduced a new check
for _transport.is_closing() in mininode's P2PConnection's. This function
is only available from Python 3.4.4, though, while Bitcoin is supposed
to support all Python 3.4 versions.
In this change, we make the check conditional on is_closing() being
available. If it is not, then we revert to the behaviour before the
check was introduced; this means that
https://github.com/bitcoin/bitcoin/issues/13579 is not fixed for old
systems, but at least the tests work as they used to do before.
This includes a small refactoring from a one-line lambda to an
inline function, because this makes the code easier to read with more
and more conditions being added.
Fixes https://github.com/bitcoin/bitcoin/issues/13745.
fa4bf92be9 Remove dead service bits code (MarcoFalke)
Pull request description:
Seems fine to remove for the upcoming 0.17 release
Fixes#10993
Tree-SHA512: 3a4664b787e3da399bcaaba693619bd384826df14f469dbdfbbfffc540d9da3f2b322cda262b43388376785f77907c2540541c239ab0fca82bd7eb69d02b6b7a
a3fa4d6a6a QA: Fix bug in -usecli logic that converts booleans to non-lowercase strings (Jonas Schnelli)
4704e5f074 [QA] add createwallet disableprivatekey test (Jonas Schnelli)
c7b8f343e9 [Qt] Disable creating receive addresses when private keys are disabled (Jonas Schnelli)
2f15c2bc20 Add disable privatekeys option to createwallet (Jonas Schnelli)
cebefba085 Add option to disable private keys during internal wallet creation (Jonas Schnelli)
9995a602a6 Add facility to store wallet flags (64 bits) (Jonas Schnelli)
Pull request description:
This mode ('createwallet {"disableprivatekeys": true}') is intended for a sane pure watch-only mode, ideal for a use-case where one likes to use Bitcoin-Core in conjunction with a hardware-wallet or another solutions for cold-storage.
Since we have support for custom change addresses in `fundrawtransaction`, pure watch-only wallets including coin-selection are possible and do make sense for some use cases.
This new mode disables all forms of private key generation and ensure that no mix between hot and cold keys are possible.
Tree-SHA512: 3ebe7e8d54c4d4e5f790c348d4c292d456f573960a5b04d69ca5ef43a9217c7e7671761c6968cdc56f9a8bc235f3badd358576651af9f10855a0eb731f3fc508
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
