The settings warning message is meant to be used only to discourage
users from modifying the file manually. Therefore, there is no need
to keep it in memory.
These exceptions are not related to situations specific to tests,
but are required in general:
Without the first check CheckBlockindex could fail for blocks where we
only know the header.
Without the second, it could fail when blocks are received out of order.
18ad1b9142 refactor: pass CRecipient to FundTransaction (josibake)
5ad19668db refactor: simplify `CreateRecipients` (josibake)
47353a608d refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c refactor: move parsing to new function (josibake)
6f569ac903 refactor: move normalization to new function (josibake)
435fe5cd96 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 18ad1b9142
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
6acec6b9ff multiprocess: Add type conversion code for UniValue types (Ryan Ofsky)
0cc74fce72 multiprocess: Add type conversion code for serializable types (Ryan Ofsky)
4aaee23921 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 6acec6b9ff
dergoegge:
reACK 6acec6b9ff
Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
e9014042a6 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a 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 e9014042a6
hebasto:
re-ACK e9014042a6.
ryanofsky:
Code review ACK e9014042a6. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
32a9f13cb8 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 32a9f13cb8
ryanofsky:
Code review ACK 32a9f13cb8. This seems like a potentially real race condition, and the fix here is pretty simple.
furszy:
ACK 32a9f13c
Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1
d55fdb1a49 Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee6 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 d55fdb1a49
achow101:
ACK d55fdb1a49
murchandamus:
ACK d55fdb1a49
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
97181decf5 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 97181decf5
dergoegge:
ACK 97181decf5
Tree-SHA512: 92bbcd3cd10a569757b4de91e1b2bcfebc2b75ddb0160be36d8e512a6fa4623cced1aba93bd1cc044962cd2b10e1d184ef109ccdfe3cfcf85cf4b9585d80d115
This test-only RPC is required when a TestNode initiates
an outbound v2 p2p connection. Add a new arg `v2transport`
so that the node can attempt v2 connections.
Introduces functionality to detect when limited peers connections
are desirable or not. Ensuring that the new connections desirable
services flags stay relevant throughout the software's lifecycle.
(Unlike the previous approach, where once the validation IBD flag
was set, the desirable services flags remained constant forever).
This will let us recover from stalling scenarios where the node had
successfully synced, but subsequently dropped connections and remained
inactive for a duration longer than the limited peers threshold (the
timeframe within which limited peers can provide blocks). Then, upon
reconnection to the network, the node may end up only establishing
connections with limited peers, leading to an inability to synchronize
the chain.
This also fixes a possible limited peers threshold violation during IBD,
when the user configures `-maxtipage` further than the BIP159's limits.
This rule violation could lead to sync delays and, in the worst-case
scenario, trigger the same post-IBD stalling scenario (mentioned above)
but during IBD.
In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.
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>
282b12ddb0 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 282b12ddb0
TheCharlatan:
ACK 282b12ddb0
glozow:
ACK 282b12ddb0. 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.
5555d8db33 test: Use blocks_path where possible (MarcoFalke)
fa9108941f 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 5555d8db33
pablomartin4btc:
ACK 5555d8db33
BrandonOdiwuor:
Code Review ACK 5555d8db33
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;
| ^
```
6044628543 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 6044628543
Tree-SHA512: db55b366f20fca2ef62e5f10a838f8a709d531678c35c1dba20898754029c788a2fd47995208ed6d187cf814109a7ca397bc2c301504500aee79da04c95d6895
fa96d93711 refactor: Allow std::span construction from CKey (MarcoFalke)
999962d68d 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 fa96d93711
willcl-ark:
ACK fa96d93711
Tree-SHA512: 44fccdce5f32bc16b44f3b1bd32e86d9eabfd09bca6abe79f2d6db0cb0b5e4aaeaff710f023cb21ccde9315d2007d55f1b43f29416e81bceeeabe3948f673d3a
3ba815b42d 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 3ba815b42d
kristapsk:
ACK 3ba815b42d
theStack:
Code-review ACK 3ba815b42d
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
cdc6ac4126 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 cdc6ac4126
achow101:
ACK cdc6ac4126
pablomartin4btc:
tACK cdc6ac4126
Tree-SHA512: f7488a85cc29056e2ac443ce8f34aea4dfde6ba246efce82235d6a4dca2dca4344f07b93c93424b4addcb83e4cb2ae49a3ebb37d89840d42d2aeea35904cab04
74ebd4d135 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 74ebd4d135
furszy:
ACK 74ebd4d135
brunoerg:
utACK 74ebd4d135
Tree-SHA512: 604adc3304b8e3cb1a10dfd017025c10b029bebd3ef533f96bcb5856fee5d4396a9aed4949908b8e7ef267ad21320d1814dd80f88426330c5c9c2c529c497591
It's preferable to use type-safe transaction identifiers to avoid
confusing txid and wtxid. The next commit will add a reference to this
set; we use this opportunity to change it to Txid ahead of time instead
of adding new uses of uint256.
ec779a2b8e doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack)
Pull request description:
Commit ab34dc6012 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 ec779a2b8e
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.
2d1b1c7dae 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 2d1b1c7dae
hebasto:
ACK 2d1b1c7dae.
Tree-SHA512: 91812de7da35346f51850714a188fcffbac478bc8b348bf756c2555fcbde86ba622ac2fb77d294dea0378c741d3656f06121ef3a795aeed63fd170fc31bfa5af
eb78ea4eeb [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 eb78ea4eeb
dergoegge:
Code review ACK eb78ea4eeb
theStack:
re-ACK eb78ea4eeb
mzumsande:
tested ACK eb78ea4eeb
Tree-SHA512: ae4420986dc7bd5cb675a7ebc76b24c8ee60007f0296ed37e272f1c3415764d44963bea84c51948da319a65661dca8a95eac2a59bf7e745519b6fcafa09812cf
No behavior change. Just an intermediate refactoring.
By relocating the peer desirable services flags into the peer
manager, we allow the connections acceptance process to handle
post-IBD potential stalling scenarios.
In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.
