3431839c33fa3892c982322e4add39e28ddba719 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
fa9aaf8694c63200ad01fd5659aba4f409b2c3b9 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 fa9aaf8694c63200ad01fd5659aba4f409b2c3b9 - checked `clang-tidy` and it's fine here, (but throwing errors in other files. i.e `wallet/test/wallet_tests.cpp`).
Tree-SHA512: e09dae8ee999a5c4819e6f848c12139593ca0e915e645c8fabeb97c379188fb9104d286c02c71f590abc64cdec125f78026735f83e016111976baa49d588a9bc
ffd11ea87640c8a3d83b6f83ac18e65234fc6002 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 ffd11ea87640c8a3d83b6f83ac18e65234fc6002
Tree-SHA512: bba07c2efd5140fb3e021618739d70aaa761bbc274afb8158809492b0606773c217e42e58e58b18a2454b9c45ebc883ebece17cdc467ac60e3d3140d7a979db7
1dcba996d30d83aebe8c73f42f5d4056d6472166 Coinbase receivedby rpcs release notes (Andrew Toth)
b5696750a925c07261287b043ffdfb393cbb1327 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth)
bce20c34d6b999e700a560f95351c212ed8c36f4 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 1dcba996d30d83aebe8c73f42f5d4056d6472166
Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df
fa5362a9a0c5665c1a4de51c3ce4758c93a9449e 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 fa5362a9a0c5665c1a4de51c3ce4758c93a9449e
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
31ba1af74a0aaec690a01ea061264a6d5039d885 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 31ba1af74a0aaec690a01ea061264a6d5039d885
achow101:
ACK 31ba1af74a0aaec690a01ea061264a6d5039d885
Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
576720850467b7b21ca1ab59deab27b7a0c1c176 correct rpc address_type helptext (brianddk)
Pull request description:
RPC calls `getnewaddress`/`getrawchangeaddress` support the address_type of `bech32m` but it is omitted in the `RPCHelpMan` help text.
The `createmultisig` and `addmultisigaddress` help text was not updated since `bech32m` is not yet supported in these.
ACKs for top commit:
shaavan:
ACK 576720850467b7b21ca1ab59deab27b7a0c1c176
Tree-SHA512: 3c0cfb96019ca6d316c4a2fe27786d1b621c49b31b3aa61068bad737a5a0ceed89babad704b9923f9aedcabfa670d752916803bdf22236403061ddf9295a2637
fa37e798b2660d8e44e31c944a257b55aeef5de2 wallet: Replace confusing getAdjustedTime() with GetTime() (MarcoFalke)
Pull request description:
Setting `nTimeReceived` to the adjusted time has several issues:
* `m_best_block_time` is set to the "unadjusted" time, thus a comparison of the two times is like comparing apples to oranges. In the worst case this opens up an attack vector where remote peers can force a premature re-broadcast of wallet txs.
* The RPC documentation for `"timereceived"` doesn't mention that the network adjusted time is used, possibly confusing users when the time reported by RPC is off by a few seconds compared to their local timestamp.
Fix all issues by replacing the call with `GetTime()`. Also a style fix: Use non-narrowing integer conversion in the RPC method.
ACKs for top commit:
theStack:
Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2
shaavan:
crACK fa37e798b2660d8e44e31c944a257b55aeef5de2
Tree-SHA512: 8d020ba400521246b7aed4b6c41319fc70552e8c69e929a5994500375466a9edac02a0ae64b803dbc6695df22276489561a23bd6e030c44c97d288f7b9b2b3fa
This removes the ability to set an offset in the SpanReader constructor,
as the current code is broken. 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}.
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.
11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 More Span simplifications (Pieter Wuille)
568dd2f83900a11a4dbba1250722791a135bf0a9 Replace MakeSpan helper with Span deduction guide (Pieter Wuille)
Pull request description:
C++17 supports [user-defined deduction guides](https://en.cppreference.com/w/cpp/language/class_template_argument_deduction), allowing class constructors to be invoked without specifying class template arguments. Instead, the code can contain rules to infer the template arguments from the constructor argument types.
This alleviates the need for the `MakeSpan` helper. Convert the existing MakeSpan rules into deduction rules for `Span` itself, and replace all invocations of `MakeSpan` with just `Span` ones.
ACKs for top commit:
MarcoFalke:
re-ACK 11daf6ceb1d9ea1f8d638b123eecfe39d162a7c3 Only change is removing a hunk in the tests 🌕
Tree-SHA512: 10f3e82e4338f39d9b7b407cd11aac7ebe1e9191b58e3d7f4e5e338a4636c0e126b4a1d912127c7446f57ba356c8d6544482e47f97901efea6a54fffbfd7895f
2c35a93b3cc19dc71d5664f9f61c24a04f419e35 Generalize/simplify VectorReader into SpanReader (Pieter Wuille)
Pull request description:
Originally written for #21590 (safegcd-based MuHash inverses), but then found a better way that removed the need for it, so I'm submitting it independently.
ACKs for top commit:
MarcoFalke:
re-ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35 🖨
shaavan:
ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35
Tree-SHA512: 959e3251e0cfe20e13a50639b617c9dc2a561d613a0884d983c93d15dacb6d2305d760aa933d18ba055cef8a1651a344bcb6b3f93051ecf26d3f2efc5779efa4
3d71d16d1eb4173c70d4c294559fc2365e189856 test: listtranscations with externally generated addresses (S3RK)
d04566415e16ae685af066384f346dff522c068f Add to spends only transcations from me (S3RK)
9f3a622b1cea37e452560f2f82d8e82d3b48a73a Automatically add labels to detected receiving addresses (S3RK)
c1b99c088c54eb101c0a28a67237965576ccf5ad Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses (S3RK)
03840c20640685295a65ed8c82456e877f668b9b Add CWallet::IsInternalScriptPubKeyMan (S3RK)
456e350926adde5dabdbc85fc0f017fb29bdadb3 wallet: resolve ambiguity of two ScriptPubKey managers providing same script (S3RK)
Pull request description:
This PR fixes certain use-cases when **send-to-self** transactions are missing from `listtransactions` output.
1. When a receiving address is generated externally to the wallet
(e.g. same wallet running on two nodes, or by 3rd party from xpub)
2. When restoring backup with lost metadata, but keypool gap is not exceeded yet
When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label.
Works both for legacy and descriptors wallets.
- For legacy it uses the internal flag from the keypool entry. Caveat: because we don't know which script type would be used we add all possible destinations for such keys.
- For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors.
fixes#19856fixes#20293
ACKs for top commit:
laanwj:
Code review ACK 3d71d16d1eb4173c70d4c294559fc2365e189856
Tree-SHA512: 03fafd5548ead0c4ffe9ebcc9eb2849f1d2fa7270fda4166419b86877d4e57dcf04460e465fbb9c90b42031f3c05d1b83f1b67a9f82c2a42980825ed1e7b52e6
ff945e553affbb7e25da1257a0dd47e413ec5164 MOVEONLY: Move utility functions from rpcwallet to wallet/rpc/util (Samuel Dobson)
7b04a064f6e8ee9d93e5a5ad54dab20b769083f2 Introduce wallet/rpc/util (Samuel Dobson)
Pull request description:
This is part one of multiple to split up rpcwallet.cpp into smaller, more logical units.
See #23622 for context and overall plan. I'll open PRs in stages to hopefully minimise conflicts.
Can be reviewed with `--color-moved=dimmed-zebra`
The end goal can be seen here: https://github.com/meshcollider/bitcoin/tree/202111_split_walletrpc
ACKs for top commit:
MarcoFalke:
nice, ACK ff945e553affbb7e25da1257a0dd47e413ec5164 🐰
shaavan:
ACK ff945e553affbb7e25da1257a0dd47e413ec5164
Tree-SHA512: 6e3d1de6db770fe2fca540c8c4f30183ab8258c26e3a1fb46937714d28818a52933eafbfcafe2995f6a6e2551a3f3dd3ec93363b81de7912c0d81f5749d1c86d
c771ee8571dc63d27747ef565671eb1dcdc0ff4b doc: use BIP125-replaceable (fanquake)
36dc5bb8cb1d71356d645338b5b49361f3bda956 doc: Extract CreateTxDoc in rawtransaction (fanquake)
Pull request description:
For the fields: inputs, outputs, locktime, replaceable. Similar to #23172.
Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
MarcoFalke:
ACK c771ee8571dc63d27747ef565671eb1dcdc0ff4b 😸
Tree-SHA512: e6e4211b89bedec472f8381b3cbea5670f82b728955888c794f694164b8d8bdd51a99c64762b625357ac2171005712b82f81ee7c1b8f5c5620bdedeeefa2b9da
fa00447442f22a24e5ca5fc538d0bf7bef575544 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke)
fae13c39896898aef2281433af143c22d8b3a3b4 doc: Use clang-tidy comments in crypto_tests (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:
shaavan:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
rajarshimaitra:
ACK fa00447442
jonatack:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
fanquake:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
Tree-SHA512: 4d23a8363da81dfea21a4cd8516ab5e0dc70119e4d503f3f240f38573218b2c2e84083b97e956c62942d78b2f17490f8b3b2e8077d257644fda1d901e2b80507
fae239208d3676452a755f736ee5aaa17adeb493 wallet: Split signmessage from rpcwallet (MarcoFalke)
Pull request description:
rpcwallet is the file that takes longest to compile, especially with sanitizers enabled it can take several 10s of seconds.
Allow faster incremental and parallel builds by starting to split it up. First, split off `signmessage`, which is unrelated to other stuff such as wallet file handling, wallet encryption, tx creation, or wallet status/info.
ACKs for top commit:
ryanofsky:
Code review ACK fae239208d3676452a755f736ee5aaa17adeb493. Confirmed move only
meshcollider:
Code review ACK fae239208d3676452a755f736ee5aaa17adeb493
Tree-SHA512: 250445cd544e39376f225871270cdcae462f16cfd9d25ede4b148e915642bfac9ee7ef3e8eccdd2443dc74dbf794d3bcd5fe5c58b1d05a2dcec70b8e03b37dff
a99ed8986554fa1ecc854e43ea373d957e598db8 psbt: sign without finalizing (Andrew Chow)
Pull request description:
It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default.
This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast.
ACKs for top commit:
meshcollider:
utACK a99ed8986554fa1ecc854e43ea373d957e598db8
Sjors:
utACK a99ed89
Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6
fa2c991ec93bc72d276f0dcd747b3e57c246139b refactor: Use underlying type of isminetype for isminefilter (MarcoFalke)
Pull request description:
This does not change behavior, but it would be good for code clarity and to avoid `-Wimplicit-int-conversion` compiler warnings to use the an int of the same width for both `isminetype` and `isminefilter`.
ACKs for top commit:
laanwj:
Code review ACK fa2c991ec93bc72d276f0dcd747b3e57c246139b
shaavan:
crACK fa2c991ec93bc72d276f0dcd747b3e57c246139b
promag:
Code review ACK fa2c991ec93bc72d276f0dcd747b3e57c246139b.
Tree-SHA512: b3e255de7c9b1dea272bc8cb9386b339fe701f18580e03e997c270cac6453088ca2032e26e39f536d66cd1b6fda3e96bdbdc6e960879030e635338d0916277e6
f13a22a631efe01e1fbae4ae78a4901d14ebda3c wallet: Call load handlers without cs_wallet locked (João Barbosa)
Pull request description:
Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
ACKs for top commit:
meshcollider:
re-utACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
ryanofsky:
Code review ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c. Only change since last review is adding a lock order comment
jonatack:
ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
d8ee8f3cd32bbfefec931724f5798cbb088ceb6f refactor: Make CWalletTx sync state type-safe (Russell Yanofsky)
Pull request description:
Current `CWalletTx` state representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form.
For example, it is possible to call `setConflicted` without setting a conflicting block hash, or `setConfirmed` with no transaction index. And it's possible update individual `m_confirm` and `fInMempool` data fields without setting an overall consistent state that can be serialized and handled correctly.
Fix this without changing behavior by using `std::variant`, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible.
This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.
ACKs for top commit:
laanwj:
re-ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f
jonatack:
Code review ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f
Tree-SHA512: b9f15e9d99dbdbdd3ef7a76764e11f66949f50e6227e284126f209e4cb106af6d55e9a9e8c7d4aa216ddc92c6d5acc6f4aa4746f209bbd77f03831b51a2841c3
fa3e0da06b491b8c0fa2dbae37682a9112c9deb8 policy: Treat taproot as always active (MarcoFalke)
Pull request description:
Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things:
* Importing `tr` descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate `tr` descriptors by default (regardless of the taproot status) after commit 47fe7445e7f54aee10ec6dbc53f1db1adbeb43de.
* Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe7445e7f54aee10ec6dbc53f1db1adbeb43de, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See https://github.com/bitcoin/bitcoin/issues/11887.
A similar change was done for segwit v0 in https://github.com/bitcoin/bitcoin/pull/13120 .
This effectively reverts commit c5ec0367d718544caa3a1578d6c730fc92ee4e94.
ACKs for top commit:
mjdietzx:
Code Review ACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
achow101:
ACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
sipa:
utACK fa3e0da06b491b8c0fa2dbae37682a9112c9deb8
gruve-p:
ACK fa3e0da06b
gunar:
Code Review + tACK fa3e0da06
rajarshimaitra:
code review + tACK fa3e0da06b
Tree-SHA512: c6dc7a4e6c345bdec33f256847dc63906ab1696aa683ab9b32a79e715613950884ac3a1a7a44e95f31bb28e58dd64679a616175f7e152b21f5550f3337c8e622
4868c9f1b39f03adee0009cd41d96598b43e8b78 Extract Taproot internal keyid with GetKeyFromDestination (Andrew Chow)
d8abbe119c71f917e0fd2e80536c1e5d979b4dc6 Mention bech32m in -addresstype and -changetype help (Andrew Chow)
8fb57845ee3844c9ba854471065109d2e409300f Create a tr() descriptor bech32m DescriptorScriptPubKeyMan by default (Andrew Chow)
54b3699862de687f782c7c52500d6a2372478355 Store pubkeys in TRDescriptor::MakeScripts (Andrew Chow)
Pull request description:
Make a `tr()` descriptor by default in descriptor wallets so that users will be able to make and use segwit v1 bech32m addresses.
ACKs for top commit:
MarcoFalke:
Concept ACK 4868c9f1b39f03adee0009cd41d96598b43e8b78
Sjors:
re-utACK 4868c9f1b39f03adee0009cd41d96598b43e8b78
gruve-p:
ACK 4868c9f1b3
meshcollider:
Concept + code review ACK 4868c9f1b39f03adee0009cd41d96598b43e8b78
Tree-SHA512: e5896e665b8d559f1d759b6582d1bb24f70d4698a57307684339d9fdcdac28ae9bc17bc946a7efec9cb35c130a95ffc36e3961a335124ec4535d77b8d00e9631
ee03c782ba61993d9e95fa499546cd14cee35445 wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets (Hennadii Stepanov)
3e4f069d23cd2ea5de8fa3c4b1a761ab097ad56f wallet, refactor: Make GetOldestKeyPoolTime return type std::optional (Hennadii Stepanov)
Pull request description:
The "keypoololdest" field in the `getwalletinfo` RPC response should be used for legacy wallets only.
Th current implementation (04437ee721e66a7b76bef5ec2f88dd1efcd03b84) assumes that `CWallet::GetOldestKeyPoolTime()` always return `0` for descriptor wallets. This assumption is wrong for _blank_ descriptor wallets, when `m_spk_managers` is empty. As a result:
```
$ src/bitcoin-cli -signet -rpcwallet=211024-d-DPK getwalletinfo
{
"walletname": "211024-d-DPK",
"walletversion": 169900,
"format": "sqlite",
"balance": 0.00000000,
"unconfirmed_balance": 0.00000000,
"immature_balance": 0.00000000,
"txcount": 0,
"keypoololdest": 9223372036854775807,
"keypoolsize": 0,
"keypoolsize_hd_internal": 0,
"paytxfee": 0.00000000,
"private_keys_enabled": false,
"avoid_reuse": false,
"scanning": false,
"descriptors": true
}
```
This PR fixes this issue with direct checking of the `WALLET_FLAG_DESCRIPTORS` flag.
ACKs for top commit:
lsilva01:
re-ACK ee03c78
stratospher:
ACK ee03c78.
meshcollider:
Code review ACK ee03c782ba61993d9e95fa499546cd14cee35445
Tree-SHA512: 9852f9f8ed5c08c07507274d7714f039bbfda66da6df65cf98f67bf11a600167d0f7f872680c95775399477f4df9ba9fce80ec0cbe0adb7f2bb33c3bd65b15df
