Since the only remaining isminetypes are ISMINE_NO and ISMINE_SPENDABLE,
this enum is now just a bool and can be removed. IsMine is changed to
return a bool and any usage of isminetypes and isminefilters are changed
to be the remaining ISMINE_SPENDABLE case.
This isminetype is not a real isminetype as it is never returned by
IsMine. This is only used for isminefilters in one function, which can
be better represented with a bool parameter avoid_reuse.
In descriptor wallets, we consider all outputs to be spendable as we no
longer have mixed mine and watchonly in a wallet. As such,
COutput::spendable is meaningless and can be removed.
Furthermore, CoinFilterParams::only_spendable can be removed as that was
essentially checking for COutput::spendable.
Lastly, AvailableCoinsListUnspent can also be removed as the wrapper is
now only setting the feerate to std::nullopt which is trivial enough that
a dedicated wrapper is not needed.
5c8bf7b39e doc: add release notes for version 3 transactions (ishaanam)
4ef8065a5e test: add truc wallet tests (ishaanam)
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam)
2cb473d9f2 rpc: Support version 3 transaction creation (Bue-von-hon)
4c20343b4d rpc: Add transaction min standard version parameter (Bue-von-hon)
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam)
da8748ad62 wallet: limit v3 tx weight in coin selection (ishaanam)
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam)
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam)
cc155226fe wallet: set m_version in coin control to default value (ishaanam)
2e9617664e wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam)
ec2676becd wallet: unconfirmed ancestors and descendants are always truc (ishaanam)
Pull request description:
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from #31936)
- Limits a v3 transaction weight in coin selection
Closes#31348
To-Do:
- [x] Test a v3 sibling conflict kicking out one of our transactions from the mempool
- [x] Implement separate size limit for TRUC children
- [x] Test that we can't fund a v2 transaction when everything is v3 unconfirmed
- [x] Test a v3 sibling conflict being removed from the mempool
- [x] Test limiting v3 transaction weight in coin selection
- [x] Simplify tests
- [x] Add documentation
- [x] Test that user-input max weight is not overwritten by truc max weight
- [x] Test v3 in RPCs other than `createrawtransaction`
ACKs for top commit:
glozow:
reACK 5c8bf7b39e
achow101:
ACK 5c8bf7b39e
rkrux:
ACK 5c8bf7b39e
Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5
60d1042b9a wallet: Remove unused `WalletFeature` enums (woltx)
66de58208a wallet: Remove `CWallet::nWalletVersion` and related functions (woltx)
7cda3d0f5b wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()` (woltx)
ba01585229 wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature` (woltx)
63acee2797 wallet: Remove `GetClosestWalletFeature()` (woltx)
e27da3150b wallet: Remove `GetVersion()` (woltx)
Pull request description:
This PR incorporates the suggestion provided by PRabahy and pablomartin4btc in https://github.com/bitcoin/bitcoin/pull/32944 of removing `CWallet::nWalletVersion` and several related functions, such as `SetMinVersion()`, `GetVersion()`, `GetClosestWalletFeature()`, `IsFeatureSupported()`, `CanSupportFeature()`, etc ...
This field is no longer used in the descriptor wallet and there is still a lot of code related to it, so the changes here provide a good cleanup in the wallet code.
Built on top of https://github.com/bitcoin/bitcoin/pull/32944
ACKs for top commit:
maflcko:
review ACK 60d1042b9a🐾
achow101:
ACK 60d1042b9a
pablomartin4btc:
ACK 60d1042b9a
Tree-SHA512: 1a7ad8e15d57df8f66545776e7d178a2cd5312c87769a29770588375e3de5f24247aab9919acf004ed3eca16d08ba595b5f1c7b2b3eef7752e89d9c295624583
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
aac0b6dd79 test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65 wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79
murchandamus:
ACK aac0b6dd79
glozow:
ACK aac0b6dd79
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
251d020846 init, wallet: replace hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
e3ba0757a9 rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
Pull request description:
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8d) to get rid of the remaining hardcoded output types in wallet RPC and command line arguments documentation [1]. Note that it can't be used in the [`createmultisig` RPC](fc162299f0/src/rpc/output_script.cpp (L100)), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
[1] instances were found via `$ git grep legacy.*p2sh-segwit ./src/rpc/ ./src/wallet/`
ACKs for top commit:
nervana21:
tACK [251d020](251d020846)
maflcko:
review ACK 251d020846 🌨
pablomartin4btc:
re-utACK 251d020846
rkrux:
crACK 251d020846
Tree-SHA512: 23d1025d068f3a44b115a34b217b808fcae59fc574e35a899f0d43a85512935c90675d2e98c621287e02adc3a9f4a08289a26596c66e2122262af0cd2dfbde72
c5c1960f93 doc: Add release notes for changes in RPCs (pablomartin4btc)
90fd5acbe5 rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc)
39fef1d203 test: Add missing logging info for each test (pablomartin4btc)
53ac704efd rpc, test: Fix error message in unloadwallet (pablomartin4btc)
1fc3a8e8e7 rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc)
b635bc0896 rpc, util: Add EnsureUniqueWalletName (pablomartin4btc)
Pull request description:
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all RPC commands calls finding that `getdescriptoractivity` had the same problem, but related to the array input types (blockhashes & descriptors), so I've corrected that RPC as well. For consistency I've added the missing logging info for each test case in `test/functional/rpc_getdescriptoractivity.py` in preparation for the new test.
**_-Before_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -3
error message:
JSON value of type number is not of expected type string
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -3
error message:
JSON value of type null is not of expected type array
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -3
error message:
JSON value of type null is not of expected type array
```
**_-After_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -8
error message:
Either the RPC endpoint wallet or the wallet name parameter must be provided
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
Get spend and receive activity associated with a set of descriptors for a set of blocks. This command pairs well with the `relevant_blocks` output of `scanblocks()`.
This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)
Arguments:
1. blockhashes (json array, required) The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.
[
"blockhash", (string) A valid blockhash
...
]
2. scanobjects (json array, required) Array of scan objects. Every scan object is either a string descriptor or an object:
[
"descriptor", (string) An output descriptor
{ (json object) An object with output descriptor and metadata
"desc": "str", (string, required) An output descriptor
"range": n or [n,n], (numeric or array, optional, default=1000) The range of HD chain indexes to explore (either end or [begin,end])
},
...
]
3. include_mempool (boolean, optional, default=true) Whether to include unconfirmed activity
...
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
...
```
ACKs for top commit:
achow101:
ACK c5c1960f93
stickies-v:
re-ACK c5c1960f93
furszy:
ACK c5c1960f93
Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
This commit takes use of the `FormatAllOutputTypes` helper
(introduced in PR #32432, commit 8cc9845b8d)
to get rid of the hardcoded output types in wallet RPC documentation.
Note that it can't be used in the `createmultisig` RPC, as this one is
only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
6135e0553e wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC (Ava Chow)
Pull request description:
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful information for future PRs.
Split from #32489
ACKs for top commit:
rkrux:
ACK 6135e05
Sjors:
ACK 6135e0553e
w0xlt:
ACK 6135e0553e
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
The unloadwallet RPC previously failed with a low-level JSON parsing error
when called without any arguments (wallet_name).
Although this issue was first identified during review of the original unloadwallet
implementation in #13111, it was never addressed.
Raise RPC_INVALID_PARAMETER instead describing that either the RPC endpoint or wallet
name must be provided.
Adding a new functional test for this use case.
Refactor migratewallet to use the same logic as the wallet_name argument handling
is identical.
Co-authored-by: maflcko <maflcko@users.noreply.github.com>
Add a new function called EnsureUniqueWalletNamet that returns the
selected wallet name across the RPC request endpoint and wallet_name.
Supports the case where the wallet_name argument may be omitted—either
when using a wallet endpoint, or when not provided at all. In the latter
case, if no wallet endpoint is used, an error is raised.
Internally reuses the existing implementation to avoid redundant URL
decoding and logic duplication.
This is a preparatory change for upcoming refactoring of unloadwallet
and migratewallet, which will adopt EnsureUniqueWalletName for improved
clarity and consistency.
fcfd3db563 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795f use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563
achow101:
ACK fcfd3db563
furszy:
ACK fcfd3db563
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
b1a8ac07e9 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e1 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41 wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198 wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435 wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aa wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38 test: Watchonly wallets should estimate larger size (Ava Chow)
Pull request description:
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.
With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.
Split from #32523
Depends on #32594 for tests that are easier to read
ACKs for top commit:
Eunovo:
ACK b1a8ac07e9
maflcko:
re-ACK b1a8ac07e9🌈
rkrux:
ACK b1a8ac07e9
furszy:
light code review ACK b1a8ac07e9
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
8cc9845b8d wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them (w0xlt)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32429, built on top of it.
This PR addresses the https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076251627 that the RPC documentation does not use `OUTPUT_TYPES`, but rather hardcodes them, as is already the case for the `getnewaddress` command.
So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() execution.
It also updates `wallet/rpc/addresses.cpp` to write the RPC docs according to `OUTPUT_TYPES` instead of using hardcoded version.
It also updates the documentation in outputtypes.h, adding Doxygen comments,
ACKs for top commit:
maflcko:
lgtm ACK 8cc9845b8d
achow101:
ACK 8cc9845b8d
Tree-SHA512: e86d813d6d158dd2f6c62519a7ecaa878f2e4f686b5bae82028a106bd6671a13b10fb366f9bb7b94974777217e1852f38e8aa05bba00cd27f94f4412167a3562
c3fe85e2d6 wallet, rpc, test: Remove deprecated getunconfirmedbalance (Ava Chow)
0ec255139b wallet, rpc: Remove deprecated balances from getwalletinfo (Ava Chow)
Pull request description:
`getwalletinfo` result fields `balance`, `immature_balance`, and `unconfirmed_balance`, and the `getunconfirmedbalance` RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by `getbalances`.
ACKs for top commit:
davidgumberg:
ACK c3fe85e2d6
rkrux:
ACK c3fe85e2d6
BrandonOdiwuor:
ACK c3fe85e2d6 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
w0xlt:
reACK c3fe85e2d6
Tree-SHA512: c7c4acfd9cabc7517ba813b95281a6c6a717a417312afd9346298669b4f7bd37724ad977148ce42db7fd47fc3d1f5a8482d8ff2e7b9cb74756b171a5b8b91ef2
47237cd193 wallet, rpc: Output wallet flags in getwalletinfo (Ava Chow)
bc2a26b296 wallet: Add GetWalletFlags (Ava Chow)
69f588a99a wallet: Set upgraded descriptor cache flag for newly created wallets (Ava Chow)
Pull request description:
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
ACKs for top commit:
Sjors:
ACK 47237cd193
w0xlt:
ACK 47237cd193
rkrux:
ACK 47237cd193
Tree-SHA512: 97c7f85b858efe5ced9b8aafb6cd7c1a547de6f8013b82bfc75bc567cf73c9db5e168e3980355756541305520022fd776b8d4d240d3fb34ed86c27d2acaf4863
ce90f0c99f rpc, wallet, refactor: Remove non-descriptor errors (pablomartin4btc)
573bcd75d7 wallet, refactor: Remove unused SetupGeneration (pablomartin4btc)
5431f2dc21 wallet, refactor: Remove Legacy warnings and errors (pablomartin4btc)
Pull request description:
Remove dead code due to legacy wallet support removal.
These changes have no impact on functionality. They are transparent to the end user, as legacy wallets can't be created or loaded anymore, so these checks are no longer reached. The legacy-to-descriptor wallet migration flow is not affected either, as these removals are not part of its process.
ACKs for top commit:
achow101:
ACK ce90f0c99f
rkrux:
utACK ce90f0c99f
Tree-SHA512: 9229ad9dda9ff1dece73b5b15a20d69c6ab1ff2c75b2ec430ddbbaeb3467f6a850f53df527bcb4a8114ccbf1aa9c794462d71a8d516aed6f9a9da74edae16feb
0def84d407 test: Verify parent_desc in RPCs (Ava Chow)
2554cee988 test: Enable default wallet for wallet_descriptor.py (Ava Chow)
3fc9d9f241 wallet, rpc: Push the normalized parent descriptor (Ava Chow)
Pull request description:
Instead of prividing the descriptor string as stored in the db, use the normalized descriptor as is done for getaddressinfo's parent_desc field.
Split from #32489
ACKs for top commit:
Sjors:
re-utACK 0def84d407
rkrux:
ACK 0def84d407
w0xlt:
reACK 0def84d407
Tree-SHA512: 575c5b545d6f0aa7e135696b7a55c004e754fca4dd35dd9cf71b0b45b49a2e86e7b20570e768534d587005953bb893645379ec1ba4f98cfd26811f9c2f17de2d
It is not possible to load a legacy/ non-descriptor wallet anymore
so no need to check for WALLET_FLAG_DESCRIPTORS in RPC calls, even when
passing -rpcwallet/ JSON `/wallet/<walletname>/` endpoint, that searches
for the wallets loaded already in the context.
This RPC lists all the descriptors present in the wallet, not only
the ones that were imported, but also the ones generated when a
new wallet is created.
It can be verified by creating a new wallet and calling the
`listdescriptors` RPC, which will contain 8 ranged descriptors that
are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled"
because this is the only wallet type available now after removal of
legacy wallets.
f98e1aaf34 rpc: Note in fundrawtransaction doc, fee rate is for package (benthecarman)
Pull request description:
Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.
ACKs for top commit:
achow101:
ACK f98e1aaf34
rkrux:
re-ACK f98e1aaf34
danielabrozzoni:
ACK f98e1aaf34
Tree-SHA512: 9f961de1200803ec4d1c6901fd606bb6cf707ffd03942d9dc0d4b6554c827075f99d693b93e892f728679d67e63e12c71da4426dab091b3311d1605bc37251a2
86e1111239 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5bb5 wallet: init, don't error out when loading legacy wallets (furszy)
Pull request description:
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
ACKs for top commit:
achow101:
ACK 86e1111239
w0xlt:
ACK 86e1111239
Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78
If the locked coin needs to be persisted to the wallet database,
insteead of having the RPC figure out when to create a WalletBatch and
having LockCoin's behavior depend on it, have LockCoin take whether to
persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as
well, we need to track whether the locked coin was persisted to the
wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful
information for future PRs.
This is the RPC example counterpart to commit
86de8c1668 (PR #32544).
Since the recent legacy wallet removal this parameter *must* be
true, so providing it in the examples doesn't contain valuable
information anymore and it seems best to remove them.
Now that we only ever operate on descriptor wallets, mentioning
that a faster rescan is only available for them is redundant and
can be removed.
These texts were originally introduced in commit
ca48a4694f (PR #25957).
This `getwalletinfo()` result field was only ever returned for
legacy wallets and is hence not relevant anymore, so we can
delete it and the corresponding CWallet/ScriptPubKeyMan code
behind it.
785e1407b0 wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0
ryanofsky:
Code review ACK 785e1407b0
furszy:
Code review ACK 785e1407b0
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
