f336ff7f21 rpc: avoid expensive `IsMine` calls in `GetReceived` tally (Sebastian Falbesoner)
a7b65af2a4 rpc: avoid scriptPubKey<->CTxDestination conversions in `GetReceived` tally (Sebastian Falbesoner)
Pull request description:
The RPC calls `getreceivedbyaddress`/`getreceivedbylabel` both use the internal helper function `GetReceived` which was introduced in PR #17579 to deduplicate tallying code. For every wallet-related transaction output, the following unnecessary operations are currently performed in the tally loop, leading to a quite bad performance (as reported in #23645):
- converting from CScript -> TxDestination (`ExtractDestination(...)`), converting from TxDestination -> CScript (`CWallet::IsMine(const CTxDestination& dest)`); this can be avoided by directly using output scripts in the search set instead of addresses (first commit)
- checking if the iterated output script belongs to the wallet by calling `IsMine`; this can be avoided by only adding addresses to the search set which fulfil `IsMine` in the first place (second commit)
### Benchmark results
The functional test [wallet_pr23662.py](https://github.com/theStack/bitcoin/blob/pr23662_benchmarks/test/functional/wallet_pr23662.py) (not part of this PR) creates transactions with 15000 different addresses:
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine set (received)
- 5000 outputs (500 txs with 10 outputs each) with label set, IsMine not set (sent)
- 5000 outputs (500 txs with 10 outputs each) without label set, IsMine not set (sent)
Then, the time is measured for calling `getreceivedbyaddress` and `getreceivedbylabel`, the latter with two variants. Results on my machine:
| branch | `getreceivedbyaddress` (single) | `getreceivedbylabel` (single) | `getreceivedbylabel` (10000) |
|--------------------|---------------------------------|-------------------------------|------------------------------|
| master | 406.13ms | 425.33ms | 446.58ms |
| PR (first commit) | 367.18ms | 365.81ms | 426.33ms |
| PR (second commit) | 3.96ms | 4.83ms | 339.69ms |
Fixes#23645.
ACKs for top commit:
achow101:
ACK f336ff7f21
w0xlt:
ACK f336ff7f21
furszy:
Code ACK f336ff7f
Tree-SHA512: 9cbf402b9e269713bd3feda9e31719d9dca8a0dfd526de12fd3d561711589195d0c50143432c65dae279c4eab90a4fc3f99e29fbc0452fefe05113e92d129b8f
f64aa9c411 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
5f213213cb tests: add tests for cross-chain wallet use prevention (Seibart Nedor)
968765973b wallet: ensure wallet files are not reused across chains (Seibart Nedor)
Pull request description:
This implements a proposal in #12805 and is a rebase of #14533.
This seems to be a working approach, but I'm not sure why the `p2p_segwit.py` functional test needed a change, so I'll look into it more.
ACKs for top commit:
achow101:
ACK 5f213213cb
dongcarl:
Code Review ACK 5f213213cb
[deleted]:
tACK 5f213213cb
Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec
2052e3aa9a wallet: ignore chainStateFlushed notifications while attaching chain (Martin Zumsande)
Pull request description:
Fixes#24487
When a rescan is performed during `CWallet::AttachChain()` (e.g. when loading an old wallet) but this is interrupted by a shutdown signal, the wallet will currently stop the rescan, receive a `chainStateFlushed` signal, set the saved best block to the tip and shut down. At next startup, the rescan is not continued or repeated because of this. But some blocks have never been scanned by the wallet, which could lead to an incorrect balance.
Fix this by ignoring `chainStateFlushed` notifications until the chain is attached. Since `CWallet::chainStateFlushed` is being manually called by `AttachChain()` anyway after finishing with the rescan, it is not a problem if intermediate notifications are ignored.
Manual rescans started / aborted by the `rescanblockchain` / `abortrescan` RPCs are not affected by this.
I didn't choose alternative ways of fixing this issue that would delay the validationinterface registration or change anything else about the handling of `blockConnected` signals for the reasons mentioned in [this existing comment](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L2937-L2944).
ACKs for top commit:
achow101:
ACK 2052e3aa9a
ryanofsky:
Code review ACK 2052e3aa9a. This is a straightforward fix for the bug described in #24487 where a wallet could skip scanning blocks if is shut down in the middle of a sync and a chainStateFlushed notification was received during the sync. It would be nice to write a test for this but probably would be tricky to write.
w0xlt:
Code Review ACK 2052e3aa9a
Tree-SHA512: a6186173d72b26bd4adbf2315e11af365004a723ea5565a0f7b868584dc47c321a6572eafaeb2420bd21eed1c7ad92b47e6218c5eb72313a3c6bee58364e2247
ab5af9ca72 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0 wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181 wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca72
josibake:
crACK ab5af9ca72
0xB10C:
ACK ab5af9ca72. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
bae4561938 scripted-diff: rename BytePtr to AsBytePtr (João Barbosa)
Pull request description:
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in [/usr/include/MacTypes.h](https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h.auto.html):
```cpp
typedef UInt8 * BytePtr;
```
ACKs for top commit:
MarcoFalke:
cr ACK bae4561938
laanwj:
Code review ACK bae4561938
sipa:
utACK bae4561938
prusnak:
Approach ACK bae4561938
Tree-SHA512: fb4d4a94c9c2238107952c071bae9bf6bbde6ed6651f6d300f222adf8a6a423f0567cbbcc3102d4167ef2e4e6f9988a2f91d75a5418bf6bcd64eebb4bcd1077f
4637bbe448 rpc: Explain active and internal in listdescriptors (Andrew Chow)
Pull request description:
The current help text for active and internal in listdescriptors is not particularly helpful. They require the reader to already know what those terms mean. This help text is updated to actually explain the definitions of those words in context of a descriptor wallet.
ACKs for top commit:
S3RK:
ACK 4637bbe448
jarolrod:
ACK 4637bbe448
w0xlt:
ACK 4637bbe448
Tree-SHA512: 0af2c04f3b9920799cf616ad618bde9248eb9f74cc28f443b5b0f6646deba76e9b1415aca0865ad3bcc24aa6af0e9d07ad7b7cd80f0fe80838cf847f1b944426
9c96f1008b tidy: enable modernize-use-nullptr (fanquake)
e53274868e Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) (practicalswift)
Pull request description:
Alternative to #15112 which uses `clang-tidy` to do perform the checking, rather than `-Wzero-as-null-pointer-constant`, and avoids having to uses pragmas, i.e:
```cpp
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
#if defined(HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
#pragma GCC diagnostic ignored "-Wunknown-pragmas"
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant"
#endif
```
to suppress warnings coming from upstream code.
Can be tested by dropping the preceding commit. Should produce errors like:
```bash
clang-tidy-14 --use-color -p=/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu /home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp
/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp:678:36: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (!Socks5(strDest, port, 0, sock)) {
^
nullptr
```
ACKs for top commit:
laanwj:
Concept and code review ACK 9c96f1008b
Tree-SHA512: d822a354e44ba8f7fc53da9a4be7de5c25cc4ffc7c57651b76fdd1a030764b0390cfd79fca94685b8a3ff4f4d13054764f12d1f0d8c2a1b9ba519a7524f7f5bf
fa870e3d4c Remove not needed clang-format off comments (MarcoFalke)
Pull request description:
It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments.
Can be reviewed with `--word-diff-regex=. --ignore-all-space`
Looks like this was initially added in commit d9d79576f4 to accommodate a linter that has since been removed and replaced by a functional test.
ACKs for top commit:
laanwj:
Code review ACK fa870e3d4c
fanquake:
ACK fa870e3d4c
Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
a62e84438d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d9 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e4 refactor: introduce single-separator split helper `SplitString` (Sebastian Falbesoner)
Pull request description:
This PR adds a simple string split helper `SplitString` that takes use of the spanparsing `Split` function that was first introduced in #13697 (commit fe8a7dcd78). This enables to replace most calls to `boost::split`, in the cases where only a single separator character is used. Note that while previous attempts to replace `boost::split` were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all `boost::split` instances can be tackled.
As a possible optimization, one could return a vector of `std::string_view`s (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.
ACKs for top commit:
martinus:
Code review ACK a62e84438d. Ran all tests. I also like that with `boost::split` it was not obvious that the resulting container was cleared, and with `SplitString` API that's obvious.
Tree-SHA512: 10cb22619ebe46831b1f8e83584a89381a036b54c88701484ac00743e2a62cfe52c9f3ecdbb2d0815e536c99034558277cc263600ec3f3588b291c07eef8ed24
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in /usr/include/MacTypes.h.
-BEGIN VERIFY SCRIPT-
sed -i 's/BytePtr/AsBytePtr/' $(git grep -l "BytePtr" src)
-END VERIFY SCRIPT-
The current help text for active and internal in listdescriptors is not
particularly helpful. They require the reader to already know what those
terms mean. This help text is updated to actually explain the
definitions of those words in context of a descriptor wallet.
ee02c8bd9a util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9a
MarcoFalke:
ACK ee02c8bd9a🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
3ae7791bca refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bca
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
6f29409ad1 test: Add a test that creates a wallet with invalid parameters (w0xlt)
0359d9b6a3 Change wallet validation order (w0xlt)
Pull request description:
In the current code, the database is created before the last validation, which checks that passphrase is set and private keys are disabled.
Therefore, if this validation fails, it will result in an empty database and the user will not be able to recreate a wallet with the same name and with the correct parameters.
Behavior on the master branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01"
error code: -4
error message:
Wallet file verification failed. Failed to create database path '/home/w/.bitcoin/regtest/wallets/invalid_wallet'. Database already exists.
```
Behavior on the PR branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02"
{
"name": "invalid_wallet_01",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6f29409ad1
Tree-SHA512: d192955fc2285bf27ae5dd4c1b7cfd3d85441a7f3554b189b974aefb319c6b997543991dbb0ca2c8cb980f7058913a77cf0164c02e9b51ceb9c2cb601317c428
In the current code, the database is created before the last validation,
which checks that passphrase is set and private keys are disabled.
Therefore, if this validation fails, it will result in an empty database
and the user will not be able to recreate a wallet with the same name
and with the correct parameters.
When we use only manually specified inputs, we should still calculate
the waste so that if anything later on calls GetWaste (in order to log
it), there won't be an error.
This helper uses spanparsing::Split internally and enables to replace
all calls to boost::split where only a single separator is passed.
Co-authored-by: Martin Ankerl <Martin.Ankerl@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
21520b9551 fuzz: add target for coinselection (Martin Zumsande)
Pull request description:
This adds a fuzz target for the coinselection algorithms by creating random `OutputGroup`s and running all three coin selection algorithms for them.
It does not fuzz higher-level wallet logic for selecting eligible coins (as in `SelectCoins()`), thought it probably would make sense to have a fuzz target for that too.
ACKs for top commit:
achow101:
ACK 21520b9551
vasild:
ACK 21520b9551
Tree-SHA512: c763003cf5ff5317f929d3d0b2f06fa739ae41dd642042d9a5c5c96e6cb9b349a6c7aeabc77bc2b846d12c8bcb60e07ee20a9f38539429c65723ab76aeee6b2e
0c12f0116c wallet: Postpone NotifyWalletLoaded() for encrypted wallets (Hennadii Stepanov)
aeee419c6a wallet, refactor: Add wallet::NotifyWalletLoaded() function (Hennadii Stepanov)
Pull request description:
Fixesbitcoin-core/gui#571.
`CWallet::Create()` notifies about wallet loading too early, that results the notification goes before `DescriptorScriptPubKeyMan`s were created and added to an encrypted wallet.
And `interfaces::Wallet::taprootEnabled()` in ecf692b466/src/qt/receivecoinsdialog.cpp (L100-L102) erroneously returns `false` for just created encrypted descriptor wallets.
ACKs for top commit:
Sjors:
utACK 0c12f0116c
achow101:
ACK 0c12f0116c
Tree-SHA512: 2694bacd12748cd5f6c95d9d3bf8bcf4502ee67fecd8d057f33236b72069c61401b08f49deb013fc71c3f1e51ae16bdfd827ddcbc2a083d7044589be7a78982e
9563a645c2 refactor: add stdd:: includes to core_write (fanquake)
8b9efebb0a refactor: use named args when ScriptToUniv or TxToUniv are invoked (Michael Dietz)
22f25a6116 refactor: prefer snake case, TxToUniv arg hashBlock renamed block_hash (Michael Dietz)
828a094ecf refactor: merge ScriptPubKeyToUniv & ScriptToUniv into one function (Michael Dietz)
Pull request description:
I've cherry-picked some of the commits out of #22924, and made minor changes (like fixing named args).
ACKs for top commit:
MarcoFalke:
re-ACK 9563a645c2🕓
Tree-SHA512: 4f0e5b45c14cbf68b9e389bbe1211c125d95cbd3da5205b1cff6a4c44f15b15039ba2a5b25cd7e2580d9169404f1b7ff620d8a7e01f6112e3cb153ecfaef8916
2ef47ba6c5 util/check: stop using lambda for Assert/Assume (Anthony Towns)
7c9fe25c16 wallet: move Assert() check into constructor (Anthony Towns)
Pull request description:
Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.
Fixes#21596Fixes#24654
ACKs for top commit:
MarcoFalke:
ACK 2ef47ba6c5🚢
jonatack:
Tested re-ACK 2ef47ba6c5
Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
bb84b7145b add tests for no recipient and using send_max while inputs are specified (ishaanam)
49090ec402 Add sendall RPC née sweep (Murch)
902793c777 Extract FinishTransaction from send() (Murch)
6d2208a3f6 Extract interpretation of fee estimation arguments (Murch)
a31d75e5fb Elaborate error messages for outdated options (Murch)
35ed094e4b Extract prevention of outdated option names (Murch)
Pull request description:
Add sendall RPC née sweep
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _given UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _given budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending a given set of
UTXOs such as paying the value from one or more specific UTXOs, emptying
a wallet, or burning dust, we realized that there are some cases in
which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual
computation of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific
subset of the wallet's UTXO pool (by default all of them) and assigns
the funds to one or more receivers. Receivers can either be specified
with a given amount or receive an equal share of the remaining
unassigned funds. At least one recipient must be provided without
assigned amount to collect the remainder. The `sendall` call will
never create change. The call has a `send_max` option that changes the
default behavior of spending all UTXOs ("no UTXO left behind"), to
maximizing the output amount of the transaction by skipping uneconomic
UTXOs. The `send_max` option is incompatible with providing a specific
set of inputs.
---
Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal.
ACKs for top commit:
achow101:
re-ACK bb84b7145b
Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
_Motivation_
Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the
recipients objects for all forms of sending calls. According to the
commit discussion, this flag was chiefly introduced to permit sweeping
without manually calculating the fees of transactions. However, the flag
leads to unintuitive behavior and makes it more complicated to test
many wallet RPCs exhaustively. We proposed to introduce a dedicated
`sendall` RPC with the intention to cover this functionality.
Since the proposal, it was discovered in further discussion that our
proposed `sendall` rpc and SFFO have subtly different scopes of
operation.
• sendall:
Use _specific UTXOs_ to pay a destination the remainder after fees.
• SFFO:
Use a _specific budget_ to pay an address the remainder after fees.
While `sendall` will simplify cases of spending from specific UTXOs,
emptying a wallet, or burning dust, we realized that there are some
cases in which SFFO is used to pay other parties from a limited budget,
which can often lead to the creation of change outputs. This cannot be
easily replicated using `sendall` as it would require manual computation
of the appropriate change amount.
As such, sendall cannot replace all uses of SFFO, but it still has a
different use case and will aid in simplifying some wallet calls and
numerous wallet tests.
_Sendall call details_
The proposed sendall call builds a transaction from a specific subset of
the wallet's UTXO pool (by default all of them) and assigns the funds to
one or more receivers. Receivers can either be specified with a specific
amount or receive an equal share of the remaining unassigned funds. At
least one recipient must be provided without assigned amount to collect
the remainder. The `sendall` call will never create change. The call has
a `send_max` option that changes the default behavior of spending all
UTXOs ("no UTXO left behind"), to maximizing the output amount of the
transaction by skipping uneconomic UTXOs. The `send_max` option is
incompatible with providing a specific set of inputs.
21db4eb3ff test: fix incorrect named args in wallet tests (fanquake)
8b0e776718 test: fix incorrect named args in coin_selection tests (fanquake)
6fc00f7331 bench: fix incorrect named args in coin_selection bench (fanquake)
Pull request description:
Should be one of the last changes split from #24661.
Motivation:
> Incorrect named args are source of bugs, like https://github.com/bitcoin/bitcoin/pull/22979.
> To allow them being checked by clang-tidy, use a format it can understand.
ACKs for top commit:
MarcoFalke:
Concept ACK 21db4eb3ff
Tree-SHA512: c29743a70f6118cf73dc37b56b30f45da55b7d7b3b8ed36859ad59f602c3e6692eb755e05d9a4dd17f05085bcd6cb5b8c4007090a76e4fbfb053f925322cf985
fc892c3a80 rpc: Fail to return undocumented or misdocumented JSON (MarcoFalke)
f4bc4a705a rpc: Add m_skip_type_check to RPCResult (MarcoFalke)
Pull request description:
This avoids documentation shortcomings such as the ones fixed in commit e7b6272b30, 138d55e6a0, 577bd51a4b, f8c84e047c, 0ee9a00f90, 13f41855c5, or faecb2ee0a
ACKs for top commit:
fanquake:
ACK fc892c3a80 - tested that this catches issue, i.e #24691:
Tree-SHA512: 9d0d7e6291bfc6f67541a4ff746d374ad8751fefcff6d103d8621c0298b190ab1d209ce96cfc3a0d4a6a5460a9f9bb790eb96027b16e5ff91f2512e40c92ca84
9053f64fcb [doc] release notes for random change target (glozow)
46f2fed6c5 [wallet] remove MIN_CHANGE (glozow)
a44236addd [wallet] randomly generate change targets (glozow)
1e52e6bd0a refactor coin selection for parameterizable change target (glozow)
Pull request description:
Closes#24458 - the wallet always chooses 1 million sats as its change target, making it easier to fingerprint transactions created by the Core wallet. Instead of using a fixed value, choose one randomly each time (within a range). Using 50ksat (around $20) as the lower bound and `min(1 million sat, 2 * average payment value)` as the upper bound.
RFC: If the payment is <25ksat, this doesn't work, so we're using the range (payment amount, 50ksat) instead.
ACKs for top commit:
achow101:
ACK 9053f64fcb
Xekyo:
reACK 9053f64fcb
Tree-SHA512: 45ce5d064697065549473347648e29935733f3deffc71a6ab995449431f60302d1f9911a0994dfdb960b48c48b5d8859f168b396ff2a62db67d535a7db041d35
da2bc865d6 [wallet] don't create long chains by default (glozow)
Pull request description:
Default mempool policy doesn't let you have chains longer than 25 transactions. This is locally configurable of course, but it's not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set `DEFAULT_WALLET_REJECT_LONG_CHAINS` to true.
Closes#9752Closes#10004
ACKs for top commit:
MarcoFalke:
re-ACK da2bc865d6 only change is fixing typos in tests 🎏
Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
The final step of send either produces a PSBT or the final transaction.
We extract these steps to a new helper function `FinishTransaction()` to
reuse them in `sendall`.
049003fe68 coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6adb coinselection: Remove CInputCoin (Andrew Chow)
70f31f1a81 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57b79 coinselection: Add effective value and fees to COutput (Andrew Chow)
f0821230b8 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e15c wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5ad1 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d1916e wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d3bb wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814bbd wallet: Store tx time in COutput (Andrew Chow)
46022953ee wallet: Remove use_max_sig default value (Andrew Chow)
10379f007f scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db41e wallet: cleanup COutput constructor (Andrew Chow)
Pull request description:
While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.
`COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.
ACKs for top commit:
ryanofsky:
Code review ACK 049003fe68, just adding comments and removing == operators since last review
w0xlt:
reACK 049003f
Xekyo:
reACK 049003fe68
Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15
39b1763730 Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)
Pull request description:
Contributes to #21005.
The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.
Notes:
* My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
* GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
* My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.
ACKs for top commit:
achow101:
ACK 39b1763730
ryanofsky:
Code review ACK 39b1763730. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review
Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
fae5d06eed Remove unused feebumper code (MarcoFalke)
Pull request description:
This was accidentally added in commit 0ea47ba7b3. Presumably due to a copy-paste error, as `CreateTransaction` already takes care of the rbf-signal.
ACKs for top commit:
achow101:
ACK fae5d06eed
promag:
Code review ACK fae5d06eed
Tree-SHA512: 81aaf9c6bd9a4e2ad1789880bd8f2191f0ae9ba0a02794aa5db523236ea7df1c0dca078563219d293c694373c0a63c5bf168a85443e86556453ae5439791a618
These operators are used only by the tests in std::mismatch. As
std::mismatch can take a binary predicate, we can use a lambda that
achieves the same instead.
Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.
fa7deaa046 wallet: Pass FastRandomContext& to coin selection (MarcoFalke)
77773b061c wallet: Pass FastRandomContext& to DiscourageFeeSniping (MarcoFalke)
Pull request description:
Passing around a single randomness context shouldn't come with any downsides, but documents better where randomness is used and allows the unit test to be deterministic, if they wish to be so.
ACKs for top commit:
achow101:
ACK fa7deaa046
promag:
Code review ACK fa7deaa046.
glozow:
light code review ACK fa7deaa046
Tree-SHA512: c16287708cc82ce58311710595d0127af42fb156c93fbcaa5bde634ce323d325f4d8c99a74af24423ab22b5ad58163dd771e8b1a0e7d6bff39c9fb2a1cb21bc7
9d2005285c doc: Revise comments and whitespace to clarify (Ben Woosley)
def43a4d88 refactor: Rename i to curr_try in SelectCoinsBnB (Ben Woosley)
1dd0923677 refactor: Track BnB selection by index (Ben Woosley)
Pull request description:
This is prompted by #13167 and presented as a friendly alternative to it.
IMO you can improve code readability and performance by about 20% by tracking the selected utxos by index, rather than by position. This reduces the storage access complexity from roughly O(utxo_size) to O(selection_size).
On my machine (median of 5 trials):
```
BnBExhaustion, 5, 650, 2.2564, 0.000672999, 0.000711565, 0.000693112 - master
BnBExhaustion, 5, 650, 1.76232, 0.000528563, 0.000568806, 0.000539147 - this PR
```
ACKs for top commit:
achow101:
reACK 9d2005285c
glozow:
code review ACK 9d2005285c
Xekyo:
reACK 9d2005285c
Tree-SHA512: 453ea11ad58c48928dc76956e3e98916f6924e95510eb02fe89a899ff102fe9cc08a04d557f381ad0218a210275e5383101d971c1ffad38b06b1c57d81144315
Instead of determining whether the containing transaction is from the
wallet dynamically as needed, just pass it in to COutput and store it.
The transaction ownership isn't going to change.
This is a performance optimization - rather than track all visited values
in a bool vector, track the selected index in a vector. This results in a
complexity reduction of O(utxo_size) to O(selection_size).
fa8e76bb90 wallet: Add sanity checks to AntiFeeSnipe (MarcoFalke)
Pull request description:
I added those sanity checks as part of implementing BIP 326, but I think they make sense on their own. The checks require the transaction to be passed in to `DiscourageFeeSniping`. Also, replace `(int)locktime` cast with the equivalent `int(locktime)` cast.
ACKs for top commit:
chris-belcher:
ACK fa8e76bb90
S3RK:
Code review ACK fa8e76bb90
achow101:
ACK fa8e76bb90
w0xlt:
Code Review ACK fa8e76bb90
Tree-SHA512: 6fe37c85f074851ef09cae8addcb836257089290fecec515c129c3180e9ccb64740aaac76043af10ce7c213e5001568ca6b4b62ae9631f0994ed676b07118074
61152183ab wallet: Add a deprecation warning for newly created legacy wallets (Andrew Chow)
Pull request description:
As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future.
ACKs for top commit:
jonatack:
ACK 61152183ab
S3RK:
reACK 61152183ab
theStack:
ACK 61152183ab
Tree-SHA512: e89bfb8168869542498958f0c9a2ab302dfd43287f8a49e7d9e09f60438a567bb8b7219a4e569797ee819b30b624f532fcc0b70c6aa0edcb392a301b8ce8b541
ec7d73628a [wallet] assert BnB internally calculated waste is the same as GetSelectionWaste() (glozow)
Pull request description:
#22009 introduced a `GetSelectionWaste()` function to determine how much "waste" a coin selection solution has, and is a mirror of the waste calculated inside of `SelectCoinsBnB()`. It would be bad for these two waste metrics to deviate, since it could negatively affect how often we select the BnB solution. Add an assertion to help tests catch a potential accidental change.
ACKs for top commit:
achow101:
ACK ec7d73628a
Xekyo:
ACK ec7d73628a
Tree-SHA512: 3ab7ad45ae92e7ce3f21824fb975105b6be8382edf47c252df5d13d973a3abdcb675132d223b42fcbb669cca879672c904b8a58d0676e12bf381a9219f4db37c
e8272024ab doc: Use human-friendly DefaultHint for change_address/changeAddress in wallet RPC help (Luke Dashjr)
9d5e693c9d Bugfix: doc: Correct type of change_address/changeAddress in wallet RPC help (STR, not STR_HEX) (Luke Dashjr)
Pull request description:
ACKs for top commit:
achow101:
ACK e8272024ab
Tree-SHA512: da4db2b241160c93ea66f8c572c69d4688f52a5fd8c32b66b1192925fcb360baf91be9771eaca178f5b08e1920559174260ed57caddcffade48156ec0c83c0bc
These two implementations of waste calculation should never deviate.
Still keep the SelectCoinsBnB internal calculation because incremental
calculate-as-you-go is much more performant than calling
GetSelectionWaste() over and over again.
8ea6167099 wallet: refactor: dedup sqlite blob binding (Sebastian Falbesoner)
Pull request description:
This refactoring PR deduplicates repeated SQLite blob binding to a statement with a newly introduced helper function `BindBlobToStatement`, abstracting away the calls to `sqlite3_bind_blob(...)`.
This should be more readable and less error-prone, e.g. in case that the error handling has to be adapted. As a slight drawback, the function where the binding happens is not printed anymore (`__func__`), i.e. one could argue this is not strictly a refactoring, but IMHO the advantages of deduplication outweigh this; binding errors are purely internal logic errors (wrong use of the sqlite API) rather than something that is dependend on external data like DB content.
ACKs for top commit:
laanwj:
Code review ACK 8ea6167099
achow101:
ACK 8ea6167099
klementtan:
ACK 8ea6167099
Tree-SHA512: 1de0d214f836bc405a01e98a3a2d71f2deaddc7d23c31cad80219d1614bec92619c06d9a4a091dd563d3e95ffb879649c29745d8f89669b2a5330552c212af3f
7abd8b21ba doc: include wtxid in TransactionDescriptionString (brunoerg)
2d596bce6f doc: add wtxid info in release-notes (brunoerg)
a5b66738f1 test: add wtxid in expected_fields for wallet_basic (brunoerg)
e8c659a297 wallet: add wtxid in WalletTxToJSON (brunoerg)
7482b6f895 wallet: add GetWitnessHash() (brunoerg)
Pull request description:
This PR add `wtxid` in `WalletTxToJSON` which allows to return this field in `listsinceblock`, `listtransactions` and `gettransaction` (RPCs).
ACKs for top commit:
achow101:
re-ACK 7abd8b21ba
w0xlt:
crACK 7abd8b2
luke-jr:
re-utACK 7abd8b21ba
Tree-SHA512: f86f2dbb5e38e7b19932006121802f47b759d31bdbffe3263d1db464f6a3a30fddd68416f886a44f6d3a9fd570f7bd4f8d999737ad95c189e7ae5e8ec1ffbdaa
6c23c41561 refactor: Rewrite AddToBlockIndex with try_emplace (Carl Dong)
c05cf7aa1e style: Modernize range-based loops over m_block_index (Carl Dong)
c2a1655799 style-only: Use using instead of typedef for BlockMap (Carl Dong)
dd79dad175 refactor: Rewrite InsertBlockIndex with try_emplace (Carl Dong)
531dce0347 tests: Remove now-unnecessary manual Unload's (Carl Dong)
bec86ae326 blockstorage: Make m_block_index own CBlockIndex's (Carl Dong)
Pull request description:
Part of: #24303
Split off from: #22564
```
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
```
The second commit demonstrates how this makes calls to `Unload()` to satisfy the address sanitizer unnecessary.
ACKs for top commit:
ajtowns:
ACK 6c23c41561
MarcoFalke:
re-ACK 6c23c41561🎨
Tree-SHA512: 81b2b5119be27cc0f8a9457b11da60cc60930315d2a5be36be89fe253d32073ffe622348ff153114b9b3212197bddbc791810913a43811b33cc58e7162bd105b
c4d76c6faa tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
c7376cc8d7 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43 wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d7
benthecarman:
tACK c7376cc8d7 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
7f3a6a9495 wallet: Add external-signer-support specific error message (Hennadii Stepanov)
Pull request description:
On master (5f44c5c428) an attempt to load an external signer wallet using Bitcoin Core compiled without external signer support fails with the following log messages:
```
2022-02-20T19:01:11Z [qt-walletctrl] Using SQLite Version 3.31.1
2022-02-20T19:01:11Z [qt-walletctrl] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/coldcard-0220
2022-02-20T19:01:11Z [qt-walletctrl] init message: Loading wallet…
2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Error: External signer wallet being loaded without external signer support compiled
2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Releasing wallet
```
While log messages are good, a message in the GUI window is completely misleading:
This PR fixes this issue:
ACKs for top commit:
achow101:
ACK 7f3a6a9495
kristapsk:
ACK 7f3a6a9495
brunoerg:
crACK 7f3a6a9495
Tree-SHA512: a4842751c0ca8a37ccc3ea00503678f6b712a7f53d6cbdc07ce02dcb85ca8a94890d1c2da20307be043faa347747abeba29185c88ba12edd5253bfca56531585
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
A change to BlockManager::LookupBlockIndex:
- Previously, it was a const member function returning a non-const CBlockIndex*
- Now, there's are const and non-const versions of
BlockManager::LookupBlockIndex returning a CBlockIndex with the same
const-ness as the member function:
(e.g. const CBlockIndex* LookupBlockIndex(...) const)
See next commit for some weirdness that this eliminates.
The range based for-loops are modernize (using auto + destructuring) in
a future commit.
48742693ac Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd434 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693ac
hebasto:
re-ACK 48742693ac, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
Refuse to load a wallet if it requires a rescan lower than the height of
an unvalidated snapshot we're running -- in more general terms, if we
don't have data for the blocks.
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
3ee6d0788e test: add more wallet conflicts assertions (S3RK)
3b98bf9c43 Revert "Add to spends only transcations from me" (S3RK)
Pull request description:
This reverts commit d04566415e from #22929.
This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature.
ACKs for top commit:
achow101:
ACK 3ee6d0788e
Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
fa4339e4c1 Extract CTxIn::MAX_SEQUENCE_NONFINAL constant (MarcoFalke)
Pull request description:
Extracting the constant makes it possible to attach documentation to it.
Also, rework the docs for the other "sequence constants".
ACKs for top commit:
w0xlt:
reACK fa4339e for specifying the transaction version.
darosior:
re-ACK fa4339e4c1
luke-jr:
crACK fa4339e4c1
Tree-SHA512: 8d8f3dd5afb33eb5b72aa558e1e03de874c5ed02aa1084888e92ed86f3aaa5c725db45ded02e14cdfa67a92ac6774e97185b697f20a8ab63abbfcaa2fcd1fc6a
fa5d2e678c Remove unused char serialize (MarcoFalke)
fa24493d63 Use spans of std::byte in serialize (MarcoFalke)
fa65bbf217 span: Add BytePtr helper (MarcoFalke)
Pull request description:
This changes the serialize code (`.read()` and `.write()` functions) to take a `Span` instead of a pointer and size. This is a breaking change for the serialize interface, so at no additional cost we can also switch to `std::byte` (instead of using `char`).
The benefits of using `Span`:
* Less verbose and less fragile code when passing an already existing `Span`(-like) object to or from serialization
The benefits of using `std::byte`:
* `std::byte` can't accidentally be mistaken for an integer
The goal here is to only change serialize to use spans of `std::byte`. If needed, `AsBytes`, `MakeUCharSpan`, ... can be used (temporarily) to pass spans of the right type.
Other changes that are included here:
* [#22167](https://github.com/bitcoin/bitcoin/pull/22167) (refactor: Remove char serialize by MarcoFalke)
* [#21906](https://github.com/bitcoin/bitcoin/pull/21906) (Preserve const in cast on CTransactionSignatureSerializer by promag)
ACKs for top commit:
laanwj:
Concept and code review ACK fa5d2e678c
sipa:
re-utACK fa5d2e678c
Tree-SHA512: 08ee9eced5fb777cedae593b11e33660bed9a3e1711a7451a87b835089a96c99ce0632918bb4666a4e859c4d020f88fb50f2dd734216b0c3d1a9a704967ece6f
6ea5682784 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack)
5d59ae0ba8 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov)
eaeeb88768 Require IsBlockPruned() to hold mutex cs_main (Jon Atack)
ca47b00577 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov)
e9f3aa5f6a Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov)
8ef457cb83 Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov)
572393448b Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack)
2e557ced28 Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack)
6fd4341c10 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack)
Pull request description:
Issues:
- `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step:
- `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`.
This pull:
- adds thread safety lock annotations for the functions listed above
- guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main
How to review and test:
- debug build with clang and verify there are no `-Wthread-safety-analysis` warnings
- review the code to verify each annotation or lock is necessary and sensible, or if any are missing
- look for whether taking a lock can be replaced by a lock annotation instead
- for more information about Clang thread safety analysis, see
- https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization
Mitigates/potentially closes#17161.
ACKs for top commit:
laanwj:
Code review ACK 6ea5682784
Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
fac8165443 Remove unused checkFinalTx (MarcoFalke)
fa272eab44 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3 qt: Treat unconfirmed txs as unconfirmed (MarcoFalke)
Pull request description:
The wallet has several issues:
## Unconfirmed txs in the GUI
The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics.
## Confirmed txs in the wallet
The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`.
The issues are fixed in separate commits and there is even a test.
ACKs for top commit:
achow101:
ACK fac8165443
prayank23:
reACK fac8165443
glozow:
code review ACK fac8165443, I understand now how this fixes both issues.
Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
Coin selection requires knowing the weight of a transaction so that fees
can be estimated. However for external inputs, the weight may not be
avialble, and solving data may not be enough as the input could be one
that we do not support. By allowing users to specify input weights,
those external inputs can be included in the transaction.
Additionally, if the weight for an input is specified, that value will
always be used, regardless of whether the input is in the wallet or
solving data is available. This allows us to account for scenarios where
the wallet may be more conservative and estimate a larger input than may
actually be created.
For example, we assume the maximum DER signature size, but an external
input may be signed by a wallet which does nonce grinding in order to get
a smaller signature. In that case, the user can specify the smaller
input weight to avoid overpaying transaction fees.
Refactors TopUp so that it also tops up inactive chains. The bulk of
TopUp is moved to TopUpChain.
CHDChain also has 2 new in memory variables to track its highest used
indexes. This is used only for inactive hd chains so that they can be
topped up later in the same session (e.g. if the wallet is encrypted and
not unlocked at the time of MarkUnusedAddresses).
e5b6aef612 Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef612
MarcoFalke:
Concept ACK e5b6aef612🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
3a45dc36a6 Change type of `backup_file` parameter in RestoreWallet/restoreWallet (Hennadii Stepanov)
213172c734 refactor: Block unsafe std::string fs::path conversion copy_file calls (Hennadii Stepanov)
Pull request description:
This PR is an optional prerequisite for bitcoin/bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which:
- makes further code changes safer
- prevents [some](https://cirrus-ci.com/task/6525835388649472) test failures on native Windows
ACKs for top commit:
ryanofsky:
Code review ACK 3a45dc36a6. Looks great! Thanks for debugging and fixing this and making #20744 smaller!
Tree-SHA512: c6dfaef6b45b9c268bc9ee9b943b9d679152c9d565ca4f86da8d33f8eb9b3cdbe9ba6df7b7578eacc0d00db6551048beff97419f86eb4b1d3182c43e2b4eb9a5
`fs::path` looks more native than `std::string` for a parameter which
represents a backup file. This change eliminates back-and-forth type
conversions.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding
copy_file calls that will be unsafe after the transition to
std::filesystem to due lack of a boost::filesystem::path::imbue
equivalent and inability to set a predictable locale.
fa8e01a5b8 doc: Remove outdated scriptChange TODO comment (MarcoFalke)
Pull request description:
This was added in commit bf798734db (raw multisig). Raw multisig isn't a thing, so remove the TODO.
ACKs for top commit:
S3RK:
ACK fa8e01a5b8
achow101:
ACK fa8e01a5b8
Tree-SHA512: 01d521ca3605ab130c43531da4922ea85461ca5e7436267a34fb5df348009e086b3c66d85532c62255d9a0ba43db56424884808e773d0ef0177035dfb25d6a6c
fa68a6c2fc scripted-diff: Rename touched member variables (MarcoFalke)
facd3df21f Make blockstorage globals private members of BlockManager (MarcoFalke)
faa8c2d7d7 doc: Clarify nPruneAfterHeight for signet (MarcoFalke)
fad381b2f8 test: Load genesis block to allow flush (MarcoFalke)
fab262174b Move blockstorage-related unload to BlockManager::Unload (MarcoFalke)
fa467f3913 move-only: Create WriteBlockIndexDB helper (MarcoFalke)
fa88cfd3f9 Move functions to BlockManager (MarcoFalke)
Pull request description:
Globals aren't too nice because they hide dependencies, also they make testing harder.
Fix that by removing some.
ACKs for top commit:
Sjors:
ACK fa68a6c2fc
ryanofsky:
Code review ACK fa68a6c2fc. Nice changes!
Tree-SHA512: 6abc5929a5e43a05e238276721d46a64a44f23dca18c2caa9775437a32351d6815d88b88757254686421531d0df13861bbd3a202e13a3192798d87a96abef65d
ac617cc141 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
fa993d0e7e doc: Fix -changetype help text (MarcoFalke)
Pull request description:
This was forgotten in commit 3ac38058ce
ACKs for top commit:
shaavan:
ACK fa993d0e7e
w0xlt:
ACK fa993d0
josibake:
ACK fa993d0e7e
Tree-SHA512: 9f84b1168e3b3ab06e5c1f4915a1874598b273099eb5878ed28c3a66f1484e34c836fd3c68c4131bee541f3428052f6b66e02b192170752d1082de689d44cd4d
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
a2a92317ad rpc: Add warning to user about newkeypool command (Samuel Dobson)
Pull request description:
This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.
David Harding also suggested [here](https://github.com/bitcoin/bitcoin/pull/23093#issuecomment-945350003) that the RPC help text should include a warning to the users about the interaction between newkeypool.
ACKs for top commit:
achow101:
ACK a2a92317ad
Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
65efbba45d rpcwallet: mention labels are deactivated for ranged descriptors (Antoine Poinsot)
Pull request description:
It was confusing when trying to use it as a blackbox. So mention it so next ones don't have to open the said box :)
See #23749 for context
ACKs for top commit:
Sjors:
utACK 65efbba45d
achow101:
ACK 65efbba45d
Tree-SHA512: d8a3d1f81c16d95855ac2b01e8fd20e83d6dac1721b3da464a9a890e46102992a6882918be87b2a28b929349ee7f1beb1af6c88b22f065fbbb6948275a6d2b8f
62fa61fa4a refactor: remove the wallet folder if the restore fails (w0xlt)
abbb7eccef refactor: Move restorewallet() RPC logic to the wallet section (w0xlt)
4807f73f48 refactor: Implement restorewallet() logic in the wallet section (w0xlt)
Pull request description:
Currently `restorewallet()` logic is written in the RPC layer and it can´t be reused by GUI. So it moves this to the wallet section and then, GUI can access it.
This is necessary to implement the "Restore Wallet" menu item in the GUI (which is already implemented in https://github.com/bitcoin-core/gui/pull/471 ).
This commit also simplifies error handling and adds a new behavior: if the restore fails, the invalid wallet folder is removed.
ACKs for top commit:
achow101:
ACK 62fa61fa4a
shaavan:
crACK 62fa61fa4a
Tree-SHA512: 7ccfbad5943f38616ba0c2dd443c97a4b5bc1f6612dbf5a9e7a0263100aba36671fae929a2e7688442667be394645f44484af137a4802f204a33c4689eb27c39
Currently restorewallet() logic is written in the RPC layer
and it can´t be reused by GUI. So it reimplements this in the
wallet and interface sections and then, GUI can access it.
d5cab1a96d Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson)
e46fc935aa Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson)
d1a9742623 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson)
Pull request description:
Fixes#21368
Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different.
ACKs for top commit:
achow101:
ACK d5cab1a96d
Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
c0405ee27f rpc: Document that DEFAULT is for Taproot, ALL for everything else (Andrew Chow)
d3992669df psbt: Actually use SIGHASH_DEFAULT (Andrew Chow)
eb9a1a2c59 psbt: Make sighash_type std::optional<int> (Andrew Chow)
Pull request description:
Make the behavior align with the help text by actually using SIGHASH_DEFAULT as the default sighash for signing PSBTs.
ACKs for top commit:
Sjors:
re-utACK c0405ee27f
Tree-SHA512: 5199fb41de416b2f10ac451f824e7c94b428ba11fdb9e50f0027c692e959ce5813a340c34a4e52d7aa128e12008303d80939a693eff36a869720e45442119828
5493e92501 Check descriptors returned by external signers (sstone)
Pull request description:
Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
See https://github.com/bitcoin/bitcoin/issues/23627 for context.
The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`.
I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`.
ACKs for top commit:
jamesob:
Code review ACK 5493e92501
achow101:
ACK 5493e92501
Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04
05300c1439 Use SelectionResult in SelectCoins (Andrew Chow)
9d9b101d20 Use SelectionResult in AttemptSelection (Andrew Chow)
bb50850a44 Use SelectionResult for waste calculation (Andrew Chow)
e8f7ae5eb3 Make an OutputGroup for preset inputs (Andrew Chow)
51a9c00b4d Return SelectionResult from SelectCoinsSRD (Andrew Chow)
0ef6184575 Return SelectionResult from KnapsackSolver (Andrew Chow)
60d2ca72e3 Return SelectionResult from SelectCoinsBnB (Andrew Chow)
a339add471 Make member variables of SelectionResult private (Andrew Chow)
cbf0b9f4ff scripted-diff: Use SelectionResult in coin selector tests (Andrew Chow)
9d1d86da04 Introduce SelectionResult struct (Andrew Chow)
94d851d28c Fix bnb_search_test to use set equivalence for (Andrew Chow)
Pull request description:
Instead of returning a set of selected coins and their total value as separate items, encapsulate both of these, and other variables, into a new `SelectionResult` struct. This allows us to have all of the things relevant to a coin selection solution be in a single object. `SelectionResult` enables us to implement the waste calculation in a cleaner way.
All of the coin selection functions (`SelectCoinsBnB`, `KnapsackSolver`, `AttemptSelection`, and `SelectCoins`) are changed to use a `SelectionResult` as the output parameter.
Based on #22009
ACKs for top commit:
laanwj:
Code review ACK 05300c1439
Tree-SHA512: e4dbb4d78a6cda9c237d230b19e7265591efac5a101a64e6970f0654e2c4f93d13bb5d07b98e8c7b8d37321753dbfc94c28c3a7810cb1c59b5bc29b08a8493ef
3431839c33 util, refactor: Improve headers for bitcoin-wallet tool (Hennadii Stepanov)
Pull request description:
This PR:
- removes unneeded `#include <wallet/wallet.h>` from `<wallet/wallettool.h>`
- introduces class forward declaration in `<wallet/wallettool.h>`
- added `#include <config/bitcoin-config.h>` to `wallet/wallettool.cpp` where the `USE_BDB` macro is used
Top commit has no ACKs.
Tree-SHA512: a0de560d821f8b570ae806a1165b9b382c9e0b339687d932052fa4c38ab2ba493e7e050f19adc02ad7db40c42cf88ac1d37209f9071494a0ab268ed33ff22b9f
fa9aaf8694 scripted-diff: Use named args in RPC docs (MarcoFalke)
Pull request description:
Incorrect named args are source of bugs, like #22979.
To allow them being checked by `clang-tidy`, use a format it can understand.
ACKs for top commit:
fanquake:
ACK fa9aaf8694 - checked `clang-tidy` and it's fine here, (but throwing errors in other files. i.e `wallet/test/wallet_tests.cpp`).
Tree-SHA512: e09dae8ee999a5c4819e6f848c12139593ca0e915e645c8fabeb97c379188fb9104d286c02c71f590abc64cdec125f78026735f83e016111976baa49d588a9bc
When topping up an inactive HD chain, either key_origin will be
available and we can use the path given there, or we need to figure out
the path from the string hdKeypath.
ffd11ea876 Fix typo and grammar (Heebs)
Pull request description:
Fix typo and grammar in the coin selection algorithm's description.
ACKs for top commit:
meshcollider:
ACK ffd11ea876
Tree-SHA512: bba07c2efd5140fb3e021618739d70aaa761bbc274afb8158809492b0606773c217e42e58e58b18a2454b9c45ebc883ebece17cdc467ac60e3d3140d7a979db7
1dcba996d3 Coinbase receivedby rpcs release notes (Andrew Toth)
b5696750a9 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth)
bce20c34d6 Include coinbase transactions in receivedby wallet rpcs (Andrew Toth)
Pull request description:
The current `*receivedby*` RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction *is* receiving those coins.
This PR corrects this behaviour. Also, a new option `include_immature_coinbase` is added (default=`false`) that includes immature coinbase transactions when set to true.
However, since this is potentially a breaking change this PR introduces a hidden configuration option `-deprecatedrpc=exclude_coinbase`. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release.
Fixes https://github.com/bitcoin/bitcoin/issues/14654.
ACKs for top commit:
jnewbery:
reACK 1dcba996d3
Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df
fa5362a9a0 rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs (MarcoFalke)
Pull request description:
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
ACKs for top commit:
meshcollider:
utACK fa5362a9a0
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
31ba1af74a Remove unused (and broken) functionality in SpanReader (Pieter Wuille)
Pull request description:
This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since #23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`.
It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`.
This was pointed out by achow101 in https://github.com/bitcoin/bitcoin/pull/23653#discussion_r763370432.
ACKs for top commit:
jb55:
crACK 31ba1af74a
achow101:
ACK 31ba1af74a
Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
