The test was a bit confusing, because it just referred to the "global
log level" without explicitly specifying what it is. The level is set
though the LogSetup constructor. However, it is easier to follow unit
tests, if they are self-contained. So just set the level to Debug
explicitly here.
Also, add a new debug_3 log, to further document the intended behavior
of the unit test.
Also, replace the LogPrintLevel with the shorter and exact replacements
LogTrace and LogDebug.
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "ipc:".
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "libevent:".
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
faa23738fc refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke)
fa114be27b Add util::Expected (std::expected) (MarcoFalke)
Pull request description:
Some low-level code could benefit from being able to use `std::expected` from C++23:
* Currently, some code is using `std::optional<E>` to denote an optional error. This is fine, but a bit confusing, because `std::optional` is normally used for values, not errors. Using `std::expected<void, E>` is clearer.
* Currently, some code is using `std::variant<V, E>` to denote either a value or an error. This is fine, but a bit verbose, because `std::variant` requires a visitor or get_if/holds_alternative instead of a simple call of the `operator bool` for `std::expected`.
In theory, `util::Result` could be taught to behave similar to `std::expected` (see https://github.com/bitcoin/bitcoin/pull/34005). However, it is unclear if this is the right approach:
* `util::Result` is mostly meant for higher level code, where errors come with translated error messages.
* `std::expected` is mostly meant for lower level code, where errors could be an enum, or any other type.
* https://github.com/bitcoin/bitcoin/pull/25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. `std::expected` requires the value and error to be "nested within it" (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible.
* `std::expected` also comes with `std::unexpected`, which also does not map cleanly to `util::Result`.
So just add a minimal drop-in port of `std::expected`.
ACKs for top commit:
romanz:
tACK faa23738fc
sedited:
Re-ACK faa23738fc
hodlinator:
ACK faa23738fc
rkrux:
light Code Review ACK faa23738fc
ryanofsky:
Code review ACK faa23738fc, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.
stickies-v:
ACK faa23738fc
Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872
48840bfc2d refactor: Prefer `<=>` over multiple relational operators (Daniel Pfeifer)
5a0f49bd26 refactor: Remove all `operator!=` definitions (Daniel Pfeifer)
Pull request description:
Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before.
The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages:
1. less code
2. guaranteed consistency
Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR.
ACKs for top commit:
optout21:
utACK 48840bfc2d
Chand-ra:
tACK [`48840bf`](48840bfc2d). Built the PR and ran unit tests; everything passes.
maflcko:
review ACK 48840bfc2d🌖
stickies-v:
utACK 48840bfc2d. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
janb84:
ACK 48840bfc2d
sipa:
ACK 48840bfc2d
Tree-SHA512: 7fedc4abc451c7ad611e3a960ff939a35580667222009cb30ca546e564dc9161e3e8d4d1d7d44c538d961cc8f7adba6e6dbcebcd1be370bf33aef294d06f236b
41e657aacf guix: add bitcoin-qt runtime libs doc in symbol-check (fanquake)
ef4ce19a15 depends: freetype 2.11.1 (fanquake)
Pull request description:
Update freetype to `2.11.1`.
Updating fontconfig (currently `2.12.6`) to `2.13.1` requires what looks like a hard dep on gperf; leaving that as-is for now.
Document expectations in `symbol-check.py`.
Closes#29977 (changes are based on discussion there).
ACKs for top commit:
sedited:
ACK 41e657aacf
Tree-SHA512: 71c4ccc442df0b90bebc475003eb325564111b8312c42bc7d7a9c81a2fc166fdc0814c9ddde3cfe562c3c835556e7f97107458b02a07b981b1a199bf65d5ac1d
7b90b4f5bb guix: reduce allowed exported symbols (fanquake)
Pull request description:
Need to double-check, but pretty sure this is atleast partly from #33181.
ACKs for top commit:
sedited:
Nice, ACK 7b90b4f5bb
Tree-SHA512: 538c03dc32aab9b3e18100e8ffa0d664aea5ceba6aafee9e8e0894c2d02eea3b3fb09733cf7b5bd0aefb6b56d0ac3b92f28da932e135b23f55404efd8f43664a
fa4395dffd refactor: Remove unused LogPrintf (MarcoFalke)
fa05181d90 scripted-diff: LogPrintf -> LogInfo (MarcoFalke)
Pull request description:
`LogPrintf` has many issues:
* It does not mention the log severity (info).
* It is a deprecated alias for `LogInfo`, according to the dev notes.
* It wastes review cycles, because reviewers sometimes point out that it is deprecated.
* It makes the code inconsistent, when both versions of the alias are used.
Fix all issues by removing the deprecated alias.
ACKs for top commit:
ajtowns:
ACK fa4395dffd
stickies-v:
ACK fa4395dffd
rkrux:
lgtm ACK fa4395dffd
Tree-SHA512: de95d56df27b9ee33548cc7ee7595e2d253474094473089ee67787ddb171384383c683142672c3e2c1984e19eee629b2c469dc85713640a73391610581edbdbe
57b888ce0e fuzz: Add a test case for `ParseByteUnits()` (Chandra Pratap)
Pull request description:
`ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized `default_multiplier`.
ACKs for top commit:
maflcko:
lgtm ACK 57b888ce0e
dergoegge:
utACK 57b888ce0e
marcofleon:
crACK 57b888ce0e
Tree-SHA512: c16557442987437e5e0c9d9a8b016df93e513e34acb78242a1f73dabc4482632ec57eb35cb4c84f9a1ea838fa6bda2094f2a8b52ace431f8064a79fad96e9a52
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
710031ebef Revert "guix: sqlite wants tcl" (Hennadii Stepanov)
4cf5ea6c3d depends: Propagate native C compiler to `sqlite` package (Hennadii Stepanov)
Pull request description:
This PR:
1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system.
2. Removes the `tcl` package from the Guix manifest.
This is an alternative to https://github.com/bitcoin/bitcoin/pull/33975.
ACKs for top commit:
fanquake:
ACK 710031ebef
sedited:
ACK 710031ebef
Tree-SHA512: bdaa29af977799669bfc2aa3a8d0a4a688263b99c5f06b1582fbefb71ef77be0ee6223903e8357e51a9e0a7744807174b94262c2f4a3afd9f39737b61b00863e
fd4ce55121 contrib: Count entry differences in asmap-tool diff summary (Fabian Jahr)
Pull request description:
Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list.
Example output currently:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed
```
This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugging of certain issues also the more relevant value. This PR adds the count of changed entries to this summary output at the end. There as also a bit more structure so it's easier to parse as well.
Example new output:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# Summary
IPv4: 0 entries with 0 addresses changed
IPv6: 12 entries with 79552154633921058212365205504 (2^96.01) addresses changed
```
ACKs for top commit:
jurraca:
utACK [`fd4ce55121`](fd4ce55121)
janb84:
utACK fd4ce55121
hodlinator:
ACK fd4ce55121
Tree-SHA512: 97cc543eaba80a33f0291b20630411bda869d3b8d1b35ed7f36792064cb1edccc8fe4740b7229b5451a88b7bd8d68c42f96829ce4255ecac3e29d70b68061608
`ParseByteUnits()` is the only parsing function in `strencodings.cpp`
lacking a fuzz test. Add a test case to check the function against
arbitrary strings and randomized default_multiplier's.
ffcae82a68 test: exercise TransactionMerklePath with empty block; targets the MerkleComputation empty-leaves path that was only reached by fuzz tests (frankomosh)
Pull request description:
As noted in [#32243 (comment)](https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2988854482), the early return inside `MerkleComputation` when `leaves.size() == 0` was only exercised by fuzz tests.
The existing `merkle_test_empty_block` calls `BlockMerkleRoot`, which uses `ComputeMerkleRoot`, but does not exercise the `TransactionMerklePath` → `ComputeMerklePath` → `MerkleComputation` code path.
Coverage before adding test:
<img width="2459" height="66" alt="before" src="https://github.com/user-attachments/assets/ca94015a-d7c2-4281-ac60-13b22f177b67" />
Coverage after adding test:
<img width="2459" height="66" alt="after" src="https://github.com/user-attachments/assets/b1d4e1bb-af72-46ab-8898-f18db39dd2fb" />
ACKs for top commit:
kevkevinpal:
ACK [ffcae82](ffcae82a68)
maflcko:
lgtm ACK ffcae82a68
brunoerg:
code review ACK ffcae82a68
sedited:
ACK ffcae82a68
Tree-SHA512: d2499d91269c4f4f9a86011f7ad13f675834662a5bd37b0e7cbe887a7d9acf4170e53f0bdc528011fc82866b9c1dec34f4e7e9cd64cc3100591c1580a4df5d00
167df7a98c net: fix use-after-free with v2->v1 reconnection logic (Eugene Siegel)
Pull request description:
`CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a `grant` member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations.
I was able to reproduce the original issue https://github.com/bitcoin/bitcoin/issues/33615 with the following diff by randomly stopping my node while it was attempting to reconnect (and verified that this patch fixes the issue, at least in my ~40-50 runs):
<details>
<summary> diff </summary>
```diff
diff --git a/src/net.cpp b/src/net.cpp
index ef1c63044a..9c1d161d8b 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1918,8 +1918,8 @@ void CConnman::DisconnectNodes()
{
LOCK(m_nodes_mutex);
- const bool network_active{fNetworkActive};
- if (!network_active) {
+// const bool network_active{fNetworkActive};
+// if (!network_active) {
// Disconnect any connected nodes
for (CNode* pnode : m_nodes) {
if (!pnode->fDisconnect) {
@@ -1927,7 +1927,7 @@ void CConnman::DisconnectNodes()
pnode->fDisconnect = true;
}
}
- }
+// }
// Disconnect unused nodes
std::vector<CNode*> nodes_copy = m_nodes;
@@ -1941,7 +1941,7 @@ void CConnman::DisconnectNodes()
// Add to reconnection list if appropriate. We don't reconnect right here, because
// the creation of a connection is a blocking operation (up to several seconds),
// and we don't want to hold up the socket handler thread for that long.
- if (network_active && pnode->m_transport->ShouldReconnectV1()) {
+ if (true) {
reconnections_to_add.push_back({
.addr_connect = pnode->addr,
.grant = std::move(pnode->grantOutbound),
```
</details>
I'm curious to see if others can reproduce as well.
ACKs for top commit:
dergoegge:
Code review ACK 167df7a98c
darosior:
utACK 167df7a98c
mzumsande:
ACK 167df7a98c
Tree-SHA512: 33fdfb110a7cdae182b5cd5400eea8a271308a62dd56491e0aef8865eff24a9ea908be74e4e2e2ee00ac1cb698e46f270f56dffffe34cf2cfd79e9b1079d6531
b0c706795c Remove unreliable seed from chainparams.cpp, and the associated README (SatsAndSports)
Pull request description:
The DNS seed `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` is not returning a representative sample of bitcoin nodes. It currently returns nothing later than 28.1.0, breaching the policy.
This PR removes that seed from the list of DNS seeds
### Rationale
The [policy for seeds](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) includes this:
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network
A number of comments below, in response to this PR, include apparent breaches of this policy: [1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231) [2](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457655364), [3](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457712557), in particular the first linked comment ([1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231)) comparing the distribution at this seed to other seeds. This seed is not including anything later than 28.2.0, breaching this policy.
To ensure the policy is followed, and the seeds include a representative sample of Bitcoin nodes, this PR removes this seed from the list
### Data
I ran this:
```
# Get some ip address from that seed:
# Repeated multiple times, to get many different IPs:
dig +short dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us >> dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us
# For each distinct ip gathered from the seed, get basic info about the node, including it's User Agent string:
cat dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us | sort -u | while read ip; do echo ===; echo $ip; nmap -p 8333 --script bitcoin-info "$ip"; done > seed_versions.txt
```
and then summarized the agents with `egrep 'User Agent' seed_versions.txt | sort | uniq -c` and got:
```
1 User Agent: /Satoshi:22.0.0/
1 User Agent: /Satoshi:22.1.0/
5 User Agent: /Satoshi:24.0.1/
1 User Agent: /Satoshi:25.1.0/
30 User Agent: /Satoshi:27.0.0/
1 User Agent: /Satoshi:27.1.0/
1 User Agent: /Satoshi:27.1.0/Knots:20240801/
1 User Agent: /Satoshi:28.0.0/
7 User Agent: /Satoshi:28.1.0/
2 User Agent: /Satoshi:28.1.0/Knots:20250305/
```
ACKs for top commit:
l0rinc:
reACK b0c706795c
delta1:
reACK b0c706795c
Crypt-iQ:
crACK b0c706795c
laanwj:
ACK b0c706795c
murchandamus:
ACK b0c706795c
RandyMcMillan:
ACK b0c7067
wiz:
ACK b0c706795c
dergoegge:
ACK b0c706795c
stickies-v:
re-ACK b0c706795c
mzumsande:
ACK b0c706795c
instagibbs:
ACK b0c706795c
Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
866bbb98fd cmake, test: Improve locality of `bitcoin_ipc_test` library description (Hennadii Stepanov)
ae2e438b25 cmake: Move IPC tests to `ipc/test` (Hennadii Stepanov)
Pull request description:
This PR follows up on https://github.com/bitcoin/bitcoin/pull/33445 and:
1. Organizes the IPC tests in the same way as the wallet tests.
2. Removes no longer needed `src/test/.clang-tidy.in`.
See the previous discussion:
- https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340
- https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329
Additionally, the locality of the `bitcoin_ipc_test` build target description has been improved.
ACKs for top commit:
Sjors:
ACK 866bbb98fd
janb84:
ACK 866bbb98fd
ryanofsky:
Code review ACK 866bbb98fd, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.
Tree-SHA512: ed7cc817ccb88595d8516978bff0ea2560048d35b3f548e7913aec7d58b8d6ac550e230e992c527fb747bef175580be92dc4df6342e4485f3a9870dba0a25cba
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
e9536faaee contrib: fix manpage generation (fanquake)
Pull request description:
0972f55040 from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`.
ACKs for top commit:
janb84:
re ACK e9536faaee
rkrux:
re-ACK e9536faaee
Tree-SHA512: 471b1800beeec3ea70d722ac2dcc26073805c8fcdf0418ceb79728cc001eb7c2f11a3d832b54a7ae68d26fe5c97934a9c87eedae7601515857e660fac7532c0a
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
0972f55040 from #33229 broke manpage
generation, because the assumption that the last word in the line
containing the version number, was the version number, no-longer holds
for some binaries. i.e bitcoind.
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
804329400a fuzz: gate mempool entry based on weight (Greg Sanders)
Pull request description:
The mempool implementation now uses TxGraph with entries using FeePerWeight, not vsize. This means our package_rbf harness will erroneously add more transaction weight than we can support inside of FeeFrac. Gate more aggressively using WITNESS_SCALE_FACTOR.
Fixes https://github.com/bitcoin/bitcoin/issues/33981
ACKs for top commit:
sdaftuar:
ACK 804329400a
ismaelsadeeq:
utACK 804329400a
dergoegge:
utACK 804329400a
Tree-SHA512: e78d0f73f9b9cbb8c0db1e8e91dbffeb4110cf8113e90f34af5c132acf0819c54254891a4dd5da63016e4edf9d8e886f469f959bd3504b7deb66989d96fe4cf1
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
The mempool implementation now uses TxGraph with entries
using FeePerWeight, not vsize. This means our package_rbf
harness will erroneously add more transaction weight than we
can support inside of FeeFrac. Gate more aggressively using
WITNESS_SCALE_FACTOR.
fe1815d48f cmake: Make `BUILD_KERNEL_TEST` depend on `BUILD_KERNEL_LIB` (Hennadii Stepanov)
Pull request description:
The CMake script in the `test/kernel` subdirectory is already gated by `BUILD_KERNEL_LIB`:f6acbef108/src/CMakeLists.txt (L405-L409)
As a result, the following configuration summary is misleading:
```
$ cmake -B build -DBUILD_KERNEL_LIB=OFF -DBUILD_KERNEL_TEST=ON
<snip>
bitcoin-chainstate (experimental) ... OFF
libbitcoinkernel (experimental) ..... OFF
kernel-test (experimental) .......... ON
<snip>
```
This PR fixes the behaviour by making the `BUILD_KERNEL_TEST` option explicitly depend on `BUILD_KERNEL_LIB`.
ACKs for top commit:
maflcko:
lgtm ACK fe1815d48f
sedited:
ACK fe1815d48f
Tree-SHA512: 24524d43b195b0e3907f3257ef907c5ead8e9921b888bc82765f4dbbe44728b92956233c8fe624e8509bf8146a41cf8c1ac26f6043b8a21f681ad2ae19bebc5d
