5d718f6913 Mitigate timeout in CalculateTotalBumpFees (Murch)
Pull request description:
The slow fuzz seed described in #27799 was just slower than expected, not an endless loop. Ensuring that every anscestor is only processed once speeds up the termination of the graph traversal.
Fixes#27799
ACKs for top commit:
glozow:
ACK 5d718f6913
Tree-SHA512: f3c7cd2ef6716332136c75b43f6d54ce920be6f546a11bbf92b1fd65575607c42cc24b319691d86d0db038335636ba12b6387383a184f1589a8d71d1180f194f
5763b232e6 ci: return to using Ubuntu 22.04 in MSAN jobs (fanquake)
d3cbcbf626 ci: compile clang and compiler-rt in MSAN jobs (fanquake)
796bd1d0d1 ci: use LLVM 16.0.4 in MSAN jobs (fanquake)
883bc9f561 ci: remove extra CC & CXX from MSAN jobs (fanquake)
2d4f4b8f29 ci: standardize custom libc++ usage in MSAN jobs (fanquake)
Pull request description:
This reworks the MSAN CIs, to first compile Clang and compiler-rt (using GCC 12), and then, compile an MSAN instrumented libc++ using the just-built Clang 16. This fixes the `native_fuzz_with_msan` job, working around https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1005341, by not using the Debian provided Clang/LLVM.
Also included are changes to streamline how we use our "custom libc++", according to upstream: https://releases.llvm.org/16.0.0/projects/libcxx/docs/UsingLibcxx.html#using-a-custom-built-libc, as well as other minor cleanups in the CI configs.
An example job is currently running in the qa-assets repo: https://github.com/bitcoin-core/qa-assets/pull/129 (https://cirrus-ci.com/task/4632561431871488).
ACKs for top commit:
dergoegge:
utACK 5763b232e6
Tree-SHA512: 4f2a6e0b796bb1830b8346dd1e55eaa86a79037b8b4f16a336c1e29f4fc460acca2ecba076635459370bcbb4009333cb79d27ef1521c1fb5db7599cd5bdf558c
5cd0717a54 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky)
Pull request description:
DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this PR just drops the DataStream::Serialize method.
ACKs for top commit:
furszy:
lgtm ACK 5cd0717a
MarcoFalke:
lgtm ACK 5cd0717a54🌙
Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2
The slow fuzz seed described in #27799 was just slower than expected,
not an endless loop. Ensuring that every anscestor is only processed
once speeds up the termination of the graph traversal.
Fixes#27799
2cd28e9fef rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky)
Pull request description:
Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects.
This makes it possible to make calls like:
```sh
src/bitcoin-cli -named bumpfee txid fee_rate=10
```
instead of
```sh
src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'
```
RPC help is also updated to show options as top level named arguments instead of as nested objects.
<details><summary>diff</summary>
<p>
```diff
@@ -15,16 +15,17 @@
Arguments:
1. txid (string, required) The txid to be bumped
-2. options (json object, optional)
+2. options (json object, optional) Options object that can be used to pass named arguments, listed below.
+
+Named Arguments:
- {
- "conf_target": n, (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
+conf_target (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
- "fee_rate": amount, (numeric or string, optional, default=not set, fall back to wallet fee estimation)
+fee_rate (numeric or string, optional, default=not set, fall back to wallet fee estimation)
Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
Must be at least 1.000 sat/vB higher than the current transaction fee rate.
WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.
- "replaceable": bool, (boolean, optional, default=true) Whether the new transaction should still be
+replaceable (boolean, optional, default=true) Whether the new transaction should still be
marked bip-125 replaceable. If true, the sequence numbers in the transaction will
be left unchanged from the original. If false, any input sequence numbers in the
original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
@@ -32,11 +33,10 @@
still be replaceable in practice, for example if it has unconfirmed ancestors which
are replaceable).
- "estimate_mode": "str", (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
+estimate_mode (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
"unset"
"economical"
"conservative"
- }
Result:
{ (json object)
```
</p>
</details>
**Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.
ACKs for top commit:
achow101:
ACK 2cd28e9fef
Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
DataStream Serialize method has surprising behavior because it just serializes
raw bytes without a length prefix. When you serialize a string or vector, a
length prefix is serialized before the raw object contents so the object can be
unambiguously deserialized later. But DataStreams don't support deserializing
at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see
https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
PR just drops the DataStream::Serialize method.
6fce5ddc17 doc: update getnodeaddresses for CJDNS, I2P and Tor and rm link (Marnix)
Pull request description:
- remove broken link about how to properly configure tor
- generalize getnodeaddresses RPC in doc
ACKs for top commit:
fanquake:
ACK 6fce5ddc17
Tree-SHA512: 3a077a0724c57a5c6182d40fbf34a84d2515bf1bf06ea0ce717174d0a27f5b19b9521c1ed1995adfdf4d43c2ce978a81e2ec9e3c8faf83f5188571fa75ea5314
3126454dcf index: prevent race by calling 'CustomInit' prior setting 'synced' flag (furszy)
Pull request description:
Decoupled from #27607.
Fixed a potential race condition in master (not possible so far) that could become an actual issue soon.
Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its
`CustomInit` method, causing the index to try to update itself before it is initialized.
This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
before calling to the child class init function (`CustomInit`). So, for example, the block filter index could
process a block before initialize the next filter position field and end up overwriting the first stored filter.
This race was introduced in bef4e405f3 from https://github.com/bitcoin/bitcoin/pull/25494.
ACKs for top commit:
achow101:
ACK 3126454dcf
mzumsande:
Code review ACK 3126454dcf
TheCharlatan:
Nice, ACK 3126454dcf
Tree-SHA512: 7a53fed1d2035cb4c1f331d6ee9f92d499b6cbb618ea534c6440f5a45ff9b3ac4dcff5fb4b88937f45a0be249e3a9c6dc6c3ac77180f12ae25fc56856ba39735
fa3ab45203 ci: Enable float-divide-by-zero check (MarcoFalke)
Pull request description:
Enable it, because
* It is enabled on OSS-Fuzz, so to be able to catch bugs earlier, enable it here as well.
* It makes sense to enable, because when a float is divided by zero, it may be a logic bug in our code, so it should be suppressed in the suppressions file.
ACKs for top commit:
willcl-ark:
utACK fa3ab45203
dergoegge:
ACK fa3ab45203
Tree-SHA512: 2c2c025af4fe3ec267b3cfa38f25495e9da678cf6c529a6438ec923ef09a06ad37fa4503c30cbacc83578ac2856a7f729ef70a24befffd61d10ec075132d1ee0
facbcd3742 doc: Remove unused NO_BLOOM_VERSION constant (MarcoFalke)
Pull request description:
This source code is the wrong place to document historic and now irrelevant details. Also, while touching the docs, clarify that the BIP 35 `mempool` message type is currently also guarded by the BIP 111 `NODE_BLOOM` flag, even though BIP 111 does not mention the `mempool` message type.
ACKs for top commit:
0xB10C:
ACK facbcd3
dergoegge:
ACK facbcd3742
Tree-SHA512: e69cf5cc2a4e6b061e8996bd9afc0c3e3c4e8174c086ecde425e0e9350b918f1c6612ce273ea1722d30e856049b83dada8782e7e96f676ae0112af85b22f868f
fafb4da121 fuzz: Avoid timeout in utxo_total_supply (MarcoFalke)
Pull request description:
Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773.
It can be checked that the fuzz target can still find the CVE, see https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057 with a diff of:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..6f4cfb5f51 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -39,8 +39,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
// the underlying coins database.
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin) {
- if (!vInOutPoints.insert(txin.prevout).second)
- return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
}
if (tx.IsCoinBase())
```
Also, fix a nit, see https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1186451948
ACKs for top commit:
dergoegge:
ACK fafb4da121
Tree-SHA512: a28fe9cd6ebb4c9bed5a5b35be76c1c436a87586c8fc3b3c4c8559a4a77ac08098324370da421d794c99579882c0872b6b29415de47ade6a05a08504a3d494c4
a10f032115 fuzz: fix wallet notifications.cpp (furszy)
Pull request description:
Fixing https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816.
As the fuzzing test requires all blocks to be scanned by the wallet
(because it is asserting the wallet balance at the end), we need to
ensure that no blocks are skipped by the recently added wallet
birth time functionality.
This just means setting the chain accumulated time to the maximum
value, so the wallet birth time is always below it, and the block is
always processed by the wallet.
ACKs for top commit:
MarcoFalke:
lgtm ACK a10f032115, thanks
Tree-SHA512: c9b38c52917cc36674415470752625b8161fc6b878b0b87d6926b462ba9666be3c225d396604c7e944a4c268fc35fc624807777aa0ed94bddbe18d8f8436de3c
9fe9074266 test: add block sync to getblockfrompeer.py (Martin Zumsande)
Pull request description:
This adds an additional `sync_blocks` call, fixing an intermittent error caused by blocks arriving out of order due to how compact block relay may revert to headers processing when the tip hasn't caught up, and resulting in slightly different pruning behavior.
Making sure that all blocks from the previous tests are synced before generating more blocks makes this impossible.
See https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566354933 and https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566554075 for a more detailed analysis.
#27770 is a more long-term approach to avoid having to deal with magic pruneheight numbers in the first place, but that PR introduces a new RPC and needs more discussion.
Fixes#27749.
ACKs for top commit:
MarcoFalke:
lgtm ACK 9fe9074266
theStack:
ACK 9fe9074266
Tree-SHA512: f3de1ea68725429aeef448c351ea812b805fa216912b112d7db9aceeddb1f2381b705c2577734b0d308e78ec5e0c4d26dc65fc2171f6e21f13061fc71d48216c
fa123077bc ci: Use podman for persistent workers (MarcoFalke)
fa9c65a74c ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN (MarcoFalke)
Pull request description:
This should prevent the persistent workers from running out of disk space. Containers are already removed, but not images. This is required since CI images are built and cached.
ACKs for top commit:
hebasto:
ACK fa123077bc
Tree-SHA512: 07c4faec57d659d1762e4e6d776c882ee48d4bac6ce6d438d56d9ab13277be3e39d6aa38816165a5a3e0938ac5d47674ee2921b6e115a4bb54e3e4910b34c4b6
As the fuzzer test requires all blocks to be
scanned by the wallet (because it is asserting
the wallet balance at the end), we need to
ensure that no blocks are skipped by the recently
added wallet birth time functionality.
This just means setting the chain accumulated time
to the maximum value, so the wallet birth time is
always below it, and the block is always processed.
This fixes an intermittent error, caused by blocks arriving
out of order due to how compact block relay may revert to headers
processing when the tip hasn't caught up, and resulting in slightly
different pruning behavior.
Making sure that all blocks from the previous tests are synced before
generating more blocks makes this impossible.
See Issue #27749 for more details.
5c832c3820 p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost` (brunoerg)
34bcdfc6a6 p2p, refactor: return vector/optional<CService> in `Lookup` (brunoerg)
7799eb125b p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost` (brunoerg)
5c1774a563 p2p, refactor: return `std::vector<CNetAddr>` in `LookupIntern` (brunoerg)
Pull request description:
Continuation of #26078.
To improve readability instead of returning a bool and passing stuff by reference, this PR changes:
- `LookupHost` to return `std::vector<CNetAddr>`
- `LookupHost` to return `std::optional<CNetAddr>`
- `Lookup` to return `std::vector<CService>`
- `Lookup` to return `std::optional<CService>`.
- `LookupIntern` to return `std::vector<CNetAddr>`
As discussed in #26078, it would be better to avoid using `optional` in some cases, but for specific `Lookup` and `LookupHost` functions it's necessary to use `optional` to verify if they were able to catch some data from their overloaded function.
ACKs for top commit:
achow101:
ACK 5c832c3820
stickies-v:
re-ACK 5c832c3820 - just addressing two nits, no other changes
theStack:
re-ACK 5c832c3820
Tree-SHA512: ea346fdc54463999646269bd600cd4a1590ef958001d2f0fc2be608ca51e1b4365efccca76dd4972b023e12fcc6e67d226608b0df7beb901bdeadd19948df840
7379a54ec4 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow)
846b2fe67e tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow)
c61d3f02f5 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow)
Pull request description:
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.
The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.
The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I'm working on.
ACKs for top commit:
Sjors:
utACK 7379a54ec4
furszy:
ACK 7379a54
Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba
fa5680b752 fix includes for touched header files (iwyu) (MarcoFalke)
dddde27f6f Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke)
Pull request description:
Only add it for those where it is an error to ignore. Also, fix the gcc compile warning https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1564350880. Also, fix iwyu for touched header files.
ACKs for top commit:
TheCharlatan:
ACK fa5680b752
stickies-v:
ACK fa5680b752
Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415
7d3b35004b refactor: Move system from util to common library (TheCharlatan)
7eee356c0a refactor: Split util::AnyPtr into its own file (TheCharlatan)
44de325d95 refactor: Split util::insert into its own file (TheCharlatan)
9ec5da36b6 refactor: Move ScheduleBatchPriority to its own file (TheCharlatan)
f871c69191 kernel: Add warning method to notifications (TheCharlatan)
4452707ede kernel: Add progress method to notifications (TheCharlatan)
84d71457e7 kernel: Add headerTip method to notifications (TheCharlatan)
447761c822 kernel: Add notification interface (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: https://github.com/bitcoin/bitcoin/pull/27419https://github.com/bitcoin/bitcoin/pull/27254https://github.com/bitcoin/bitcoin/pull/27238.
`interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`.
The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated.
ACKs for top commit:
MarcoFalke:
re-ACK 7d3b35004b (no change) 🎋
stickies-v:
Code Review ACK 7d3b35004b
hebasto:
re-ACK 7d3b35004b, only last two commits dropped since my [recent](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435394620) review.
Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d
bc862fad29 ConnectTip: don't log total disk read time in bench (Sjors Provoost)
Pull request description:
The " Load block from disk" log introduced in #24216 incorrectly assumed `num_blocks_total` would be greater than 0. This is not guaranteed until the `ConnectBlock` call right below it.
The total and average metric is not very useful because it does not distinguish between blocks read from disk and those loaded from memory. So rather than fixing the divide by zero issue, we just drop the metric.
Fixes#27635
ACKs for top commit:
MarcoFalke:
lgtm ACK bc862fad29🐓
willcl-ark:
tACK bc862fad29
Tree-SHA512: ff52ff8a8a93f1c82071ca84c57ce5839e14271943393deac0aa5555d63383708777ed96e7226be6dd71b63ed07dc27bad1634ee848e88e4d0b95d511a8267e7
015cc5e588 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 015cc5e588
Tree-SHA512: ebb754f293c2a61a0ef64c3552f7c700ceb3054b50fd3f1573e4a9e87773ddeba47bd9875f6ab055043012dbc20aeb71e4d76cd3da535c76651dfb1fbfc66e89
59c8944749 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 59c8944749
Tree-SHA512: ed654f63dbebdd02e4414d1f81147d92a4d490dbb5a2e0376858e3129097645f3a2df45191d6b40c410a76e803b0d28796d1a01c1d2fd995b94e8b7eb3949027
1f97572b9c 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 1f97572b9c
Tree-SHA512: de885210076d23f3394c42ca50e6ae2470c0ae6523399a2fa3ebb7c06383bdacef9c26166fa19747200396bed796c8772165e24416eb30ed8edd024e3394b2fe
689a65d878 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 689a65d878 tested this service file with 25.0
Tree-SHA512: 9a52ad5cf25886c0d8dabc986d8920602a056db25875b5edd910b387043b78bb78c76d6df82e6e322e3be3bfd5c35c80721cbc8308cec946060bd7586820e9c6
fa6b11a556 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 fa6b11a556
Tree-SHA512: 018ebbbf819ba7cdf0c6dd294fdfaa5ddb81b87058a8b9c57b96066d5b07e1656fd78f18e3cef375aebefa191fa515c2c70bc764880fa05f98f526334431a616
fa12558d21 ci: Avoid leaking HOME var into CI pod (MarcoFalke)
aaaa432603 ci: Remove "default" test env (MarcoFalke)
fa7a87bc7c 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 fa12558d21
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'
89df7987c2 Add wallets_conflicts (Antoine Riard)
dced203162 wallet, tests: mark unconflicted txs as inactive (ishaanam)
096487c4dc 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 89df7987c2
rajarshimaitra:
tACK 89df7987c2.
glozow:
ACK 89df7987c2
furszy:
Tested ACK 89df7987
Tree-SHA512: 3133b151477e8818302fac29e96de30cd64c09a8fe3a7612074a34ba1a332e69148162e6cb3f1074d9d9c9bab5ef9996967d325d8c4c99ba42b5fe3b66a60546
1111c9ac97 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 1111c9ac97
Tree-SHA512: 9495538d9bc83b24671a44e9311a4e82ce5b2fa89e431e42772dcfa19f675a9fe2dd8cd3d3b15b154c8b7f400e57ee08a976d37e55a5425778ced0ee85fb984c
82bb7831fa wallet: skip block scan if block was created before wallet birthday (furszy)
a082434d12 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 82bb7831fa
achow101:
re-ACK 82bb7831fa
pinheadmz:
ACK 82bb7831fa
Tree-SHA512: 70158c9657f1fcc396badad2c4410b7b7f439466142640b31a9b1a8cea4555e45ea254e48043c9b27f783d5e4d24d91855f0d79d42f0484b8aa83cdbf3d6c50b
