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
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.
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
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
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.
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
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
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
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>
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.
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
`fs::path` looks more native than `std::string` for a parameter which
represents a backup file. This change eliminates back-and-forth type
conversions.
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
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
