The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.
Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
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.
Instead of fetching a SigningProvider from ScriptPubKeyMan in order
to fill and sign the keys and scripts for a PSBT, just pass that
PSBT to a new FillPSBT function that does all that for us.
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
fae86c38bc util: Remove unused MilliSleep (MarcoFalke)
fa9af06d91 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke)
fa4620be78 util: Add UnintrruptibleSleep (MarcoFalke)
Pull request description:
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread`
ACKs for top commit:
ajtowns:
ACK fae86c38bc quick code review
practicalswift:
ACK fae86c38bc -- patch looks correct
sipa:
Concept and code review ACK fae86c38bc
fanquake:
ACK fae86c38bc - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later.
Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
fa6b061fc1 rpc: Auto-format RPCResult (MarcoFalke)
fa7d0503d3 rpc: Move OuterType enum to header (MarcoFalke)
Pull request description:
This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)
Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
ACKs for top commit:
Sjors:
Indeed, re-ACK fa6b061fc1
ajtowns:
ACK fa6b061fc1 -- skimmed code changes and differences to rpc help output
Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28
* 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
* GetWalletAddressesForKey is, as the name implies, immutable; the one change besides the parameter constness is a [] -> .at() change, to a verified-existing key.
* dumpprivkey and dumpwallet are both similarly immutable, for obvious reasons.
CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.
10efc0487c Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef28 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c🐱
ajtowns:
ACK 10efc0487c -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
c72a11a1a0 test: Add cost_of_change parameter assertions to bnb_search_test (Yancy Ribbens)
Pull request description:
If the `cost_of_change` variable is removed from the method body `SelectCoinsBnB`, there are currently no failing unit tests. This PR adds assertions about the behavior of the `cost_of_change`: If the cost of creating a change output is greater than what's leftover, then consume the output and create no change, otherwise, don't consume the output (no match found).
ACKs for top commit:
achow101:
ACK c72a11a1a0
Tree-SHA512: 613aa411df5e2911446e0e8bf3309336faaadf2d3c56e7d125b76454e7c6f9e4f5e8f0910dc6222282628e38cd8a4a7c56bb3d36b564a17f396b9b503ecc64c8
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
e193a84fb2 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1 Deduplicate the message verifying code (Vasil Dimov)
Pull request description:
The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced `MessageSign()` and
`MessageVerify()` and add unit tests for them.
ACKs for top commit:
Sjors:
re-ACK e193a84fb2
achow101:
ACK e193a84fb2
instagibbs:
utACK e193a84fb2
meshcollider:
utACK e193a84fb2
Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
7f1475c711 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)
Pull request description:
Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.
ACKs for top commit:
fanquake:
ACK 7f1475c711
MarcoFalke:
ACK 7f1475c711
Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
fad027fb0c scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)
Pull request description:
This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)
ACKs for top commit:
Sjors:
ACK fad027fb0c
Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
The logic of signing a message was duplicated in 3 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
src/rpc/misc.cpp
signmessagewithprivkey()
src/wallet/rpcwallet.cpp
signmessage()
Move the logic into
src/util/message.cpp
MessageSign()
and call it from all the 3 places.
The logic of verifying a message was duplicated in 2 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
src/rpc/misc.cpp
verifymessage()
with the only difference being the result handling. Move the logic into
a dedicated
src/util/message.cpp
MessageVerify()
which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
25bc17fceb refactor: rpc: Remove vector copy from listtransactions (João Barbosa)
Pull request description:
Current approach
- copy accumulated `ret` vector to `arrTmp`
- drop unnecessary elements from `arrTmp`
- reverse `arrTmp`
- clear `ret`
- copy `arrTmp` to the `ret`
New approach
- create a vector from the accumulated `ret` with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
ACKs for top commit:
ryanofsky:
Code review ACK 25bc17fceb. Just comment and commit message tweaks since last review
Tree-SHA512: 87906561e3accdbdb0f4a8194cbcd76ea53ae53d0ce135b90bc54a5f77e300b14ef08505e7daf1fe52426f135442a743da5a027416a769bd454922357cebe7c0
also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
(mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
address suitable for RPCExamples help documentation)
Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts
when the redeem script is present in the mapScripts map without the p2sh script
also having to be added to the mapScripts map. This restores behavior prior to
https://github.com/bitcoin/bitcoin/pull/17261, which I think broke backwards
compatibility with old wallet files by no longer treating addresses created by
`addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261
is because of a workaround added in 4a7e43e846
"Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem
for new `addmultisigaddress` RPC calls without fixing it for multisig addresses
already created in old wallet files.
This change adds a lot of comments and allows reverting commit
4a7e43e846 "Store p2sh scripts in
AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function,
CanProvide() method, and mapScripts map should all be more comprehensible
19a354b11f Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f
MarcoFalke:
ACK 19a354b11f
promag:
Code review ACK 19a354b11f.
meshcollider:
utACK 19a354b11f
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
e9434ee03e Remove false positive GCC warning (Hennadii Stepanov)
Pull request description:
On master (f05c1ac444) GCC compiler fires a false positive `-Wmaybe-uninitialized`:
```
wallet/wallet.cpp: In static member function ‘static std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::vector<std::__cxx11::basic_string<char> >&, uint64_t)’:
wallet/wallet.cpp:3913:27: warning: ‘*((void*)& time_first_key +8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Optional<int64_t> time_first_key;
^~~~~~~~~~~~~~
```
The same as #15292.
This PR leverages a workaround and removes the warning.
ACKs for top commit:
laanwj:
ACK e9434ee03e, removes the warning for me (gcc 7.4.0)
kristapsk:
ACK e9434ee03e
Tree-SHA512: 8820a8ba6a75aa6b1ac675a38c883a77f12968b010533b6383180aa66e7e0d570bf6300744903ead91cf9084e5345144959cd6b0cea1b763190b8dd49bacce75
d3bc184081 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f364 test: getaddressinfo label deprecation test (Jon Atack)
d48875fa20 rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabeda4 test: remove getaddressinfo label tests (Jon Atack)
c7654af6f8 doc: address pr17578 review feedback (Jon Atack)
Pull request description:
This PR builds on #17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.
See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001 for more context.
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.
Next step: add support for multiple labels.
ACKs for top commit:
jnewbery:
ACK d3bc184081
laanwj:
ACK d3bc184081
meshcollider:
utACK d3bc184081
Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
3f373659d7 Refactor: Replace SigningProvider pointers with unique_ptrs (Andrew Chow)
3afe53c403 Cleanup: Drop unused GUI learnRelatedScripts method (Andrew Chow)
e2f02aa59e Refactor: Copy CWallet signals and print function to LegacyScriptPubKeyMan (Andrew Chow)
c729afd0a3 Box the wallet: Add multiple keyman maps and loops (Andrew Chow)
4977c30d59 refactor: define a UINT256_ONE global constant (Andrew Chow)
415afcccd3 HD Split: Avoid redundant upgrades (Andrew Chow)
01b4511206 Make UpgradeKeyMetadata work only on LegacyScriptPubKeyMan (Andrew Chow)
4a7e43e846 Store p2sh scripts in AddAndGetDestinationForScript (Andrew Chow)
501acb5538 Always try to sign for all pubkeys in multisig (Andrew Chow)
81610eddbc List output types in an array in order to be iterated over (Andrew Chow)
eb81fc3ee5 Refactor: Allow LegacyScriptPubKeyMan to be null (Andrew Chow)
fadc08ad94 Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman (Andrew Chow)
f5be479694 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Continuation of wallet boxes project.
Actually makes ScriptPubKeyMan an interface which LegacyScriptPubkeyMan. Moves around functions and things from CWallet into LegacyScriptPubKeyMan so that they are actually separate things without circular dependencies.
***
Introducing the `ScriptPubKeyMan` (short for ScriptPubKeyManager) for managing scriptPubKeys and their associated scripts and keys. This functionality is moved over from `CWallet`. Instead, `CWallet` will have a pointer to a `ScriptPubKeyMan` for every possible address type, internal and external. It will fetch the correct `ScriptPubKeyMan` as necessary. When fetching new addresses, it chooses the `ScriptPubKeyMan` based on address type and whether it is change. For signing, it takes the script and asks each `ScriptPubKeyMan` for whether that `ScriptPubKeyMan` considers that script `IsMine`, whether it has that script, or whether it is able to produce a signature for it. If so, the `ScriptPubKeyMan` will provide a `SigningProvider` to the caller which will use that in order to sign.
There is currently one `ScriptPubKeyMan` - the `LegacyScriptPubKeyMan`. Each `CWallet` will have only one `LegacyScriptPubKeyMan` with the pointers for all of the address types and change pointing to this `LegacyScriptPubKeyMan`. It is created when the wallet is loaded and all keys and metadata are loaded into it instead of `CWallet`. The `LegacyScriptPubKeyMan` is primarily made up of all of the key and script management that used to be in `CWallet`. For convenience, `CWallet` has a `GetLegacyScriptPubKeyMan` which will return the `LegacyScriptPubKeyMan` or a `nullptr` if it does not have one (not yet implemented, but callers will check for the `nullptr`). For purposes of signing, `LegacyScriptPubKeyMan`'s `GetSigningProvider` will return itself rather than a separate `SigningProvider`. This will be different for future `ScriptPubKeyMan`s.
The `LegacyScriptPubKeyMan` will also handle the importing and exporting of keys and scripts instead of `CWallet`. As such, a number of RPCs have been limited to work only if a `LegacyScriptPubKeyMan` can be retrieved from the wallet. These RPCs are `sethdseed`, `addmultisigaddress`, `importaddress`, `importprivkey`, `importpubkey`, `importmulti`, `dumpprivkey`, and `dumpwallet`. Other RPCs which relied on the wallet for scripts and keys have been modified in order to take the `SigningProvider` retrieved from the `ScriptPubKeyMan` for a given script.
Overall, these changes should not effect how everything actually works and the user should experience no difference between having this change and not having it. As such, no functional tests were changed, and the only unit tests changed were those that were directly accessing `CWallet` functions that have been removed.
This PR is the last step in the [Wallet Structure Changes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes).
ACKs for top commit:
instagibbs:
re-utACK 3f373659d7
Sjors:
re-utACK 3f373659d7 (it still compiles on macOS after https://github.com/bitcoin/bitcoin/pull/17261#discussion_r370377070)
meshcollider:
Tested re-ACK 3f373659d7
Tree-SHA512: f8e2b8d9efa750b617691e8702d217ec4c33569ec2554a060141d9eb9b9a3a5323e4216938e2485c44625d7a6e0925d40dea1362b3af9857cf08860c2f344716
f41d589669 Document better -keypool as a look-ahead safety mechanism (Antoine Riard)
Pull request description:
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
While reviewing #17681, it took me a while to figure out the safety implications of keypool, I find it would be better to document this a bit farther to avoid users shooting themselves in the foot. For further context & discussion, see https://github.com/bitcoin/bitcoin/pull/17681#issuecomment-563613452
ACKs for top commit:
ryanofsky:
Code review ACK f41d589669. Just "Warning:" prefix added since the last review
jonatack:
ACK f41d589669 code review and build/test. The added `Warning:` since last review is a good addition.
Tree-SHA512: d3d0ee88fcdfc5c8841a2bd4bada0e4eeb412a0dce5054e5fb023643c2fa57206a0f3efb06890c245528dc4431413ed2fd5645b9319d26245d044c490b7f0db0
deaa6dd144 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa4 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)
Pull request description:
Fixes#17149
Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.
When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.
When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.
ACKs for top commit:
practicalswift:
ACK deaa6dd144 -- only change since last ACK was the addition of tests
gwillen:
tested ACK deaa6dd, would also like to see this merged!
Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
Needed for future ScriptPubKeyMans which may need to create
SigningProviders dynamically and thus a normal pointer is not enough
This commit does not change behavior.
Add wallet logic for dealing with multiple ScriptPubKeyMan instances. This
doesn't change current behavior because there is still only a single
LegacyScriptPubKeyMan. But in the future the new logic will be used to support
descriptor wallets.
297e098557 Fix doxygen errors (Ben Woosley)
Pull request description:
These are all the remaining errors identified via -Werror=documentation, e.g.:
```
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
```
You can use this to run with `-Wdocumentation` yourself: #14920
ACKs for top commit:
laanwj:
ACK 297e098557
Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
2b1641492f wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.
ACKs for top commit:
meshcollider:
re-utACK 2b1641492f
Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e49
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e49
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
6fc554f591 wallet: Reset reused transactions cache (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17824)
`getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](35fff5be60/src/wallet/wallet.cpp (L1826)). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in #17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.
With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.
ACKs for top commit:
kallewoof:
Code review re-ACK 6fc554f591
promag:
ACK 6fc554f591.
achow101:
Re-ACK 6fc554f591
meshcollider:
Code review ACK 6fc554f591
Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
If a destination is reused we mark the cache of the other transactions going to that destination dirty so they are not accidentally reported as trusted when the cache is hit.
6d6a7a8403 gui: Fix duplicate wallet showing up (João Barbosa)
81ea66c30e Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky)
Pull request description:
This PR includes 2 fixes:
- prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;
- prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`.
Fixes#16937
ACKs for top commit:
fjahr:
code review ACK 6d6a7a8403
ryanofsky:
Code review ACK 6d6a7a8403. No changes since last ACK other than rebase due to #17070
kallewoof:
Code review ACK 6d6a7a8403
Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4
e1e1442f3e Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)
Pull request description:
Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944
This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.
ACKs for top commit:
achow101:
ACK e1e1442f3e
Sjors:
Code review ACK e1e1442f3e
meshcollider:
Code review ACK e1e1442f3e
Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
8925df86c4 doc: update release notes (Jon Atack)
8bb405bbad test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f1 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4.
meshcollider:
Code review ACK 8925df86c4
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
091a876664 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders)
e9b4f9419c bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders)
75a5e478b6 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders)
Pull request description:
The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds.
ACKs for top commit:
achow101:
ACK 091a876664
Sjors:
Tested ACK 091a876664
meshcollider:
utACK 091a876664
Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e
09502452bb IsUsedDestination should count any known single-key address (Gregory Sanders)
Pull request description:
This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case.
ACKs for top commit:
meshcollider:
Code Review ACK 09502452bb
Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c
- change the value returned in the RPC getaddressinfo `labels` field to an array
of label name strings
- deprecate the previous behavior of returning a JSON hash structure containing
label `name` and address `purpose` key/value pairs
- update the relevant tests
If after a backup, an address is issued beyond the initial
keypool range and none of the addresses in this range
is seen onchain, if a wallet is restored from backup, even in
case of rescan, funds may be loss due to the look-ahead
buffer not being incremented and so restored wallet not detecting
onchain out-of-range address as derived from its seed.
This scenario is theoretically unavoidable due to the requirement
of the keypool to have a max size. However, given the default
keypool size, this is unlikely. Document better keypool size
implications to avoid user setting a too low value.
