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
