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
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
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
m_address_type was used for two things:
1. Determine the type of descriptor to generate during
SetupDescriptorGeneration
2. Sanity check during GetNewDestination.
There is no need to have this variable to accomplish those things.
1. Add a argument to SetupDescriptorGeneration indicating the address
type to use
2. Use Descriptor::GetOutputType for the sanity check.
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.
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.
This is a pure refactoring, no behavior is changing.
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.
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)
When a CWallet doesn't have a ScriptPubKeyMan for the requested type
in GetNewDestination, give a meaningful error. Also handle this in
Qt which did not do anything with errors.
FillPSBT will add our own scripts to the PSBT if those inputs are ours.
If an input also lists pubkeys that we happen to know the private keys
for, we will sign those inputs too.
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
92bcd70808 [wallet] allow transaction without change if keypool is empty (Sjors Provoost)
709f8685ac [wallet] CreateTransaction: simplify change address check (Sjors Provoost)
5efc25f963 [wallet] translate "Keypool ran out" message (Sjors Provoost)
Pull request description:
Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check.
Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error.
ACKs for top commit:
fjahr:
Code review ACK 92bcd70808
jonasschnelli:
utACK 92bcd70808
achow101:
ACK 92bcd70808
meshcollider:
Code review ACK 92bcd70808
Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
When a wallet uses avoid_reuse and has a large number of outputs in
a single destination, it groups these outputs in OutputGroups that
are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend
as many outputs as possible from the destination while not breaking
consensus due to a huge number of inputs and also not surprise the
use with high fees. If there are n outputs in a destination and
n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups
of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size
< OUTPUT_GROUP_MAX_ENTRIES.
Prior to this commit the coin selection in the case where
n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of
size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be
spent by the transaction is smaller than the aggregate of those
of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that
the coin selection decides between the different groups based on
fees and mostly the smaller group will cause smaller fees.
The behavior that users of the avoid_reuse flag seek is that the
full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This
commit implements this by pretending that the small group has
a large number of ancestors (one smallet than the maximum allowed
for this wallet). This dumps the small group to the bottom of the
list of priorities in the coin selection algorithm.
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
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
