76e0e6087d qa: Account for errno not always being set for ConnectionResetError (Hodlinator)
Pull request description:
The lack of errno can cause unclear and long log output.
Issue can be triggered by:
```diff
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -263,6 +263,7 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
/** HTTP request callback */
static void http_request_cb(struct evhttp_request* req, void* arg)
{
+ throw std::runtime_error{"Hello"};
evhttp_connection* conn{evhttp_request_get_connection(req)};
// Track active requests
{
```
and running a functional test such as *test/functional/feature_abortnode.py*.
`http.client.RemoteDisconnected` not specifying `errno` to `ConnectionResetError`-ctor: ce4b0ede16/Lib/http/client.py (L1556C9-L1556C29)
<details><summary>Before/after log examples</summary>
#### Log before
```
2025-11-14T20:53:05.272804Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 138, in main
self.setup()
~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 268, in setup
self.setup_network()
~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/./build/test/functional/feature_abortnode.py", line 21, in setup_network
self.setup_nodes()
~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 381, in setup_nodes
self.start_nodes()
~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 527, in start_nodes
node.wait_for_rpc_connection()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_node.py", line 326, in wait_for_rpc_connection
rpc.getblockcount()
~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/hodlinator/bc/3/test/functional/test_framework/authproxy.py", line 137, in __call__
response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hodlinator/bc/3/test/functional/test_framework/authproxy.py", line 111, in _request
return self._get_response()
~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/authproxy.py", line 174, in _get_response
http_response = self.__conn.getresponse()
File "/nix/store/62fdlzq1x1ak2lsxp4ij7ip5k9nia3hc-python3-3.13.7/lib/python3.13/http/client.py", line 1430, in getresponse
response.begin()
~~~~~~~~~~~~~~^^
File "/nix/store/62fdlzq1x1ak2lsxp4ij7ip5k9nia3hc-python3-3.13.7/lib/python3.13/http/client.py", line 331, in begin
version, status, reason = self._read_status()
~~~~~~~~~~~~~~~~~^^
File "/nix/store/62fdlzq1x1ak2lsxp4ij7ip5k9nia3hc-python3-3.13.7/lib/python3.13/http/client.py", line 300, in _read_status
raise RemoteDisconnected("Remote end closed connection without"
" response")
http.client.RemoteDisconnected: Remote end closed connection without response
```
#### Log after
```
2025-11-14T20:48:10.552126Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 138, in main
self.setup()
~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 268, in setup
self.setup_network()
~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/./build/test/functional/feature_abortnode.py", line 21, in setup_network
self.setup_nodes()
~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 381, in setup_nodes
self.start_nodes()
~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_framework.py", line 527, in start_nodes
node.wait_for_rpc_connection()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/3/test/functional/test_framework/test_node.py", line 316, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
f'bitcoind exited with status {self.process.returncode} during initialization. {str_error}'))
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status -6 during initialization. terminate called after throwing an instance of 'std::runtime_error'
what(): Hello
************************
```
Note how even the C++ exception message is now included.
</details>
ACKs for top commit:
maflcko:
review ACK 76e0e6087d 🌬
furszy:
Tested ACK 76e0e6087d
l0rinc:
untested code review ACK 76e0e6087d
Tree-SHA512: 55a83d664624932b919ab2a5b6369121db448d27628029f21c5df297892dd56d179d710ad744f6407b51aa576fb6905a38bbc29885c534ec20704c22717a0880
fa904fc683 lint: Remove confusing, redundant, and brittle lint-spelling (MarcoFalke)
Pull request description:
`codespell` was a fun experiment. However, it has many issues, when used in this project:
* The number of false-positives and true-positives are in the same ballpark. There are also many false-negatives, so the overall net-benefit is questionable.
* There is often confusion around spelling errors leading to a failing CI (they do not, which was intended).
* LLMs released this year are capable to detect typos with less false-positives and less false-negatives, so the `codespell` integration is a bit redundant in that sense.
Fix all issues by removing it.
Going forward, anyone is free to continue to use `codespell`, or any LLM, or any other tool, locally. Also, DrahtBot has the LLM typo linter integrated in the summary comment. I think the options are plenty, and are more than sufficient for now.
ACKs for top commit:
l0rinc:
ACK fa904fc683
rkrux:
ACK fa904fc683
pablomartin4btc:
ACK fa904fc683
Tree-SHA512: 5e2008a77c2c313605f30d73286111eba034a2a6bb2a0a48e2f77ec6ccc7afaa274e00bbfcb727be0ac5e547b8ae9c801d30c43589b0cad2099565e6716b9ec7
59b93f11e8 rest: print also HTTP response reason in case of an error (Roman Zeyde)
7fe94a0493 rest: add a test for unsuported `/blockpart/` request type (Roman Zeyde)
55d0d19b5c rest: deduplicate `interface_rest.py` negative tests (Roman Zeyde)
89eb531024 rest: update release notes for `/blockpart/` endpoint (Roman Zeyde)
41118e17f8 blockstorage: simplify partial block read validation (Roman Zeyde)
599effdeab rest: reformat `uri_prefixes` initializer list (Roman Zeyde)
Pull request description:
The commits below should resolve a few leftovers from #33657.
ACKs for top commit:
l0rinc:
ACK 59b93f11e8
hodlinator:
re-ACK 59b93f11e8
Tree-SHA512: ae45e08edd315018e11283b354fb32f9658f5829c956554dc662a81c2e16397def7c3700e6354e0a91ff03c850def35638a69ec2668b7c015d25d6fed42b92bb
d8fe5f0326 test: improve interface_ipc.py waitNext tests (Ryan Ofsky)
a5e61b1917 test: interface_ipc.py minor fixes and cleanup (Ryan Ofsky)
ded11fb04d test: fix interface_ipc.py template destruction (Ryan Ofsky)
Pull request description:
This PR cleans up the `interface_ipc.py` test, fixing broken checks, fixing missing await calls, removing to_dict calls, renaming variables, reducing `.result` accesses, and giving template objects explicit lifetimes. More details are in the commit messages.
The first commit changes a lot of indentation so is easiest to review ignoring whitespace.
ACKs for top commit:
Sjors:
ACK d8fe5f0326
sedited:
ACK d8fe5f0326
Tree-SHA512: f0de309a15cb23f109cf6909e51ddd132a60bd4d4cb25b20bdc74545516670f1cdb0c9cc98c397c2f24e67e2380c2dac9d00435009618a3c00b6b85cca5c3e2e
09dfa4d3f8 test: fix race condition in p2p_v2_misbehaving.py peerid assertion (stratospher)
Pull request description:
Remove the hard-coded peer id from the debug message in `p2p_v2_misbehaving.py`.
asyncio's non-deterministic task scheduling might cause [peer2](938d7aacab/test/functional/p2p_v2_misbehaving.py (L181))'s connection to happen before [peer1](938d7aacab/test/functional/p2p_v2_misbehaving.py (L179))'s. since we test that peer2 [remains connected](938d7aacab/test/functional/p2p_v2_misbehaving.py (L182)), any disconnection must originate from peer1, making the specific peer id not necessary for test correctness. so we can remove the hard coded peer id from the expected debug log message.
Fixes#34035.
ACKs for top commit:
maflcko:
lgtm ACK 09dfa4d3f8
mzumsande:
Code Review ACK 09dfa4d3f8
Tree-SHA512: 542b08ddae09db7454e8c08b1d26aade50a53c2505683df99556cf071a6a38195b64f8700f6db3f4e1b318497fc4b5232246ad4e9d6f3af45fad83e333fa91fb
fa8a5d215c log: Remove brittle and confusing LogPrintLevel (MarcoFalke)
fac24bbec8 test: Clarify logging_SeverityLevels test (MarcoFalke)
f273167661 ipc: separate log statements per level (stickies-v)
94c51ae540 libevent: separate log statements per level (stickies-v)
Pull request description:
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$LEVEL(...)` macros. Having less ways to achieve the same makes the code more consistent and easier to review.
Fix all issues by removing it
ACKs for top commit:
stickies-v:
re-ACK fa8a5d215c
ajtowns:
ACK fa8a5d215c
pablomartin4btc:
re-ACK fa8a5d215c
Tree-SHA512: 9fbb04962d9c26e566338694a7725b3c0e88ef733322d890bcc6aeddb45266c754e7c885c69bbfebd1588cc09912c6784cfc00e69882f1271a8c87d201490478
due to asyncio's non-deterministic task scheduling, peer2's
connection might happen before peer1's, causing peer2 to get
assigned peer_id=1 on bitcoind side and peer1 to get assigned
peer_id=2 on bitcoind side.
since we test that peer2 remains connected, any disconnection
must originate from peer1, making the specific peer id unnecessary
for test correctness. so we can remove the specific peer_id from
the expected debug log.
fa75480c84 test: Detect truncated download in get_previous_releases.py (MarcoFalke)
Pull request description:
Without this, and end-of-stream is not detected and will just lead to an immediate exit, instead of a re-try.
E.g. https://github.com/bitcoin/bitcoin/actions/runs/20089133013/job/57633839315?pr=34038#step:12:201:
```
...
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.5%
Downloading: [##--------------------------------------] 5.6%
Checksum dd02eab18f9154604e38135ef3f98fd310ba3c748074aeb83a71118cd2cd1367 did not match
Error: Process completed with exit code 1.
```
Also, remove the `0` fallback value, because if the fallback was ever hit, the program would fail anyway with `division by zero` error.
ACKs for top commit:
Sjors:
utACK fa75480c84
rkrux:
Looks fine, ACK fa75480c84
l0rinc:
code review ACK fa75480c84
Tree-SHA512: 230eaf155701ed833636b401118f11ff5c6521c61bf4f3a01fcf390a71a508ba6a570eea855ef659134e118b74f75e3d5772ec8a261db23ebfe4ac7ec87cab5a
e7ac5a133c doc: add release note for 34031 (fanquake)
c4c70a256e netbase: Remove "tor" as a network specification (Carl Dong)
Pull request description:
"tor" as a network specification was deprecated in 60dc8e4208 in favor of "onion"
and this commit removes it and updates the relevant test.
Previously #16029. This has been warning as being deprecated since `v0.17.0`.
This PR only removes the already deprecated usage of tor as a network specification, the use of tor throughout the codebase, is not deprecated.
ACKs for top commit:
davidgumberg:
crACK e7ac5a133c
laanwj:
Code review ACK e7ac5a133c
janb84:
ACK e7ac5a133c
stickies-v:
ACK e7ac5a133c
Tree-SHA512: f211dec151c21728b4cd2b1716ee68907871beaa85d8c89e2bc17576e701d03c03e5455593de94970d787aa3264fab60d8c6debeeff908e00d8feb48804692e9
a1f7623020 qa: Only complain about expected messages that were not found (Hodlinator)
1e54125e2e refactor(qa): Avoid unnecessary string operations (Hodlinator)
a9021101dc qa: Replace always-escaped regexps with "X in Y" (Hodlinator)
5c16e4631c doc: Remove no longer correct comment (Hodlinator)
Pull request description:
* Remove incorrect docstring in `busy_wait_for_debug_log()`.
* Replace nerfed regex searches with `X in Y` expressions.
* Only compute the log string to be printed on failure *when we actually fail* instead of every 0.05s.
* As we find each needle (expected message) in the haystack (log output), stop searching for it. **If we fail and time out, we will only complain about the needles (expected messages) we didn't find. On master we also include found needles, which is less helpful.**
Found while developing a new test case in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351892330
ACKs for top commit:
l0rinc:
Code review ACK a1f7623020
maflcko:
review ACK a1f7623020💨
Tree-SHA512: 191ea7647b0ea8b4220e37c62d176861c2fd0e3737aee3641b262915d9118f48953cf1204767c93a93a8fc78a44c2c29206fb390b44c59d99fc2aa7d12bf4889
4b47113698 validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande)
2f51951d03 p2p: Add warning message when receiving headers for blocks cached as invalid (Martin Zumsande)
Pull request description:
In case of corruption that leads to a block being marked as invalid that is seen as valid by the rest of the network, the user currently doesn't receive good error messages, but will often be stuck in an endless headers-sync loop with no explanation (#26391).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534 on how to trigger it artificially).
- Removes the IBD check from `CheckForkWarningConditions` and adds a call to the function during init (`LoadChainTip()`). The existing check was added in 55ed3f1475 a long time ago when we had more sophisticated fork detection that could lead to false positives during IBD, but that logic was removed in fa62304c97 so that I don't see a reason to suppress the warning anymore.
Fixes#26391 (We'll still do the endless looping, trying to find a peer with a headers that we can use, but will now repeatedly log warnings while doing so).
ACKs for top commit:
glozow:
ACK `git range-diff 6d2c8ea9dbd77c71051935b5ab59224487509559...4b4711369880369729893ba7baef11ba2a36cf4b`
theStack:
re-ACK 4b47113698
sedited:
ACK 4b47113698
Tree-SHA512: 78bc53606374636d616ee10fdce0324adcc9bcee2806a7e13c9471e4c02ef00925ce6daef303bc153b7fcf5a8528fb4263c875b71d2e965f7c4332304bc4d922
a7c96f874d tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() (Chris Stewart)
Pull request description:
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](fb0ada982a/src/validation.cpp (L4180)).
This change was needed in https://github.com/ajtowns/bitcoin/pull/13 which is a soft fork proposal to disallow 64 byte transactions. We want to test that 64 byte transactions serialized without the witness are invalid. If we do not have this change, we cannot directly test the logic that rejects 64 byte transactions.
I decided to PR this upstream as many soft fork proposals may not see the light of day, but this functionality seems strictly additive to the test framework.
ACKs for top commit:
theStack:
ACK a7c96f874d
sedited:
ACK a7c96f874d
glozow:
ACK a7c96f874d
Tree-SHA512: 7c185838abaf068bc96b425c3c971b73f75dfcb41dacc8b2f2543c7602f23f19d908633278b93738f18049e6bd8c845c152cfb93b289bef501c7e86ed8dae0ab
9d5021a05b script: add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY (billymcbip)
Pull request description:
We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`:
- A pre-tapscript policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: 4de26b111f/src/script/interpreter.cpp (L220)
- A [consensus error](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki?plain=1#L93) in Tapscript: 4de26b111f/src/script/interpreter.cpp (L368)
It would be good for readability and testability to have separate errors for both cases, as they are quite distinct (policy vs. consensus, format vs. emptiness).
**This PR adds `SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY` for the consensus error path.**
This change would make our error handling more consistent. We have more granular errors for other pubkey error paths already: `SCRIPT_ERR_WITNESS_PUBKEYTYPE`, `SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE`. We also have separate errors for MINIMAL_IF: `SCRIPT_ERR_MINIMALIF` for the policy error pre-tapscript, and `SCRIPT_ERR_TAPSCRIPT_MINIMALIF` for the consensus error post-tapscript.
Tests:
Added a test case to `script_tests` and ran `build/bin/test_bitcoin --run_test=script_tests --log_level=success`.
```
test/script_tests.cpp:144: info: check '[["aa","#SCRIPT# 0 CHECKSIG","#CONTROLBLOCK#",0.00000001],"","0x51 0x20 #TAPROOTOUTPUT#","P2SH,WITNESS,TAPROOT","TAPSCRIPT_EMPTY_PUBKEY","TAPSCRIPT: OP_CHECKSIG with empty pubkey must fail"] (with flags 165d5d)' has passed
...
```
Ran `DIR_UNIT_TEST_DATA="$(pwd)/../qa-assets/unit_test_data" build/bin/test_bitcoin --run_test=script_assets_tests --log_level=success`.
Updated `feature_taproot.py` and ran `build/test/functional/feature_taproot.py`.
Looking forward to your feedback.
ACKs for top commit:
sedited:
ACK 9d5021a05b
darosior:
utACK 9d5021a05b
sipa:
ACK 9d5021a05b
Tree-SHA512: bc0b7f64454313fe392ffb2d23aa4eca3deadc5ea1d10b3fba0b3ab4cb0575a5ddcb002dc27b4fa7aa3c180840a83d1b3e5c89351009ce7ffe684d58e1980ace
"tor" as a network specification was deprecated in 60dc8e4208 in favor
of "onion" and this commit removes it and updates the relevant test.
Co-authored-by: Mara van der Laan <126646+laanwj@users.noreply.github.com>
As pointed out by Sjors in
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209 and
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386 the
original intention of having waitNext and waitTipChanged calls in the test was
to ensure that if new blocks were connected or fees were increased *during* the
waits, that the calls would wake up and return.
But the tests were written incorrectly, to generate blocks and transactions
before the wait calls instead of during the calls. So the tests were less
meaningful then they should be.
There was also a similar problem in the interruptWait test. The test was
intended to test the interruptWait method, but it was never actually calling
the method due to a missing await keyword. Instead it was testing that
miniwallet.send_self_transfer would interrupt the wait.
This commit fixes these issues by introducing a wait_and_do() helper function
to start parallel tasks and trigger an action after a wait call is started.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
There are a few things that are incorrect or messy in the interface_ipc.py test.
This commit tries to clean them up:
- isTestChain and isInitialBlockDownload asserts were not checking the results
of those calls, only that calls were, made because they were not checking the
responses' .result member.
- A lot of result accesses like `template.result` `mining.result` were repeated
unnecessarily because variables like `template` and `mining` were assigned
response objects instead of result objects. These variables are now changed
to point directly to results.
- Some coroutine calls were assigned to temporary `wait` before being awaited.
This was unnecessarily confusing and would make code not run in top-down
order.
- `to_dict` calls were being made to check if result variables were unset. This
was inefficient and indirect because it iterates over all fields in response
structs instead of just checking whether the result field is present. The
to_dict calls are now replaced with more direct `_has('result')` calls.
- The `res` variables used to hold various responses did not have descriptive
names. These are replaced with clearer names.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
dcd42d6d8f [test] wallet send 3 generation TRUC (glozow)
e753fadfd0 [wallet] never try to spend from unconfirmed TRUC that already has ancestors (glozow)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660
There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in `CommitTransaction`.
This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm.
1ed00a0d39/test/functional/wallet_basic.py (L502-L506)
It's a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it's quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds.
ACKs for top commit:
achow101:
ACK dcd42d6d8f
monlovesmango:
ACK dcd42d6d8f
rkrux:
lgtm ACK dcd42d6d8f
Tree-SHA512: b4cf9685bf0593c356dc0d6644835d53e3d7089f42b65f647795257dc7f5dac90c5ee493b41ee30a1c1beb880a859db8e049d3c64a43d5ca9b3e6482ff6bddd5
Use context managers to destroy block templates. Previously, block templates
were not being destroyed before disconnecting because the destroy coroutines
were called but never awaited. It's not necessary to explicitly destroy the
templates since they will get garbage collected asynchronously, but it's good
to destroy them to make the test more predictable, and to make the destroy
calls that are present actually do something.
This change also removes `await waitnext` expressions without changing
behavior, because the previous code was misleading about what order waitNext
calls were executed.
This change is easiest to review ignoring whitespace.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
fa6db79302 test: Avoid shutdown race in NetworkThread (MarcoFalke)
Pull request description:
Locally, I am seeing rare intermittent exceptions in the network thread:
```
stderr:
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 744, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 603, in run_forever
self._run_once()
File "/python3.10/asyncio/base_events.py", line 1871, in _run_once
event_list = self._selector.select(timeout)
AttributeError: 'NoneType' object has no attribute 'select'
```
I can reproduce this intermittently via `while ./bld-cmake/test/functional/test_runner.py $(for i in {1..400}; do echo -n "tool_rpcauth "; done) -j 400 ; do true ; done`.
I suspect this is a race where the shutdown starts the close of the network thread while it is starting.
A different exception showing this race can be reproduced via:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 610aa4ccca..64561e157c 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -741,6 +741,7 @@ class NetworkThread(threading.Thread):
def run(self):
"""Start the network thread."""
+ import time;time.sleep(.1)
self.network_event_loop.run_forever()
def close(self, *, timeout=10):
```
It is trivial to reproduce via any test (e.g. `./bld-cmake/test/functional/tool_rpcauth.py`) and shows a similar traceback to the one above:
```
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 745, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 591, in run_forever
self._check_closed()
File "/python3.10/asyncio/base_events.py", line 515, in _check_closed
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
```
So fix the second runtime error in hope of fixing the first one as well.
ACKs for top commit:
brunoerg:
code review ACK fa6db79302
Tree-SHA512: ca352ebf7929456ea2bbfcfe4f726adcbfcfb3dc0edeaddae7f6926f998888f0bd8b987ddef60308266eeab6bffa7ebdc32f5908db9de5404df95635dae4a8f6
cb7d5bfe4a test, assumeutxo: loading a wallet (backup) on a pruned node (Alfonso Roman Zubeldia)
7a365244f8 test, refactor snapshot import and background validation (Alfonso Roman Zubeldia)
Pull request description:
Adding tests in `./test/functional/wallet_assumeutxo.py` to cover the following scenario:
- test loading a wallet (backup) on a pruned node
ACKs for top commit:
fjahr:
re-ACK cb7d5bfe4a
theStack:
re-ACK cb7d5bfe4a
Tree-SHA512: 88cc419f340d31e80120e0c6cafe567efc678df27576db6e08aeab62d2b50ed1153d56f3f3343e9bae49262e38f9fb81db7769f02a4a01e4ef25c5d029c12323
fad6118586 test: Fix "typo" in written invalid content (MarcoFalke)
fab085c15f contrib: Use text=True in subprocess over manual encoding handling (MarcoFalke)
fa71c15f86 scripted-diff: Bump copyright headers after encoding changes (MarcoFalke)
fae612424b contrib: Remove confusing and redundant encoding from IO (MarcoFalke)
fa7d72bd1b lint: Drop check to enforce encoding to be specified in Python scripts (MarcoFalke)
faf39d8539 test: Clarify that Python UTF-8 mode is the default today for most systems (MarcoFalke)
fa83e3a81d lint: Do not allow locale dependent shell scripts (MarcoFalke)
Pull request description:
Historically, there was an attempt via `test/lint/lint-python-utf8-encoding.py` to enforce explicit UTF8 in every Python IO statement (`open`, `subprocess`, ...). However, the lint check has many problems:
* The check is incomplete and many IO statements lack the explicit UTF8 specification.
* It was added at a time when some systems were not UTF8 by default.
* The check is brittle, as it depends on a fragile regex.
In theory, now that the minimum Python version is 3.10 (since commit 2123c94448), the check could be replaced by `PYTHONWARNDEFAULTENCODING=1` from https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option. However, this comes with many other problems:
* All our Python scripts already assume and require UTF8 to be set externally. On almost all modern systems, this is already the default. Some Windows versions do not have UTF8 by default and require `PYTHONUTF8=1` to be set for the tests to run already today (with or without the changes in this pull). Also, the CI and many other Bash scripts force UTF8 via `LC_ALL`. Finally, Python 3.15 will likely enable UTF8 on *all* systems by default, per https://peps.python.org/pep-0686/#abstract.
* So adding UTF8 to every single IO call is redundant, verbose, and confusing, given that it is the expected default.
So fix all issues, by:
* Removing the `test/lint/lint-python-utf8-encoding.py` check.
* Removing the encoding on the individual IO calls.
* Clarifying the existing docs around the existing UTF8 requirement and assumption.
Obviously, every IO call is still free to specify UTF8 or any other encoding explicitly, if there is a documented need for it in the future.
ACKs for top commit:
theStack:
re-ACK fad6118586
laanwj:
Re-ACK fad6118586
Tree-SHA512: 78025ea3508597d2299490347614f0ee3e4c66e3ba559ff50e498045a9c8bbd92f3a5ced18719d8fcebbd1e47bdbb56a0c85a5b73b425adb0ea4f02fe69c3149
The existing IBD disable was added at a time when CheckForkWarningConditions
did also sophisticated fork detection that could lead to false positives
during IBD (55ed3f1475).
The fork detection logic doesn't exist anymore
(since fa62304c97), so the IBD check is no
longer necessary.
Displaying the log at startup will help node operators diagnose the
problem better.
Also unify log message and alert warning text, since a long invalid chain
could be due to chainstate corruption or an actual consensus incompatibility
with peers. Previously the log assumed the former and the alert the latter.
fa45a1503e log: Use LogWarning for non-critical logs (MarcoFalke)
fa0018d011 log: Use LogError for fatal errors (MarcoFalke)
22229de728 doc: Fix typo in init log (MarcoFalke)
Pull request description:
Logging supports severity levels above info via the legacy `LogPrintf`. So use the more appropriate `LogError` or `LogWarning`, where it applies.
This has a few small benefits:
* It often allows to remove the manual and literal "error: ", "Warning:", ... prefixes. Instead the uniform log level formatting is used.
* It is easier to grep or glance for more severe logs, which indicate some kind of alert.
* `LogPrintf` didn't indicate any severity level, but it is an alias for `LogInfo`. So having the log level explicitly spelled out makes it easier to read the code.
* Also, remove the redundant trailing `\n` newline, while touching.
* Also, remove the `__func__` formatting in the log string, which is redundant with `-logsourcelocations`. Instead, use a unique log string for each location.
ACKs for top commit:
l0rinc:
Code review ACK fa45a1503e
stickies-v:
ACK fa45a1503e
rkrux:
crACK fa45a1503e
Tree-SHA512: 516d439c36716f969c6e82d00bcda03c92c8765a9e41593b90052c86f8fa3a3dacbb2c3dc98bfc862cefa54cae34842b488671a20dd86cf1d15fb94aa5563406
b8d279a81c doc: add comment to explain correctness of GatherClusters() (Suhas Daftuar)
aba7500a30 Fix parameter name in getmempoolcluster rpc (Suhas Daftuar)
6c1325a091 Rename weight -> clusterweight in RPC output, and add doc explaining mempool terminology (Suhas Daftuar)
bc2eb931da Require mempool lock to be held when invoking TRUC checks (Suhas Daftuar)
957ae23241 Improve comments for getTransactionAncestry to reference cluster counts instead of descendants (Suhas Daftuar)
d97d6199ce Fix comment to reference cluster limits, not chain limits (Suhas Daftuar)
a1b341ef98 Sanity check feerate diagram in CTxMemPool::check() (Suhas Daftuar)
23d6f457c4 rpc: improve getmempoolcluster output (Suhas Daftuar)
d2dcd37aac Avoid using mapTx.modify() to update modified fees (Suhas Daftuar)
d84ffc24d2 doc: add release notes snippet for cluster mempool (Suhas Daftuar)
b0417ba944 doc: Add design notes for cluster mempool and explain new mempool limits (Suhas Daftuar)
2d88966e43 miner: replace "package" with "chunk" (Suhas Daftuar)
6f3e8eb300 Add a GetFeePerVSize() accessor to CFeeRate, and use it in the BlockAssembler (Suhas Daftuar)
b5f245f6f2 Remove unused DEFAULT_ANCESTOR_SIZE_LIMIT_KVB and DEFAULT_DESCENDANT_SIZE_LIMIT_KVB (Suhas Daftuar)
1dac54d506 Use cluster size limit instead of ancestor size limit in txpackage unit test (Suhas Daftuar)
04f65488ca Use cluster size limit instead of ancestor/descendant size limits when sanity checking TRUC policy limits (Suhas Daftuar)
634291a7dc Use cluster limits instead of ancestor/descendant limits when sanity checking package policy limits (Suhas Daftuar)
fc18ef1f3f Remove ancestor and descendant vsize limits from MemPoolLimits (Suhas Daftuar)
ed8e819121 Warn user if using -limitancestorsize/-limitdescendantsize that the options have no effect (Suhas Daftuar)
80d8df2d47 Invoke removeUnchecked() directly in removeForBlock() (Suhas Daftuar)
9292570f4c Rewrite GetChildren without sets (Suhas Daftuar)
3e39ea8c30 Rewrite removeForReorg to avoid using sets (Suhas Daftuar)
a3c31dfd71 scripted-diff: rename AddToMempool -> TryAddToMempool (Suhas Daftuar)
a5a7905d83 Simplify removeRecursive (Suhas Daftuar)
01d8520038 Remove unused argument to RemoveStaged (Suhas Daftuar)
bc64013e6f Remove unused variable (cacheMap) in mempool (Suhas Daftuar)
Pull request description:
As suggested in the main cluster mempool PR (https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3177119367), I've pulled out some of the non-essential optimizations and cleanups into this separate PR.
Will continue to add more commits here to address non-blocking suggestions/improvements as they come up.
ACKs for top commit:
instagibbs:
ACK b8d279a81c
sipa:
ACK b8d279a81c
Tree-SHA512: 1a05e99eaf8db2e274a1801307fed5d82f8f917e75ccb9ab0e1b0eb2f9672b13c79d691d78ea7cd96900d0e7d5031a3dd582ebcccc9b1d66eb7455b1d3642235
Also improve test coverage for removeForReorg by creating a scenario where
there are in-mempool descendants that are only invalidated due to an in-mempool
parent no longer spending a mature coin.
Empty public keys in tapscript are rejected by consensus rules, independent of SCRIPT_VERIFY_STRICTENC. Add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY to distinguish this from STRICTENC policy failures currently reported as SCRIPT_ERR_PUBKEYTYPE.
All touched Python scripts already assume and require UTF8, so manually
specifying encoding or decoding for functions in the subprocess module
is redundant to just using text=True, which exists since Python 3.7
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
The encoding arg is confusing, because it is not applied consistently
for all IO.
Also, it is useless, as the majority of files are ASCII encoded, which
are fine to encode and decode with any mode.
Moreover, UTF-8 is already required for most scripts to work properly,
so setting the encoding twice is redundant.
So remove the encoding from most IO. It would be fine to remove from all
IO, however I kept it for two files:
* contrib/asmap/asmap-tool.py: This specifically looks for utf-8
encoding errors, so it makes sense to sepecify the utf-8 encoding
explicitly.
* test/functional/test_framework/test_node.py: Reading the debug log in
text mode specifically counts the utf-8 characters (not bytes), so it
makes sense to specify the utf-8 encoding explicitly.
The check was incomplete and brittle. A better check would be to enable
`PYTHONWARNDEFAULTENCODING=1`
https://docs.python.org/3/whatsnew/3.10.html#optional-encodingwarning-and-encoding-locale-option
However, it is unclear what the goal of adding explicit encodings
everywhere is, given that:
* Most modern systems already have UTF-8 enabled by default, except for
Windows.
* Python 3.15 will likely enable it globally by default, according to
https://peps.python.org/pep-0686/#abstract
* Adding the explicit encodings will bloat all code for no benefit.
So remove the lint check and drop all redundant encoding= kwargs.
All encoding= that are set for a reason, are kept.
It will likely be the default for all systems, starting with Python
3.15, according to https://peps.python.org/pep-0686/#abstract.
It is hard to find a system other than Windows that has it not enabled
today. Nonetheless, Bitcoin Core requires UTF-8 in scripts and normally
enforces it via LC_ALL=C.UTF-8 or PYTHONUTF8=1.
Bash is discouraged, and there was never a need to write locale
dependent Bash.
So remove the option and clarify that the LC_ALL settings enable UTF-8
mode in Python.
