fa3373d3adbace7e4665cf391363319a55a09a96 refactor: Compile unreachable code (MarcoFalke)
Pull request description:
When unreachable code isn't compiled, compile failures are not detected.
Fix this by leaving it unreachable, but compiling it.
Fixes https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1465010916
ACKs for top commit:
achow101:
ACK fa3373d3adbace7e4665cf391363319a55a09a96
ryanofsky:
Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.
Tree-SHA512: 3a3764915dfc935bf5d7a48f1ca151dcbac340c1cbdce8236b24ae9b4f04d6ee9771ed058ca60bcbca6e19d13671de3517f828a8f7ab6444c7cc4e3538d1ba4e
d298ff8b62b2624ed390c8a2f905c888ffc956ff During IBD, prune as much as possible until we get close to where we will eventually keep blocks (Luke Dashjr)
Pull request description:
This should reduce pruning flushes even more, speeding up IBD with pruning on systems that have a sufficient dbcache.
Assumes 1 MB per block between tip and best header chain. Simply adds this to the buffer pruning is trying to leave available, which results in pruning almost everything up until we get close to where we need to be keeping blocks.
ACKs for top commit:
andrewtoth:
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
fjahr:
utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
achow101:
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
Tree-SHA512: 2a482376bfb177e2ba7c2f0bb0b58b02efdb38b34755a18d1fc3e869df5959c85b6f1009e1386fa8b89c4f90d520383e36bd3e21dec221042315134efb1a455b
00c1e2aa4496b5f038ae5199dbd16d8313766533 build: fix optimisation flags used for --coverage (fanquake)
1dc2c9b385f8345c588449848149b8e470653afc ci: cleanup C*FLAG usage in Valgrind jobs (fanquake)
6cc2a38c1388b696e9c28a08c6bd9c93da4fa6b8 build: add sanitizer flags to configure output (fanquake)
08cd5aca18f0774258c7c459773b9e8b386d48ef build: always set -g -O2 in CORE_CXXFLAGS (fanquake)
Pull request description:
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults).
Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`.
Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here:
```bash
CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4
```
Fixes configure output to reflect actual compilation flag ordering, so it's useful.
Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize.
ACKs for top commit:
TheCharlatan:
lgtm ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533
hebasto:
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.
theuni:
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533
Tree-SHA512: cf6c7acf813ba10b198561e83eb72e9b2532a39cb1767c452d031e82921dcd42a47b129735b24c4e36131fd0c8fe7457f7cae870c1e011cdfdd430bdc4d4912b
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake)
5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake)
47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake)
6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake)
435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake)
Pull request description:
## Motivation
The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.
As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.
I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.
ACKs for top commit:
S3RK:
reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
6acec6b9ff02b91de132bb1575d75908a8a2d27b multiprocess: Add type conversion code for UniValue types (Ryan Ofsky)
0cc74fce72e0c79849109ee5d7afe707991b3512 multiprocess: Add type conversion code for serializable types (Ryan Ofsky)
4aaee239211a5287fbc361c0eb158b105ae8c8db test: add ipc test to test multiprocess type conversion code (Ryan Ofsky)
Pull request description:
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.
The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b
dergoegge:
reACK 6acec6b9ff02b91de132bb1575d75908a8a2d27b
Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
e9014042a6bed8c16cc9a31fc35cb709d4b3c766 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 init: improve corrupted/empty settings file error msg (furszy)
Pull request description:
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Improving a confusing situation reported by users who did not understand why a
settings parsing error occurred when the file was empty and did not know how to solve it.
Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content.
In both scenarios, the 'Unable to parse settings file' error does not help the user move forward.
ACKs for top commit:
achow101:
ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
hebasto:
re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766.
ryanofsky:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov)
Pull request description:
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
---
_Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit 856c88776f was merged in https://github.com/bitcoin/bitcoin/pull/29040 so now this only affects wallet code. The previous PR description was:_
Avoid this unsafe pattern from `ArgsManager` and `CWallet`:
```cpp
class A
{
Mutex mutex;
Foo member GUARDED_BY(mutex);
const Foo& Get()
{
LOCK(mutex);
return member;
} // callers of `Get()` will have access to `member` without owning the mutex.
```
ACKs for top commit:
achow101:
ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b
ryanofsky:
Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple.
furszy:
ACK 32a9f13c
Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
d55fdb1a495190e213b1b5127f5d91e4a409765e Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee63eebc760e2a9226afcd0c46d3ec2bd wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)
Pull request description:
This is a bugfix for from when [optional was introduced](758501b713) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.
I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.
You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`.
ACKs for top commit:
0xB10C:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
achow101:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
murchandamus:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
97181decf5726aab6c5cd01b3e1964072f2531ff Add test for negative transaction version w/ CSV to tx_valid.json (Chris Stewart)
Pull request description:
This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by dergoegge
For more information see:
https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455
ACKs for top commit:
darosior:
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
dergoegge:
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
Hopefully, refraining users from modifying the file unless they are
certain about the potential consequences.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The preceding "Unable to parse settings file" message lacked
the necessary detail and guidance for users on what steps to
take next in order to resolve the startup error.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec refactor: remove CTxMemPool::queryHashes() (stickies-v)
Pull request description:
`CTxMemPool::queryHashes()` is only used in `MempoolToJSON()`, where it can just as easily be replaced with the more general `CTxMemPool::entryAll()`. No behaviour change, just cleans up the code.
ACKs for top commit:
dergoegge:
Code review ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
TheCharlatan:
ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
glozow:
ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec. Looks like there's no conflicts.
Tree-SHA512: 16160dec8e1f2457fa0f62dc96d2d2efd92c4bab810ecdb0e08918b8e85a667702c8e41421eeb4ea6abe92a5956a2a39a7a6368514973b78be0d22de2ad299b2
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction`
take a `CRecipient` vector directly. This allows us to remove SFFO logic from
the wrapper RPC `FundTransaction` since the `CRecipient` objects have already
been created with the correct SFFO values. This also allows us to remove
SFFO from both `FundTransaction` function signatures.
This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.
Move validation logic out of `CreateRecipients` and instead take the
already validated outputs from `ParseOutputs` as an input.
Move SFFO parsing out of `CreateRecipients` into a new function,
`InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions
from `sendmany` and `sendtoaddress` and turns them into a set of integers.
In a later commit, we will also move the SFFO parsing logic from
`FundTransaction` into this function.
Worth noting: a user can pass duplicate addresses and addresses that dont exist
in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress`
without triggering a warning. This behavior is preserved in to keep this commit
strictly a refactor.
Move the parsing and validation out of `AddOutputs` into its own function,
`ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a
later commit, where the code is currently duplicated.
The new `ParseOutputs` function returns a CTxDestination,CAmount tuples.
This allows the caller to then translate the validated outputs into
either CRecipients or CTxOuts.
Move the univalue formatting logic out of AddOutputs and into its own function,
`NormalizeOutputs`. This allows us to re-use this logic in later commits.
5555d8db3313f893609eb0cf549bb597361d4466 test: Use blocks_path where possible (MarcoFalke)
fa9108941fa1a0e83484114e2d8a99d264c2ad09 rpc: Fix race in loadtxoutset (MarcoFalke)
Pull request description:
The tip may have advanced, also if it did not, there is no reason to
have two variables point to the same block.
Fixes https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1344694600
ACKs for top commit:
achow101:
ACK 5555d8db3313f893609eb0cf549bb597361d4466
pablomartin4btc:
ACK 5555d8db3313f893609eb0cf549bb597361d4466
BrandonOdiwuor:
Code Review ACK 5555d8db3313f893609eb0cf549bb597361d4466
Tree-SHA512: 23a82924a915b61bb1adab8ad20ec8914139c8ee647817af34ca27ee310a2e45833d8b285503e0feebe63e4667193d6d98cfcbbc1509bf40712225e04dd19e8b
`CWallet::GetEncryptionKey()` would return a reference to the internal
`CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.
Returning a copy would be a shorter solution, but could have security
implications of the master key remaining somewhere in the memory even
after `CWallet::Lock()` (the current calls to
`CWallet::GetEncryptionKey()` are safe, but that is not future proof).
So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
change the `GetEncryptionKey()` method to provide the encryption
key to a given callback:
`m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`
This silences the following (clang 18):
```
wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
3520 | return vMasterKey;
| ^
```
60446285436da62adef1c0a9b11c3336d82b4d89 crypto, hash: replace custom rotl32 with std::rotl (Fabian Jahr)
Pull request description:
While exploring some C++20 changes and checking against our code I found this potential improvement:
1. We can replace our custom implementation of `rotl32` in crypto/chacha20 with `std::rotl` from the [new `bit` header](https://en.cppreference.com/w/cpp/header/bit).
ACKs for top commit:
fanquake:
ACK 60446285436da62adef1c0a9b11c3336d82b4d89
Tree-SHA512: db55b366f20fca2ef62e5f10a838f8a709d531678c35c1dba20898754029c788a2fd47995208ed6d187cf814109a7ca397bc2c301504500aee79da04c95d6895
fa96d937116682f32613d31a3ae7d6f652e8146d refactor: Allow std::span construction from CKey (MarcoFalke)
999962d68d47e1e630d689aca880f41635c004cb Add missing XOnlyPubKey::data() to get mutable data (MarcoFalke)
Pull request description:
Is is possible to construct a `Span` from a reference to a `CKey`. However, the same is not possible with `std::span`.
Fix that.
ACKs for top commit:
shaavan:
ReACK fa96d937116682f32613d31a3ae7d6f652e8146d
willcl-ark:
ACK fa96d937116682f32613d31a3ae7d6f652e8146d
Tree-SHA512: 44fccdce5f32bc16b44f3b1bd32e86d9eabfd09bca6abe79f2d6db0cb0b5e4aaeaff710f023cb21ccde9315d2007d55f1b43f29416e81bceeeabe3948f673d3a
3ba815b42db74804e341ce15f648c2b297af55ca Make v2transport default for addnode RPC when enabled (Pieter Wuille)
Pull request description:
Since #29058, several types of manually configured connections will attempt v2 connections when `-v2transport` is enabled, except for the `addnode` RPC, as that one has an explicit argument to enable or disable.
Make the default for that RPC match the `-v2transport` setting so the behavior matches that of other manual connections from a user perspective.
ACKs for top commit:
achow101:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
kristapsk:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
theStack:
Code-review ACK 3ba815b42db74804e341ce15f648c2b297af55ca
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
cdc6ac4126b31426261605a757c52ea2dbfb2a81 snapshots: don't core dump when running -checkblockindex after `loadtxoutset` (Mark Friedenbach)
Pull request description:
Transaction counts aren't known for block history loaded from a snapshot. If you start with `-checkblockindex` after loading a snapshot, the bitcoin daemon will core dump. The test suite does not check for this because all the snapshots have no non-coinbase transactions (all blocks prior to the snapshot are assumed to have `nTx = 1`).
Recommend for backport to 26.x
ACKs for top commit:
fjahr:
utACK cdc6ac4126b31426261605a757c52ea2dbfb2a81
achow101:
ACK cdc6ac4126b31426261605a757c52ea2dbfb2a81
pablomartin4btc:
tACK cdc6ac4126b31426261605a757c52ea2dbfb2a81
Tree-SHA512: f7488a85cc29056e2ac443ce8f34aea4dfde6ba246efce82235d6a4dca2dca4344f07b93c93424b4addcb83e4cb2ae49a3ebb37d89840d42d2aeea35904cab04
74ebd4d1359edce82a134dfcd3da9840f8d206e2 doc, test: Test and explain service flag handling (Martin Zumsande)
Pull request description:
Service flags received from the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.
ACKs for top commit:
achow101:
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
furszy:
ACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
brunoerg:
utACK 74ebd4d1359edce82a134dfcd3da9840f8d206e2
Tree-SHA512: 604adc3304b8e3cb1a10dfd017025c10b029bebd3ef533f96bcb5856fee5d4396a9aed4949908b8e7ef267ad21320d1814dd80f88426330c5c9c2c529c497591
ec779a2b8e4fcc00596ee8833be35ae9b326552c doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack)
Pull request description:
Commit ab34dc6012351e7b8aab871dd9d2b38ade1cd9b of #28318 was an incomplete version of [`118c756` (#25203)](118c7567f6) from the `Severity-based logging` parent PR.
Add the missing text to update the `-loglevel` help doc.
While here, make the help text a little easier to understand.
Can be tested by running:
```
./src/bitcoind -regtest -help-debug | grep -A12 loglevel=
```
before
```
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC:
info, debug, trace (default=debug); warning and error levels are
always logged.
```
after
```
-loglevel=<level>|<category>:<level>
Set the global or per-category severity level for logging categories
enabled with the -debug configuration option or the logging RPC.
Possible values are info, debug, trace (default=debug). The
following levels are always logged: error, warning, info.
```
ACKs for top commit:
stickies-v:
ACK ec779a2b8e4fcc00596ee8833be35ae9b326552c
Tree-SHA512: 0c375e30a5a4c168ca7d97720e8c287f598216767afedae329824e09a480830faf8537b792c5c4bb647c68681c287fe3005c62093708ce85624e9a71c8245e42
This avoids cases of missing -O2, when *FLAGS has been overriden.
Removes the need for duplicate code to clear autoconf defaults.
Also, move CORE_CXXFLAGS before DEBUG_CXXFLAGS, so that -O2 is always
overriden if debugging etc.
2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e build: remove --enable-lto (fanquake)
Pull request description:
This has outlived its usefulness, doesn't gel well with newer compilers & `-flto` related options, i.e thin vs full, or `=auto`, and having `-flto` as the only option means that sometimes this just needs to be worked around, i.e in oss-fuzz:
https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh.
While it was convenient when `-flto` was newer, support for `-flto` is now in all compilers we use, and there's also no-longer any real need for us to treat `-flto` different to any other optimization option.
Remove it, to remove build complexity, and so there's no need to port a similar option to CMake.
Note that the LTO option remains in depends, because we still a way to build packages that have LTO specific patches/options.
ACKs for top commit:
TheCharlatan:
ACK 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e
hebasto:
ACK 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e.
Tree-SHA512: 91812de7da35346f51850714a188fcffbac478bc8b348bf756c2555fcbde86ba622ac2fb77d294dea0378c741d3656f06121ef3a795aeed63fd170fc31bfa5af
eb78ea4eebfe150bc1746282bfdad6eb0f764e3c [log] mempool loading (glozow)
Pull request description:
Motivated by #29193. Currently, we only log something (non-debug) when we fail to load the file and at the end of importing all the transactions. That means it's hard to tell what's happening if it's taking a long time to load.
This PR adds a maximum of 10 new unconditional log lines:
- When we start to load transactions.
- Our progress percentage when it advances by at least 10% from the last time we logged. Percentage is based on the number of transactions.
If there are lots of transactions in the mempool, the logs will look like this:
```
2024-01-11T11:36:30.410726Z Loading 401 mempool transactions from disk...
2024-01-11T11:36:30.423374Z Progress loading mempool transactions from disk: 10% (tried 41, 360 remaining)
2024-01-11T11:36:30.435539Z Progress loading mempool transactions from disk: 20% (tried 81, 320 remaining)
2024-01-11T11:36:30.447874Z Progress loading mempool transactions from disk: 30% (tried 121, 280 remaining)
2024-01-11T11:36:30.460474Z Progress loading mempool transactions from disk: 40% (tried 161, 240 remaining)
2024-01-11T11:36:30.473731Z Progress loading mempool transactions from disk: 50% (tried 201, 200 remaining)
2024-01-11T11:36:30.487806Z Progress loading mempool transactions from disk: 60% (tried 241, 160 remaining)
2024-01-11T11:36:30.501739Z Progress loading mempool transactions from disk: 70% (tried 281, 120 remaining)
2024-01-11T11:36:30.516334Z Progress loading mempool transactions from disk: 80% (tried 321, 80 remaining)
2024-01-11T11:36:30.531309Z Progress loading mempool transactions from disk: 90% (tried 361, 40 remaining)
2024-01-11T11:36:30.549019Z Imported mempool transactions from disk: 401 succeeded, 0 failed, 0 expired, 0 already there, 400 waiting for initial broadcast
```
If there are 0 or 1 transactions, progress logs aren't printed.
ACKs for top commit:
kevkevinpal:
Concept ACK [eb78ea4](eb78ea4eeb)
ismaelsadeeq:
ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
dergoegge:
Code review ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
theStack:
re-ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
mzumsande:
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
Tree-SHA512: ae4420986dc7bd5cb675a7ebc76b24c8ee60007f0296ed37e272f1c3415764d44963bea84c51948da319a65661dca8a95eac2a59bf7e745519b6fcafa09812cf
AttachChain will create the chain notifications handler which contains a
reference to the wallet's shared_ptr. If AttachChain fails, the wallet
needs to be unloaded, and this is expected to happen with its custom
deleter ReleaseWallet. However, if the chain notifications handler is
still set, then the shared_ptr is still referenced by something, so the
wallet is never actually released.
0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e test: Assert that a new tx with a delta of 0 is never added (kevkevin)
cfdbcd19b32fd63954d7947dcc639aef291fb6b2 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin)
252a86729a15e47ed168d8da7c4a8d6113673909 rpc: renaming txid -> transactionid (kevkevin)
2fca6c2dd03c3955d86efb0b8d2a7961e42115fd rpc: changed prioritisation-map -> "" (kevkevin)
3a118e19e100110300d3290d4c1434f963721d94 test: Directly constructing 2 entry map for getprioritisedtransactions (kevkevin)
Pull request description:
In this PR I am addressing some comments in https://github.com/bitcoin/bitcoin/pull/27501 as a followup.
- changed `prioritisation-map` in the `RPCResult` to `""`
- Directly constructing 2 entry map for getprioritisedtransactions in functional tests
- renamed `txid` to `transactionid` in `RPCResult` to be more consistent with naming elsewhere
- exposed the `modified_fee` field instead of having it be a useless arg
- Created a new test that asserts when `prioritisedtransaction` is called with a fee_delta of 0 it is not added to mempool
ACKs for top commit:
glozow:
reACK 0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e, only change is the doc suggestion
Tree-SHA512: e99056e37a8b1cfc511d87c83edba7c928b50d2cd6c2fd7c038976779850677ad37fddeb2b983e8bc007ca8567eb21ebb78d7eae9b773657c2b297299993ec05
aaaace2fd1299939c755c281b787df0bbf1747a0 fuzz: Assume presence of __builtin_*_overflow, without checks (MarcoFalke)
fa223ba5eb764fe822229a58d4d44d3ea83d0793 Revert "build: Fix undefined reference to __mulodi4" (MarcoFalke)
fa7c751bd923cd9fb4790fe7fb51fafa2faa1db6 build: Bump clang minimum supported version to 14 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-14 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
For reference:
* https://packages.debian.org/bookworm/clang (`clang-14`)
* https://packages.ubuntu.com/jammy/clang (`clang-14`)
* CentOS-like 8/9 Stream: All Clang versions from 15 to 17
* FreeBSD 12/13: All Clang versions from 15 to 16
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (`clang17`); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bullseye/g++ (g++-10)
* https://packages.ubuntu.com/focal/g++-10
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
ACKs for top commit:
fanquake:
ACK aaaace2fd1299939c755c281b787df0bbf1747a0
Tree-SHA512: 81d066b14cc568d27312f1cc814b09540b038a10a0a8e9d71fc9745b024fb6c32a959af673e6819b817ea7cef98da4abfa63dff16cffb7821b40083016b0291f
5fa74609b833643334dfb5519f2023119984267b Fix -netinfo backward compat with getpeerinfo pre-v26 (Jon Atack)
Pull request description:
Commit fb5bfed26a564014b83ccfc96ff00b630930fc61 in #29058 will cause `-netinfo` to break when calling it on a node that is running pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a "transport_protocol_type" field.
Fix this by adding an `IsNull()` check, as already done for other recent getpeerinfo fields, and also in the same commit:
a) avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v"
b) drop displaying the "v" prefix in all the rows, as it doesn't add useful information, and instead use "v" for the column header
c) display nothing when a value isn't determined yet, like for the -netinfo mping and ping columns (as `*` already has a separate meaning in this dashboard, and `?` might look like there is a bug)
ACKs for top commit:
mzumsande:
Code Review ACK 5fa74609b833643334dfb5519f2023119984267b
achow101:
ACK 5fa74609b833643334dfb5519f2023119984267b
kristapsk:
ACK 5fa74609b833643334dfb5519f2023119984267b
Tree-SHA512: 4afc513dc669b95037180008eb4c57fc0a0d742c02f24b236562d6b8daad5c120eb1ce0d90e51696e0f9b8361a72fc930c0b64f04902cf96fb48c8e042e58624
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
`CPubKey::VerifyPubKey` uses rng internally which leads to instability
in the fuzz test.
We fix this by avoiding `VerifyPubKey` in the test and verifying the
decoded public key with a fuzzer chosen message instead.
