25dac9fa65 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm)
05227a3554 tests for bumpfee / estimate_modes (Karl-Johan Alm)
3404c1b753 policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm)
6fcf448430 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm)
b188d80c2d MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm)
5d1a411eb1 fees: add FeeModes doc helper function (Karl-Johan Alm)
91f6d2bc8f rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm)
69158b41fc added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm)
Pull request description:
This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators.
ACKs for top commit:
Sjors:
re-utACK 25dac9fa65: rebased, more fancy C++,
jonatack:
ACK 25dac9fa65 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted.
fjahr:
Code review ACK 25dac9fa65
Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33
-BEGIN VERIFY SCRIPT-
# General rename helper: $1 -> $2
rename_global() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1"); }
# Helper to rename TxoutType $1
rename_value() {
sed -i "s/ TX_$1,/ $1,/g" src/script/standard.h; # First strip the prefix in the definition (header)
rename_global TX_$1 "TxoutType::$1"; # Then replace globally
}
# Change the type globally to bring it in line with the style-guide
# (clsses are UpperCamelCase)
rename_global 'enum txnouttype' 'enum class TxoutType'
rename_global 'txnouttype' 'TxoutType'
# Now rename each enum value
rename_value 'NONSTANDARD'
rename_value 'PUBKEY'
rename_value 'PUBKEYHASH'
rename_value 'SCRIPTHASH'
rename_value 'MULTISIG'
rename_value 'NULL_DATA'
rename_value 'WITNESS_V0_KEYHASH'
rename_value 'WITNESS_V0_SCRIPTHASH'
rename_value 'WITNESS_UNKNOWN'
-END VERIFY SCRIPT-
bc01f7ae05 doc: release note for rpc getaddressinfo removals (Jon Atack)
90e989390e rpc: getaddressinfo RPCResult fixup (Jon Atack)
a8507c99da rpc: remove deprecated getaddressinfo `labels: purpose` (Jon Atack)
645a8653c8 rpc: remove deprecated getaddressinfo `label` field (Jon Atack)
Pull request description:
These were deprecated in #17578 and #17585, with expected 0.21 removal notified in the 0.20 release notes.
```
- The `getaddressinfo` RPC has had its `label` field deprecated
(re-enable for this release using the configuration parameter
`-deprecatedrpc=label`). The `labels` field is altered from returning
JSON objects to returning a JSON array of label names (re-enable
previous behavior for this release using the configuration parameter
`-deprecatedrpc=labelspurpose`). Backwards compatibility using the
deprecated configuration parameters is expected to be dropped in the
0.21 release. (#17585, #17578)
```
ACKs for top commit:
Sjors:
utACK bc01f7a
adamjonas:
utACK bc01f7a
meshcollider:
utACK bc01f7ae05
Tree-SHA512: ae1af381e32c4c3bde8b061a56382838513a9a82c88767843cdeae3a2ab8aa7d8c2e66e106d2b31ea07d74bb80c191a2f842c9aaecc7c5438ad9a9bc66d1b251
e5327f947c [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)
79804fe24b [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost)
Pull request description:
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`.
Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`.
This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.
ACKs for top commit:
achow101:
ACK e5327f947c
meshcollider:
utACK e5327f947c
Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
4d7369125a Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbe Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f91 Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc468123 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce6 Prefer explicit uint160 conversion (Ben Woosley)
Pull request description:
This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.
Inspired by and built on #17924. Commits are small and focused to ease review.
Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".
ACKs for top commit:
achow101:
ACK 4d7369125a
meshcollider:
re-utACK 4d7369125a
Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
d0a3feea73 Change docs for walletcreatefundedpsbt RPC method (Ivan Vershigora)
Pull request description:
`sequence` field in the list of inputs currently marked as "required". Actually it can be omitted and it's value depends on `locktime` and `options.replaceable` fields. Just the same as in `createpsbt` call.
ACKs for top commit:
achow101:
ACK d0a3feea73
Tree-SHA512: 3f429a2c2eea283a47fb5002a99f7e2a5ed6f67df9fd895c1ab938256c48a6497ed6ac2673d8fe8968dfb67b939f4a84570899d9faf52f3abd6ec90c0703d1bd
These types are equivalent, in data etc, so they need only their
data cast across.
Note a function is used rather than a casting
operator as CKeyID is defined at a lower level than script/standard
This simplifies control flow and also helps get rid of the ::vpwallets
variable, because EnsureWalletIsAvailable doesn't have access to the request
context.
Replace with RPC request reference to new WalletContext struct similar to the
existing NodeContext struct and reference.
This PR is a followup to 25ad2c623ahttps://github.com/bitcoin/bitcoin/pull/18740 removing the g_rpc_node global.
Some later PRs will follow this up and move more wallet globals to the
WalletContext struct.
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
ca2a09640f Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1140 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8f13 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d95c docs: Add release notes for descriptor wallets (Andrew Chow)
Pull request description:
Some docs and cleanup following #16528.
* Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
* Adds a warning to `createwallet` that descriptor wallets are experimental.
* Removed unused `SetCrypted` as suggestioned: https://github.com/bitcoin/bitcoin/pull/16528#discussion_r415300916
* Removed `m_address_type` as mentioned in https://github.com/bitcoin/bitcoin/pull/18782#issuecomment-620167077
ACKs for top commit:
Sjors:
tACK ca2a09640f
instagibbs:
utACK ca2a09640f
meshcollider:
utACK ca2a09640f
Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
1ed52fbb4d Remove IBD check in sethdseed (Andrew Chow)
b1810a145a Test that keys from inactive seeds are generated (Andrew Chow)
c93082ece4 Generate new keys for inactive seeds after marking used (Andrew Chow)
45f2f6a0e8 Determine inactive HD seeds from key metadata and track them in LegacyScriptPubKeyMan (Andrew Chow)
b59b4504ab have GenerateNewKey and DeriveNewChildKey take a CHDChain as an argument (Andrew Chow)
Pull request description:
Largely implements the suggestion from https://github.com/bitcoin/bitcoin/pull/17484#issuecomment-560845316.
After `sethdseed` is called, the CHDChain for the old seed is kept in the wallet. It is kept on the file as a new `inactivehdseed` record and in memory in a map `m_inactive_hd_seeds`. In `LegacyScriptPubKeyMan::MarkUnusedAddresses` we check each used key's metadata for whether it was derived from an inactive seed. If it is, we then check to see how many keys after that key were derived from the inactive seed. If that number does not match the keypool parameter, we derive more keys from the inactive seed until it does match. This way we won't miss transactions belonging to keys outside of the range of the keypool initially.
The indexes and internal-ness of a key is gotten by checking it's key origin data.
Because of this change, we no longer need to wait for IBD to finish before `sethdseed` can work so that check is also removed.
A test case for this is added as well which fails on master.
ACKs for top commit:
ryanofsky:
Code review ACK 1ed52fbb4d. Changes since last review: various commit message, code comment, log message, error checking improvements, and fix for topping up inactive seeds if wallet isn't reloaded after calling sethdseed and test for this
ariard:
Code Review ACK 1ed52fb
jonatack:
ACK 1ed52fbb4d thanks for addressing the previous review feedback; would be happy to see the new review questions answered and feedback addressed and re-ack.
Tree-SHA512: e658ae0e1dab94be55d2b62cdda506c94815e73a6881533fd30d41cc77477f82fee2095144957a3a1df0c129e256bdd7b7abe3737d515f393610446cae4edf1c
It is no longer necessary to wait for IBD to be complete before setting
a HD seed. This check was originally to ensure that restoring an old
seed on an out of sync node would scan the entire blockchain and thus
not miss transactions that involved keys that were not in the keypool.
This was necessary as once the seed was changed, no further keys would
be derived from the old seed(s).
As we are now topping up inactive seeds as we find those keys to be
used, this check is no longer necessary. During IBD, each time we
find a used key belonging to an inactive hd seed, we will still generate
more keys from that inactive seed.
fa1f840596 rpcwallet: Replace pwallet-> with wallet. (MarcoFalke)
fa182a8794 rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} (MarcoFalke)
Pull request description:
Closes#18943
ACKs for top commit:
laanwj:
ACK fa1f840596
ryanofsky:
Code review ACK fa1f840596 and thanks for using a standalone commit for the fix
promag:
Code review ACK fa1f840596.
hebasto:
ACK fa1f840596, tested on Linux Mint 19.3.
Tree-SHA512: 0838485d1f93f737ce5bf12740669dcafeebb78dbc3fa15dbcc511edce64bf024f60f0497a04149a1e799d893d57b0c9ffe442020c1b9cfc3c69db731f50e712
9f59dde974 rpc: Relock wallet only if most recent callback (João Barbosa)
a2e6db5c4f rpc: Add mutex to guard deadlineTimers (João Barbosa)
Pull request description:
This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time
Issue introduced in #18487.
Fixes#18811.
ACKs for top commit:
MarcoFalke:
ACK 9f59dde974
ryanofsky:
Code review ACK 9f59dde974. No changes since last review except squashing commits.
jonatack:
ACK 9f59dde974
Tree-SHA512: 2f7fc03e5ab6037337f2d82dfad432495cc337c77d07c968ee2355105db6292f24543c03456f5402e0e759577a4327758f9372f7ea29de6d56dc3695fda9b379
28b112e9bd Get rid of BindWallet (Russell Yanofsky)
d002f9d15d Disable CWalletTx copy constructor (Russell Yanofsky)
65b9d8f8dd Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky)
bd2fbc7cdb Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky)
2b9cba2065 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky)
Pull request description:
This is a pure refactoring, no behavior is changing.
Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.
This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.
This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.
Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.
This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.
ACKs for top commit:
MarcoFalke:
Anyway, re-ACK 28b112e9bd
Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef
Also, mark feebumper bilingual_str as Untranslated
They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
Disable copying of CWalletTx objects to prevent bugs where instances get copied
in and out of the mapWallet map and fields are updated in the wrong copy.
This change is intended to make the bitcoin node and its rpc, network
and gui interfaces more responsive while the wallet is in use. Currently
because the node's cs_main mutex is always locked before the wallet's
cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked
while the wallet does relatively slow things like creating and listing
transactions.
This commit only remmove chain lock tacking in wallet code, and invert
lock order from cs_main, cs_wallet to cs_wallet, cs_main.
must happen at once to avoid any deadlock. Previous commit were only
removing Chain::Lock methods to Chain interface and enforcing they
take cs_main.
Remove LockChain method from CWallet and Chain::Lock interface.
a1d5b12ec0 Merge getreceivedby tally into GetReceived function (Andrew Toth)
Pull request description:
This PR merges the tally code of `getreceivedbyaddress` and `getreceivedbylabel` into a single function `GetReceived`. This reduces repeated code and makes it similar to `listreceivedbyaddress` and `listreceivedbylabel`, which use the function `ListReceived`. It will also make the change in #14707 simpler and easier to review.
ACKs for top commit:
theStack:
re-ACK a1d5b12ec0
meshcollider:
utACK a1d5b12ec0
Tree-SHA512: 43d9cd92f7c2c6a8b9c7509aa85a9b9233a6cfec1c43a9062e3bdfb83515413d1feafa8938c828351278ba22bd31c47e62ab5341e4bddc2493103b094d73b047
0d32d66148 Remove -upgradewallet startup option (Andrew Chow)
92263cce5b Add upgradewallet RPC (Andrew Chow)
1e48796c99 Make UpgradeWallet a member function of CWallet (Andrew Chow)
c988f27937 Have UpgradeWallet take the version to upgrade to and an error message out parameter (Andrew Chow)
1833237123 Only run UpgradeWallet if the wallet needs to be upgraded (Andrew Chow)
9c16b1735f Move wallet upgrading to its own function (Andrew Chow)
Pull request description:
`-upgradewallet` is largely incompatible with many recent wallet features and versions. For example, it was disabled if multiple wallets were used and would not work with encrypted wallets that were being upgraded to HD.
This PR does away with the old method of upgrading upon startup and instead allows users to upgrade their wallets via an `upgradewallet` RPC. This does largely the same thing as the old `-upgradewallet` option but because the wallet is loaded, it can be unlocked to upgrade to HD. Furthermore it is compatible with multiwallet as it works on the individual wallet that is specified by the RPC.
ACKs for top commit:
meshcollider:
Code review ACK 0d32d66148
darosior:
ACK 0d32d66148
MarcoFalke:
ACK 0d32d66148🚵
Tree-SHA512: b425bf6f5d605e26506889d63c780895482f07cbc086193218e031e8504d3072d41e90d65cd41bcc98ee4c1eb048954bc5d4ac85435f7394892373aac89a3b0a
fa168d7542 rpc: Document all aliases for first arg of listtransactions (MarcoFalke)
fa5b1f067f rpc: Document all aliases for second arg of getblock (MarcoFalke)
fa86a4bbfc rpc: Rename first arg of generateblock RPC to "output" (MarcoFalke)
Pull request description:
This fixes a bug found with #18531:
* Currently the named argument for `generateblock` is documented as `address/descriptor`, but the server only accepts a named argument of `address`. Fix it by changing the name to `output` for both the documentation and the server code. Also, add tests to prove the server understands the new name `output`.
* Unrelated to that, there have been a bunch of aliases in the server code that are not present in the source code of the documentation. Fix that by adding the alias to the source code of the documentation. Only the first alias is displayed in the rendered documentation. Also, add tests to prove the server actually understands all aliases.
ACKs for top commit:
pierreN:
Tested ACK fa168d7 tests, help messages
Tree-SHA512: 05e15628e3a667b296f3783d20f764b450b959451b5360c7eaf5993156582d47a0f5882330ca2493b851eb46324d504953b90c875bc88a15c9e8c89eb3ef8d92
38677274f9 rpc: settxfee respects -maxtxfee wallet setting (Fabian Jahr)
bda84a08a0 rpc: Add documentation for deactivating settxfee (Fabian Jahr)
Pull request description:
~~Closes 18315~~
`settxfee` can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that `settxfee` is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate.
Examples:
```
$ src/bitcoin-cli settxfee 0.0000000
{
"active": false,
"fee_rate": "0.00000000 BTC/kB"
}
$ src/bitcoin-cli settxfee 0.0001
{
"active": true,
"fee_rate": "0.00010000 BTC/kB"
}
```
ACKs for top commit:
MarcoFalke:
ACK 38677274f9, seems useful to error out early instead of later #16257🕍
jonatack:
ACK 38677274f9
meshcollider:
LGTM, utACK 38677274f9
Tree-SHA512: 642813b5cf6612abb4b6cb63728081a6bd1659d809e0149c8f56060b6da7253fee989b3b202854f3051df3773c966799af30b612648c466b099f00590f356548
48973402d8 wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9ab5 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc4cb wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff280c wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac594 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881bc6 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9bfc6 wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf9e5 wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce6bb wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87971 wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44ccce wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4922 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)
Pull request description:
This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.
ACKs for top commit:
MarcoFalke:
re-ACK 48973402d8, only change is fixing bug 📀
fjahr:
re-ACK 48973402d8, reviewed rebase and changes since last review, built and ran tests locally
ariard:
Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`
Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b
c0af173da2 doc: default minconf for getbalance should be 0 (U-Zyn Chua)
Pull request description:
- Default `minconf` for `getbalance` is `0` but example in doc was showing as `1`.
- `at least 6 blocks confirmed` now updated to be `at least 6 confirmations` to be more consistent with the terminology used elsewhere in the codebase and documentations.
ACKs for top commit:
theStack:
re-ACK c0af173da2
Tree-SHA512: 8f67af78a222a4bd2957658b37fae2224783274f355af84f39a5ce0da90b21f03dc798a6408d44a724c353ff5ed7dfec943fb28726ec423028b64fc579f937ad
01a3392b1b Drop bitcoin-wallet dependency on libevent (Russell Yanofsky)
0660119ac3 Drop unintended bitcoin-tx dependency on libevent (Russell Yanofsky)
Pull request description:
This fixes compile errors trying to build bitcoin-tx and bitcoin-wallet without libevent, which were reported by Luke Dashjr in https://github.com/bitcoin/bitcoin/issues/18465
The fix avoiding `bitcoin-tx` dependency on libevent just adds a conditional build rule. This is implemented in the first commit (more details in commit description).
The fix avoiding `bitcoin-wallet` dependency on libevent requires minor code changes, because `bitcoin-wallet` (unlike `bitcoin-tx`) links against code that calls `urlDecode` / `evhttp_uridecode`. This fix is implemented in the second commit (again details in the commit description).
ACKs for top commit:
jonasschnelli:
utACK 01a3392b1b.
Tree-SHA512: d2245e912ab494cccceeb427a1eca8e55b01a0006ff93eebcfb5461ae7cecd1083ac2de443d9db036b18bdc6f0fb615546caaa20c585046f66d234937f74870a
fa1a92224d rpc: Avoid initialization-order-fiasco on static CRPCCommand tables (MarcoFalke)
Pull request description:
Currently the fiasco is only theoretical because all content of the table are compile-time constants. However, the fiasco materializes should they ever become run-time constants (e.g. #18531).
ACKs for top commit:
promag:
ACK fa1a92224d.
practicalswift:
ACK fa1a92224d -- fiasco bad :)
Tree-SHA512: cccadb0ad56194599b74f04264d74c34fa865958580a850efc6474bbdc56f30cadce6b2e9a6ad5472ff46c3f4c793366acd8090fad409a45b25d961f2d89da19
b5795a7886 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f57a Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d886f4 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c84b Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc2b1 Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f85da Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd155f6 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)
Pull request description:
In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.
This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.
Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).
Fixing it is accomplished by:
* Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
* `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
* For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).
A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.
ACKs for top commit:
ryanofsky:
Code review ACK b5795a7886. Pretty clever and nicely implemented fix!
jonatack:
ACK b5795a7886 nice improvements -- code review, built/ran tests rebased on current master ff53433fe4 and tested manually with rpc/cli
jnewbery:
Good fix. utACK b5795a788.
Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
7b8e15728d rpc: Fix rpcRunLater race in walletpassphrase (João Barbosa)
Pull request description:
Release locks before calling `rpcRunLater`.
Quick explanation: `rpcRunLater` leads to `event_free` which calls `event_del` which can wait for the event callback to finish if it's already running and that callback will try to lock wallet mutex - which is already locked in http thread.
Fixes#14995 , fixes#18482. Best reviewed with whitespace changes hidden.
ACKs for top commit:
MarcoFalke:
ACK 7b8e15728d, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞
ryanofsky:
Code review ACK 7b8e15728d. Just updated comment since last review
Tree-SHA512: 17874a2fa7b0e164fb0d7ee4cb7d59650275b8c03476fb291d60af8b758495457660d3912623fb26259fefe84aeba21c0a9e0c6467982ba511f19344ed5413ab
Previous versions assumed absence of an entry in mapAddressBook indicated change.
This no longer holds true (due to bugs) and will shortly be made intentional.
Renaming the field helps ensure that old code using mapAddressBook directly gets checked for necessary rebasing.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/mapAddressBook/m_address_book/g' $(git grep -l 'mapAddressBook' ./src)
-END VERIFY SCRIPT-
Don't require urlDecode function in wallet code since urlDecode implementation
currently uses libevent. Just call urlDecode indirectly though URL_DECODE
function pointer constant if available.
In bitcoind and bitcoin-qt, URL_DECODE is implemented and used to interpret RPC
wallet requests. In bitcoin-wallet, URL_DECODE is null to avoid depending on
libevent.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change affects behavior in a few small ways.
- If there's no max_height specified, percentage progress is measured ending at
wallet last processed block instead of node tip
- More consistent error reporting: Early check to see if start_block is on the
active chain is removed, so start_block is always read and the triggers an
error if it's unavailable
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
https://github.com/bitcoin/bitcoin/issues/14338#issuecomment-426706574
FoundBlock class allows interfaces::Chain::findBlock to return more block
information without having lots of optional output parameters. FoundBlock class
is also used by other chain methods in upcoming commits.
There is mostly no change in behavior. Only exception is
CWallet::RescanFromTime now throwing NonFatalCheckError instead of
std::logic_error.
c3857c5fcb wallet: remove CreateTotalBumpTransaction() (Jon Atack)
4a0b27bb01 wallet: remove totalfee from createBumpTransaction() (Jon Atack)
e347cfa9a7 rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack)
bd05f96d79 test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack)
a6d1ab8caa test: update bumpfee testing from totalFee to fee_rate (Jon Atack)
Pull request description:
Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it.
ACKs for top commit:
laanwj:
ACK c3857c5fcb
Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00
fac52253f8 rpc: Document an RPCResult for all calls; Enforce at compile time (MarcoFalke)
fadd99f610 rpc: Add missing newline in RPCResult description (MarcoFalke)
Pull request description:
This documents the RPC Result (type and description, if applicable) everywhere it was missing. The patch can be reviewed with the `git diff` option `-W`/`--function-context`.
Also, code won't compile without having an RPCResult documented.
ACKs for top commit:
laanwj:
Lightly tested ACK fac52253f8
promag:
Tested ACK fac52253f8, built and verified listunspent help output.
Tree-SHA512: af2c1af1432beb944993776026c320814bfaecaf202f47359f5758849096ca7051ec6560395a2cc6678dcc111e7c9cf4917d0f0b221bdcf3ed1642e14d0e5b3c
This replaces one remaining instance of the literal "BTC" string with
the CURRENCY_UNIT constant, as is done in most of the codebase already.
The other remaining instance (which is just part of a log message and thus
not really user-visible) is just removed.
After this change, no instance of literal "BTC" remains anywhere in the
non-Qt and non-test codebase.
When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.
8a2a652e6f Remove redundant type information from rpc docs (David O'Callaghan)
Pull request description:
Simple edit of the RPC calls to remove redundant text ("A json object/array ...") from the beginning of help.
Fixes: #18258
Top commit has no ACKs.
Tree-SHA512: cbbf760e0b7b4eda61c40b420ed77f5d878318e37b0eb13e63567212240b2c4ecc15d84030e98075e21c9ae9016539adfd201e5661ea824166a76d335180c32f
3e32499909 Change example addresses to bech32 (Yusuf Sahin HAMZA)
Pull request description:
This is a follow-up PR to #18197 that fixes RPCExamples.
Fixes#18185.
ACKs for top commit:
MarcoFalke:
ACK 3e32499909
jonatack:
ACK 3e32499
Tree-SHA512: c7a6410ef8b6e169016c2c5eac3e6b9501caabd0e8a0871ec31e56bfc44589f056d3f5cb55b5a13bba36f6c15136c2352f883e30e4dcc0997ffd36b27f9173b9
d2774c09cf Clear any input_errors for an input after it is signed (Andrew Chow)
dc174881ad Replace GetSigningProvider with GetSolvingProvider (Andrew Chow)
6a9c429084 Move direct calls to MessageSign into new SignMessage functions in CWallet and ScriptPubKeyMan (Andrew Chow)
82a30fade7 Move key and script filling and signing from CWallet::FillPSBT to ScriptPubKeyMan::FillPSBT (Andrew Chow)
3d70dd99f9 Move FillPSBT to be a member of CWallet (Andrew Chow)
a4af324d15 Use CWallet::SignTransaction in CreateTransaction and signrawtransactionwithwallet (Andrew Chow)
f37de92744 Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransaction (Andrew Chow)
d999dd588c Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKeyMan (Andrew Chow)
2c52b59d0a Refactor rawtransaction's SignTransaction into generic SignTransaction function (Andrew Chow)
Pull request description:
Following #17261, the way to sign transactions, PSBTs, and messages was to use `GetSigningProvider()` and get a `SigningProvider` containing the private keys. However this may not be feasible for future `ScriptPubKeyMan`s, such as for hardware wallets. Instead of exporting a `SigningProvider` containing private keys, we need to pass these things into the `ScriptPubKeyMan` (via `CWallet`) so that they can do whatever is needed internally to sign them. This is largely a refactor as the logic of processing transactions, PSBTs, and messages for is moved into `LegacyScriptPubKeyMan` and `CWallet` instead of being handled by the caller (e.g. `signrawtransaction`).
To help with this, I've refactored the 3(!) implementations of a `SignTransaction()` function into one generic one. This function will be called by `signrawtransactionwithkey` and `LegacyScriptPubKeyMan::SignTransaction()`. `CWallet::CreateTransaction()` is changed to call `CWallet::SignTransaction()` which in turn, calls `LegacyScriptPubKeyMan::SignTransaction()`. Other `ScriptPubKeyMan`s may implement `SignTransaction()` differently.
`FillPSBT()` is moved to be a member function of `CWallet` and the `psbtwallet.cpp/h` files removed. It is further split so that `CWallet` handles filling the UTXOs while the `ScriptPubKeyMan` handles adding keys, derivation paths, scripts, and signatures. In the end `LegacyScriptPubKeyMan::FillPSBT` still calls `SignPSBTInput`, but the `SigningProvider` is internal to `LegacyScriptPubKeyMan`. Other `ScriptPubKeyMan`s may do something different.
A new `SignMessage()` function is added to both `CWallet` and `ScriptPubKeyMan`. Instead of having the caller (i.e. `signmessage` or the sign message dialog) get the private key, hash the message, and sign, `ScriptPubKeyMan` will now handle that (`CWallet` passes through to the `ScriptPubKeyMan`s as it does for many functions). This signing code is thus consolidated into `LegacyScriptPubKeyMan::SignMessage()`, though other `ScriptPubKeyMan`s may implement it differently. Additionally, a `SigningError` enum is introduced for the different errors that we expect to see from `SignMessage()`.
Lastly, `GetSigningProvider()` is renamed to `GetPublicSigningProvider()`. It will now only provide pubkeys, key origins, and scripts. `LegacySigningProvider` has it's `GetKey` and `HaveKey` functions changed to only return false. Future implementations should return `HidingSigningProvider`s where private keys are hidden.
Other things like `dumpprivkey` and `dumpwallet` are not changed because they directly need and access the `LegacyScriptPubKeyMan` so are not relevant to future changes.
ACKs for top commit:
instagibbs:
reACK d2774c09cf
Sjors:
re-utACK d2774c09cf
meshcollider:
re-utACK d2774c09cf
Tree-SHA512: 89c83e7e7e9315e283fae145a2264648a9d7f7ace8f3281cb3f44f0b013c988d67ba4fa9726e50c643c0ed921bdd269adaec984840d11acf4a681f3e8a582cc1
Not all ScriptPubKeyMans will be able to provide private keys,
but pubkeys and scripts should be. So only provide public-only
SigningProviders, i.e. ones that can help with Solving.
a652ba6293 rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure (Karl-Johan Alm)
Pull request description:
Initialize the `nFeeRequired` variable to avoid using an uninitialized value for errors happening before it is set to 0.
Note: this originally fixed `nFeeRet` in `wallet.cpp`.
ACKs for top commit:
promag:
ACK a652ba6293.
Sjors:
utACK a652ba6293
practicalswift:
ACK a652ba6293 -- patch looks correct
meshcollider:
utACK a652ba6293
Tree-SHA512: 0d12f1ffd0851ed5ce6d109d2c87f55e8b1d57da297e684feeabb57229200c4078f029c55ca5aa5712bd18e26dda3ce538443dfe68a7a6d504428068f81fded0
79facb11e9 wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm)
d9b0ebc1da wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm)
57c569e4d9 wallet: make BackupWallet() const (Karl-Johan Alm)
df3a818d2a wallet: make getters const (Karl-Johan Alm)
227b9dd2d6 wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm)
22d329ad0e wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm)
7b3587b29d wallet/db: make IsDummy() const (Karl-Johan Alm)
d366795d18 wallet/db: make Backup() const (Karl-Johan Alm)
8cd0b86340 wallet: make CanGetAddresses() const (Karl-Johan Alm)
037fa770eb wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm)
ddc93557ad wallet: make CanGenerateKeys() const (Karl-Johan Alm)
dc2d0650fd make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm)
Pull request description:
A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is
1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
2. confusing, as it tends to suggest that this is a proper way to refer to a constant `CWallet` instance.
This PR changes references to wallets to `const CWallet* const` whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.
Note from irc:
> <sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
> <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
> <sipa> CWallet objects aren't copied or moved though
ACKs for top commit:
laanwj:
ACK 79facb11e9
Empact:
ACK 79facb11e9
promag:
ACK 79facb11e9.
fjahr:
ACK 79facb11e9
Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8
* GetAvoidReuseFlag: simply gets the flag, without modifying the wallet
* ListReceived: helper function to produce lists
* ListTransactions: produces a list of transactions, without modifications; two cases of map [] -> .at() for verified-existing keys
* DescribeWalletAddress: generates a description of a given wallet address without changing the wallet
* The following functions produce a list without making any modifications to the wallet:
* listaddressgroupings
* listreceivedbyaddress
* listreceivedbylabel
* listtransactions
* listsinceblock
* listlockunspent
* listunspent
* listlabels
* getreceivedbyaddress
* getreceivedbylabel
* getaddressesbylabel
* signmessage: uses the wallet to procure a private key for signing, but does no modifications
* getbalance, getunconfirmedbalance: calculates the wallet balance, without any modifications
* gettransaction: procures transaction without any modifications
* backupwallet: makes a backup of the wallet to disk, without changing said wallet
* getwalletinfo: produces info about wallet without any modifications
* signrawtransactionwithwallet: modifies incoming transaction on the fly by signing with private key procured from within wallet; no modifications to wallet
* getaddressinfo: gets information about the given address, with no modifications done to the wallet; one case of [] -> .at() and one ::iterator -> ::const_iterator
* walletprocesspsbt: processes the given PSBT on the fly, without modifying the wallet
