015cc5e588fa25f65f6ea2ed03701def8dfd5444 lint: stop ignoring LIEF imports (fanquake)
Pull request description:
Type stubs are now available as of 0.13.0.
See https://github.com/lief-project/LIEF/issues/650.
ACKs for top commit:
TheCharlatan:
ACK 015cc5e588fa25f65f6ea2ed03701def8dfd5444
Tree-SHA512: ebb754f293c2a61a0ef64c3552f7c700ceb3054b50fd3f1573e4a9e87773ddeba47bd9875f6ab055043012dbc20aeb71e4d76cd3da535c76651dfb1fbfc66e89
59c89447499bd9d6202269879555b8bc37373aa2 build: disable boost multi index safe mode (willcl-ark)
Pull request description:
Fixes#27586
Disable boost multi index safe mode by default when configuring with
--enable-debug.
This option can cause transactions to take a long time to be accepted
into the mempool under certain conditions; iterator destruction takes
O(n) time vs O(1) as they are stored in a singly linked list. See
27586 and the [boost docs](https://www.boost.org/doc/libs/1_58_0/boost/multi_index/detail/safe_mode.hpp) for more information.
Re-enable it on the CI builds which previously had it enabled.
Re-enable it on the msan fuzz task so that we have fuzz tasks testing
with it enabled and disabled in this repo.
ACKs for top commit:
hebasto:
~ACK 59c89447499bd9d6202269879555b8bc37373aa2~
fanquake:
ACK 59c89447499bd9d6202269879555b8bc37373aa2
Tree-SHA512: ed654f63dbebdd02e4414d1f81147d92a4d490dbb5a2e0376858e3129097645f3a2df45191d6b40c410a76e803b0d28796d1a01c1d2fd995b94e8b7eb3949027
1f97572b9c0d339a8497340e7066050aba9d7694 Fix `#include`s in `src/wallet` (Hennadii Stepanov)
Pull request description:
This PR is a minimum required changes to fix https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290.
ACKs for top commit:
MarcoFalke:
lgtm ACK 1f97572b9c0d339a8497340e7066050aba9d7694
Tree-SHA512: de885210076d23f3394c42ca50e6ae2470c0ae6523399a2fa3ebb7c06383bdacef9c26166fa19747200396bed796c8772165e24416eb30ed8edd024e3394b2fe
689a65d878638ae67b07f111dd77ea3c624e4f58 contrib/init: Better systemd integration (Carl Dong)
Pull request description:
```
1. Make logs available to journalctl (systemd's logging system) by not
specifying -daemonwait, which rightfully has its own set of stdout
and stderr descriptors (a user invoking with -daemonwait on the
command line should not see any logs). It makes more sense not to
daemonize in the systemd context anyway.
2. Make systemd aware of when bitcoind is started and in steady state by
specifying -startupnotify='systemd-notify --ready' and Type=notify.
NotifyAccess=all is necessary so that the spawned thread for
startupnotify is allowed to inform systemd of bitcoind's readiness.
Note that NotifyAccess=exec won't work because it only allows
sd_notify readiness signalling from Exec*= declarations in the
.service file.
Note that we currently don't allow multiple startupnotify commands, but
users can override it in systemd via:
# systemctl edit bitcoind
By specifying something like:
[Service]
ExecStart=/usr/bin/bitcoind -pid=/run/bitcoind/bitcoind.pid \
-conf=/etc/bitcoin/bitcoin.conf \
-datadir=/var/lib/bitcoind \
-startupnotify='systemd-notify --ready; mycommandhere'
```
ACKs for top commit:
real-or-random:
ACK 689a65d878638ae67b07f111dd77ea3c624e4f58 tested this service file with 25.0
Tree-SHA512: 9a52ad5cf25886c0d8dabc986d8920602a056db25875b5edd910b387043b78bb78c76d6df82e6e322e3be3bfd5c35c80721cbc8308cec946060bd7586820e9c6
fa6b11a55663e70369bfbbba5fccc55b33f2b310 test: Throw error when -signetchallenge is non-hex (MarcoFalke)
Pull request description:
Instead of silently parsing non-hex to an empty challenge, throw an error.
Also, add missing includes while touching the file.
ACKs for top commit:
kevkevinpal:
ACK [fa6b11a](fa6b11a556)
kallewoof:
ACK fa6b11a
TheCharlatan:
Nice, ACK fa6b11a55663e70369bfbbba5fccc55b33f2b310
Tree-SHA512: 018ebbbf819ba7cdf0c6dd294fdfaa5ddb81b87058a8b9c57b96066d5b07e1656fd78f18e3cef375aebefa191fa515c2c70bc764880fa05f98f526334431a616
fa12558d21aa03c22407a1458a0345d8a7ab6a4b ci: Avoid leaking HOME var into CI pod (MarcoFalke)
aaaa4326035c889a7be59cfe738986ae5581d5ac ci: Remove "default" test env (MarcoFalke)
fa7a87bc7c3468024c6aa05cda23c575b1cf707a ci: Add missing set -e to 01_base_install.sh (MarcoFalke)
Pull request description:
Otherwise errors are silently ignored
ACKs for top commit:
TheCharlatan:
ACK [fa12558](fa12558d21)
hebasto:
ACK fa12558d21aa03c22407a1458a0345d8a7ab6a4b
Tree-SHA512: dbf3f16302c83973b78f3a5e7793090bc9ac44fdf20d51a26b30a99a97369971661e9aed1cd810d80d49d60009651ca0a8aeb2bdc24198a143bf4fff0ec89901
1. Make logs available to journalctl (systemd's logging system) by not
specifying -daemonwait, which rightfully has its own set of stdout
and stderr descriptors (a user invoking with -daemonwait on the
command line should not see any logs). It makes more sense not to
daemonize in the systemd context anyway.
2. Make systemd aware of when bitcoind is started and in steady state by
specifying -startupnotify='systemd-notify --ready' and Type=notify.
NotifyAccess=all is necessary so that the spawned thread for
startupnotify is allowed to inform systemd of bitcoind's readiness.
Note that NotifyAccess=exec won't work because it only allows
sd_notify readiness signalling from Exec*= declarations in the
.service file.
3. Also make systemd aware of when bitcoind is stopping by specifying
-shutdownnotify='systemd-notify --stopping'
Note that we currently don't allow multiple *notify commands, but users
can override it in systemd via:
# systemctl edit bitcoind
By specifying something like:
[Service]
ExecStart=/usr/bin/bitcoind -pid=/run/bitcoind/bitcoind.pid \
-conf=/etc/bitcoin/bitcoin.conf \
-datadir=/var/lib/bitcoind \
-startupnotify='systemd-notify --ready; mystartupcommandhere' \
-shutdownnotify='systemd-notify --stopping; myshutdowncommandhere'
89df7987c2f1eea42454c2b0efc31a924fbfd3a8 Add wallets_conflicts (Antoine Riard)
dced203162d45e542f187f8d0d07dab85c52cf28 wallet, tests: mark unconflicted txs as inactive (ishaanam)
096487c4dcfadebe5ca959927f5426cae1c304d5 wallet: introduce generic recursive tx state updating function (ishaanam)
Pull request description:
This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated.
Second attempt at #17543
ACKs for top commit:
achow101:
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8
rajarshimaitra:
tACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8.
glozow:
ACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8
furszy:
Tested ACK 89df7987
Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546
1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff fuzz: Change LIMIT_TO_MESSAGE_TYPE from a compile-time to a run-time setting (MarcoFalke)
Pull request description:
The `process_message_${msg_type}` fuzz targets have many issues:
* In a context where each fuzz target must be a separate binary, this bloats the storage requirements by the number of message types.
* The qa-assets repo for fuzz inputs also bloats, because each input in the type specific folder (`./process_message_${msg_type}`) is accompanied by a similar input in the general folder (`./process_message`) or a in another specific folder. The size seems to be ~3GB for the sum of all folders vs 0.3GB for the general folder.
* Handling of different folders for each message type and one general folder for all message types (and unknown message types) is undocumented and unclear. Cross-pollination is encouraged, I guess, but who does it?
* It is unclear if the fuzz target has any value at all, given that any bug that is found here should also be found by the `process_messages` fuzz target, and historically always has been? So maybe it can even be removed completely in the future?
* (minor nit): When adding a new message type, the message type has to be added to this fuzz target as well.
Fix all issues by turning the compile-time setting into a run-time setting, thus removing the extra executables and fuzz folders. The same approach is also taken by the `rpc` fuzz target.
If someone wants to limit their fuzzing to a specific message type, they can still do it. For example,
```
LIMIT_TO_MESSAGE_TYPE=inv FUZZ=process_message ./src/test/fuzz/fuzz
ACKs for top commit:
dergoegge:
ACK 1111c9ac97ca0f0afeb5df45bc0970b761c3c9ff
Tree-SHA512: 9495538d9bc83b24671a44e9311a4e82ce5b2fa89e431e42772dcfa19f675a9fe2dd8cd3d3b15b154c8b7f400e57ee08a976d37e55a5425778ced0ee85fb984c
82bb7831fa6052620998c7eef47e48ed594248a8 wallet: skip block scan if block was created before wallet birthday (furszy)
a082434d122754ec1a10e0e08a35bdb1f47989e6 refactor: single method to append new spkm to the wallet (furszy)
Pull request description:
During initial block download, the node's wallet(s) scans every arriving block looking for data that it owns.
This process can be resource-intensive, as it involves sequentially scanning all transactions within each
arriving block.
To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time,
since these blocks are guaranteed not to contain any relevant wallet data.
This has direct implications (an speed improvement) on the underlying blockchain synchronization process
as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no
more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain
(activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are
processed by the wallet(s).
So, by skipping not relevant blocks in the wallet's IBD scanning process, we will also improve the chain
synchronization time.
ACKs for top commit:
ishaanam:
re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8
achow101:
re-ACK 82bb7831fa6052620998c7eef47e48ed594248a8
pinheadmz:
ACK 82bb7831fa6052620998c7eef47e48ed594248a8
Tree-SHA512: 70158c9657f1fcc396badad2c4410b7b7f439466142640b31a9b1a8cea4555e45ea254e48043c9b27f783d5e4d24d91855f0d79d42f0484b8aa83cdbf3d6c50b
`Assume` is safer since the checks are non-fatal- errors in these functions
should provide feedback in debug builds, but do not need to deter further node
operations in production.
fb02a3cd1a105bdf60ca39e1858e77685be88976 p2p: Log addresses of stalling peers (Martin Zumsande)
Pull request description:
This was suggested in #27705 by ArmchairCryptologist.
It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them.
This is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their `nodeId` to an address unless the very noisy `net` debugging is enabled.
In case of outbound peers for which the address is potentially logged when establishing the connection, this just adds some convenience.
ACKs for top commit:
stratospher:
tACK fb02a3c.
jamesob:
github ACK fb02a3cd1a
0xB10C:
Untested ACK fb02a3cd1a105bdf60ca39e1858e77685be88976
instagibbs:
utACK fb02a3cd1a105bdf60ca39e1858e77685be88976
Tree-SHA512: 2080f794c715bd36143405828b4b0e1be859095caf8f8a0c20dd2a4b64d192d78fee0fa350a2bb7c39848718332c4dd4d8edb2cc8d22095b65afe710591f7ccb
ffffe622e9cbf926326135bb23958380dcf09df1 test: Move test_chain_listunspent wallet check from mempool_packages to wallet_basic (MarcoFalke)
Pull request description:
This fixes a bug.
On master:
```
$ ./test/functional/mempool_packages.py --legacy-wallet
File "./test/functional/mempool_packages.py", line 52, in run_test
self.nodes[0].importaddress(self.wallet.get_address())
test_framework.authproxy.JSONRPCException: Bech32m addresses cannot be imported into legacy wallets (-5)
```
On this pull, all tests pass.
ACKs for top commit:
glozow:
ACK ffffe622e9cbf926326135bb23958380dcf09df1, thanks for changing! Nice to remove wallet from another non-wallet test.
Tree-SHA512: 842c3b7c2e90285a155b8ed9924ef0c99f7773892be4f1847e5d7ece79c914ea5acee0d71de2ce46c354ee95fb74a03c20c0afb5e49c0b8e1c0ce406df963650
eefe56967b4eb4b5144325cde4f40fc1cbde3e65 bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1b732a04976fc70cbb0661f97bbbd99 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b02197ad92fded20f6ff83b364747297 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
The WalletBalance benchmarks would incorrectly call LoadWallet after the
wallet has been setup. LoadWallet expects to be the first thing that is
called and for the CWallet to be in a fresh state. When it is not, it
results in bogus pointers which can cause segfaults during this
benchmark.
The wallet tests and benchmarks both had helper functions for loading
and unloading the wallet for the test that were almost identical.
These functions are consolidated and reused.
4fe5f3c4675263ea106e7ac6d336ec769392ebc3 depends: remove redundant stdlib option (Cory Fields)
Pull request description:
Like #27628, this is another dependency of #21778, though it doesn't become obvious until used with a newer clang.
This should be a no-op.
Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.
ACKs for top commit:
fanquake:
ACK 4fe5f3c4675263ea106e7ac6d336ec769392ebc3
Tree-SHA512: 904072f2b13dffbbeab2bc9ff20a74969888502fa1ea35d9030784dd397c6751e31233e6ec7dc34e9fd42574950be733b25d6653c2a93e214cc5e4eef2e0133a
To avoid wasting processing power, we can skip blocks that occurred
before the wallet's creation time, since these blocks are guaranteed
not to contain any relevant wallet data.
This has direct implications (an speed improvement) on the underlying
blockchain synchronization process as well.
The reason is that the validation interface queue is limited to
10 tasks per time. This means that no more than 10 blocks can be
waiting for the wallet(s) to be processed while we are synchronizing
the chain (activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster
from the network than what they are processed by the wallet(s).
3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1 use 'byte'/'bytes' for bech32(m) validation error (Reese Russell)
Pull request description:
This PR rectifies a linguistic inconsistency found in merged PR [27727](https://github.com/bitcoin/bitcoin/pull/27727). It addresses the improper usage of the term 'byte' in error reports. As it stands, PR [27727](https://github.com/bitcoin/bitcoin/pull/27727) exclusively utilizes 'byte' in error messages, regardless of the context, as demonstrated below:
Currently: ```Invalid Bech32 v0 address program size (16 byte), per BIP141```
This modification enhances the accuracy of error reporting in most scenarios users are likely to encounter by checking for a plural or singular number of bytes.
This PR
**16 Bytes program size error** :
```
(
"BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P",
"Invalid Bech32 v0 address program size (16 bytes), per BIP141",
[],
)
```
**1 Byte program size error**
```
(
"bc1pw5dgrnzv",
"Invalid Bech32 address program size (1 byte)",
[]
),
```
Thank you
ACKs for top commit:
MarcoFalke:
lgtm ACK 3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1
Tree-SHA512: 55069194a6a33a37559cf14b59b6ac05b1160f57f14d1415aef8e76c916c7c7f343310916ae85b3fa895937802449c1dddb2f653340d0f39203f06aee10f6fce
d97269579769effbe6eec2303ea0cc3e396d3e0d Unconditionally return when compact block status == READ_STATUS_FAILED (Greg Sanders)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204491894 which would have resulted in wasted bandwidth every once in a while.
ACKs for top commit:
Sjors:
utACK d97269579769effbe6eec2303ea0cc3e396d3e0d
mzumsande:
ACK d97269579769effbe6eec2303ea0cc3e396d3e0d
Tree-SHA512: 2483cae03093fc61b36279bbc455829822258703ae608ff2797a20449410b12bfe8d40de63c02fc79ed4ef4d91b34c63281b71b81a1c691cab9647b65761586f
eeee55f9288740747b6e8d806ce8177fd92635cf rpc: Fix invalid bech32 handling (MarcoFalke)
Pull request description:
Currently the handling of invalid bech32(m) addresses over RPC has many issues:
* No error for invalid addresses is reported, leading to internal bugs via `CHECK_NONFATAL`, see https://github.com/bitcoin/bitcoin/issues/27723
* The error messages use "data size" (the meaning of which is unclear to the user, because the witness program data and bech32 section data are related but different) when they mean "program size"
Fix all issues. Also, use the BIP 173 and BIP 350 test vectors.
ACKs for top commit:
achow101:
ACK eeee55f9288740747b6e8d806ce8177fd92635cf
brunoerg:
crACK eeee55f9288740747b6e8d806ce8177fd92635cf
Tree-SHA512: c8639ee49e2a54b740b72d66bc4a40352dd553a6e3220dea9f94e48e33124f21f597a2817cb405d0a4c88d21df1013c0a4877a01370a2d326aa2cff1f9c381a8
