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
49c6728535 cmake: Set `WITH_ZMQ` to `ON` in Windows presets (Hennadii Stepanov)
Pull request description:
The "zeromq" feature is already enabled by default in `vcpkg.json`, and there appears to be no reason to omit this configuration option when building on Windows.
ACKs for top commit:
maflcko:
lgtm ACK 49c6728535
Tree-SHA512: acaef1eba56e75f9979db1809c6ebac59b2ed49002ae557fcb172f6119f6b8927580583616628f24d71ccbb32544f4d96317ff1d3125a9b5446ae89d1d318de0
Also update the help text for -limitancestorcount/-limitdescendantcount to
explain they no longer affect the mempool, and are only used by the wallet for
coin selection.
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.
The "zeromq" feature is already enabled by default in `vcpkg.json`, and
there appears to be no reason to omit this configuration option when
building on Windows.
2e27bd9c3a ci: Add Windows + UCRT jobs for cross-compiling and native testing (Hennadii Stepanov)
bd130db994 ci: Rename items specific to Windows + MSVCRT (Hennadii Stepanov)
Pull request description:
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.
For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593
MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
ACKs for top commit:
maflcko:
review ACK 2e27bd9c3a 🖊
fanquake:
ACK 2e27bd9c3a
Tree-SHA512: 222ca5e54646bcce9db6e20191d5891e988274e18b2f30085de6435a3b288a9d0fc414e8f76342e275ae58ee6603f751933d1faa8bdff446edf2695091f8ca4c
