facec1c643 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
CommitTransaction returns a bool to indicate success, but since commit
b3a74100b8 it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
The wallet should not be able to directly access global configuration
from the node. Remove access of "-limitancestorcount" and
"-limitdescendantcount".
ea4cc3a7b3 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b3
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented
fa734603b7 wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5 wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)
Pull request description:
Comes with a test to aid review. The test should fail without the fix to bitcoind
The following `CreateWalletFromFile` issues are fixed:
* `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
* `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation
ACKs for top commit:
promag:
ACK fa734603b7.
darosior:
ACK fa734603b7
meshcollider:
LGTM, code-read ACK fa734603b7
Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
442a87cc0a Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d9 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133 Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd Encapsulate tx status in a Confirmation struct (Antoine Riard)
Pull request description:
While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone. To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.
Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
* https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
* https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
* `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection
Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.
ACKs for top commit:
meshcollider:
Light re-Code Review ACK 442a87cc0a
ryanofsky:
utACK 442a87cc0a. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.
Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
fa61365a13 wallet: Translate all initErrors in CreateWalletFromFile (MarcoFalke)
fa70d199d0 util: Make util/error bilingual_str (refactor) (MarcoFalke)
Pull request description:
The translations are going to close in three days (#15940), so I am submitting this as a standalone pull request.
Those changes are part of a bugfix #16661, which includes a test. The first change (the refactor) is required, the second commit is not. I am happy to drop it, if needed.
ACKs for top commit:
laanwj:
utACK fa61365a13
hebasto:
ACK fa61365a13, I have tested the code on Linux Mint 19.2.
Tree-SHA512: a7616cc38b9ffd301c6b915ea808a65815c3d97e9f57ec091772eb260e5cf0d75a13a6e4dfa3913e236833677c7929b9a748cb7d7a0e406d51749944b614e11b
d9d8984270 wallet: Use wallet name instead of pointer on unload/release (João Barbosa)
Pull request description:
Fixes#16668. Wallet name is unique so it can be used instead of pointer.
ACKs for top commit:
meshcollider:
utACK d9d8984270
instagibbs:
utACK d9d8984270
ryanofsky:
utACK d9d8984270. Alternately I think it might be possible to use an intptr_t set instead of a string set to get around the undefined behavior described in the issue.
Tree-SHA512: eccd4d260cd4c02b52c30deeb32dbfd190a1151a5340eb3aa4ece0dc6ae3b3ed746ce5617336461f6f27c437c435629cd07d20beb1c5450f23b75edde6728598
Add a LockChain method to CWallet to know if we can lock or query
chain state safely.
At tx loading, we rely on chain to know if hashBlock of tx is still
in main chain. If not, we set its status to unconfirmed and reset
its hashBlock/nIndex.
If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as status is not
used by wallet-tool.
We take lock prematurely in CWallet::LoadWallet and CWallet::Verify
to ensure that lock order is respected between cs_main an cs_wallet.
The function ExtractPubKey() checks if a given script matches the P2PK pattern
(<PubKey> OP_CHECKSIG), extracts the PubKey and additionally checks if it is
cryptographically valid (full validation with ECC library via .IsFullyValid()).
Currently this is done manually in the following order:
1) check if first script OP is data push with valid PubKey length
(first part of pattern match), extract PubKey
2) create CPubKey object with extracted PubKey
3) fully validate public key
4) check if last script OP is OP_CHECKSIG
(second part of pattern match)
Using Solver, the pattern matching and PubKey extraction can be done via a
single step, leading to the following simplified order with shorter code:
1) check if given script matches P2PK pattern with Solver
(also contains valid PubKey length check), extracts Pubkey
2) create CPubKey object with extracted Pubkey
3) fully validate public key
Translated strings should not end up in the debug log, stderr, or
returned by an RPC.
Changing the util methods in util/error to return a bilingual_str paves
the way to achieve this goal in the long term.
We shouldn't rely on this sync call to get an accurate view of txn
state, if a tx conflicts with one in mapTx we are going to update
our wallet dependencies in AddToWalletIfInvolvingMe while conflicting
txn get connected. If it doesn't conflict with one of our dependencies
we are not going to track it anyway.
This is a cleanup, as this SyncTransaction is redundant with the
following one for confirmation which is triggering the MarkConflicted
logic. We keep the loop because set of conflicted txn isn't same as txn
included in block.
Instead of relying on combination of hashBlock and nIndex
values to manage tx in its lifecycle, we introduce 4
status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED.
hashBlock and nIndex magic values should only be used at
serialization/deserialization for backward-compatibility.
At block disconnection, we know flag txn as UNCONFIRMED where
previously they kept their states until being override by a
block connection or abandontransaction call. This is a change
in behavior for which user may have to call abandon twice
if transaction is disconnected and not accepted back in the mempool.
We assert status transitioning right in AddToWallet. Doing so
flagged a misbehavior in ComputeTimeSmart unit test where same
tx is confirmed twice in different block. To avoid inconsistencies
we unconfirmed tx before new connection in different block. We
also remove a cs_main lock in test, as AddToWallet and its
callees don't rely on locked chain.
Restores the confirmed/conflicted tx check removed in
8753f5652b. There should be no external
behaviour change (these txs would not get accepted to the mempool
anyway), but not having the check in the wallet causes log spam.
Also adds a comment to ResentWalletTransactions() that
confirmed/conflicted tx check is done in SubmitMemoryPoolAndRelay().
IsCoinBase check is already performed early by
AcceptToMemoryPoolWorker
GetDepthInMainChain check is already perfomed by
BroadcastTransaction
To avoid deadlock we MUST keep lock order in
ResendWalletTransactions and CommitTransaction,
even if we lock cs_main again further.
in BroadcastTransaction. Lock order will need
to be clean at once in a future refactoring
80ba4241a6 extract min & max depth onto coin control (Amiti Uttarwar)
Pull request description:
- Refactor `AvailableCoins` to pull min & max depths from coin control.
- Add `m_max_depth` to coin control to support this.
- Addresses issue https://github.com/bitcoin/bitcoin/issues/15823, see thread for further details.
ACKs for top commit:
laanwj:
ACK 80ba4241a6
Tree-SHA512: 8f7c0aa90b3bc3667baf6741b1da2829f3919e1df92ae097d86c6b239f0c024eb410d7100e6251ea8fc49d022fb5a1214bf79b0f8b0014945b7784b2311647d1
CMerkleTx only exists as a base class for CWalletTx and for wallet file
serialization/deserialization. Move CMerkleTx methods into CWalletTx,
but leave class hierarchy and serialization logic in place.
fa6f22bf44 wallet: Rename CWalletKey to OldKey (MarcoFalke)
fa6dc7fa5f wallet: Enumerate walletdb keys (MarcoFalke)
Pull request description:
It is nice to see all the keys that exists in a single enum
Also, rename CWalletKey to OldKey and update the outdated documentation
ACKs for top commit:
laanwj:
ACK fa6f22bf44, I'm a big fan of this kind of change as it prevents typos, which can happen with 'magic' strings in the code.
promag:
ACK fa6f22bf44. @jnewbery suggestions are great followups, I think this is good enough.
meshcollider:
utACK fa6f22bf44
achow101:
Code review ACK fa6f22bf44
fanquake:
ACK fa6f22bf44 - I had a quick look over, definitely prefer this to strings floating around everywhere.
Tree-SHA512: 8ac3abd5a0d22dac1d77b8f97fe1e16c2608d650f3e9d6dd1df2fd5aeb35ef6643dfd4cd5c162404bb0100343c927d66df04dc695507ffc84a6c667e603acc54
fa4a605a4c Remove wallet settings from chainparams (MarcoFalke)
Pull request description:
Feels a bit odd to have wallet setting in the chainparams, so remove them from there
ACKs for top commit:
promag:
ACK fa4a605a4c, missed s/2018/2019?
practicalswift:
utACK fa4a605a4c
darosior:
ACK fa4a605a4c
Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
40ad2f6a58 Have importwallet use ImportPrivKeys and ImportScripts (Andrew Chow)
78941da5ba Optionally allow ImportScripts to set script creation timestamp (Andrew Chow)
94bf156f39 Have importaddress use ImportScripts and ImportScriptPubKeys (Andrew Chow)
a00d1e5ec5 Have importpubkey use CWallet's ImportScriptPubKeys and ImportPubKeys functions (Andrew Chow)
c6a8274247 Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and ImportScriptPubKeys (Andrew Chow)
fae7a5befd Log when an import is being skipped because we already have it (Andrew Chow)
ab28e31c95 Change ImportScriptPubKeys' internal to apply_label (Andrew Chow)
Pull request description:
#15741 introduced `ImportPrivKeys`, `ImportPubKeys`, `ImportScripts`, and `ImportScriptPubKeys` in `CWallet` which are used by `importmulti`. This PR changes the remaining `import*` RPCs (`importaddress`, `importprivkey`, `importpubkey`, and `importwallet`) to use these functions as well instead of directly adding the imported items to the wallet.
ACKs for top commit:
MarcoFalke:
ACK 40ad2f6a58 (checked that behavior changes are mentioned in the commit body)
ryanofsky:
utACK 40ad2f6a58. Only change since last review is a tweaked commit message (mentioning label update in importpubkey commit)
Sjors:
ACK 40ad2f6a5. Those extra tests also pass.
Tree-SHA512: 910e3bbe20b6f8809a47b7293775db234125615d886c7fd99c194f4cdf00c765eb1e24b1799260f1213b98c88f9bbe696796f36087c182925e567d44e9194c98
-BEGIN VERIFY SCRIPT-
sed -i 's/inline std::string _(const char\* psz)/inline bilingual_str _(const char\* psz)/' src/util/translation.h
sed -i 's/return G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz;/return bilingual_str{psz, G_TRANSLATION_FUN ? (G_TRANSLATION_FUN)(psz) : psz};/' src/util/translation.h
sed -i 's/\b_("\([^"]\|\\"\)*")/&.translated/g' $(git grep --files-with-matches '\b_("' src)
echo Hard cases - multiline strings.
sed -i 's/"Visit %s for further information about the software.")/&.translated/g' src/init.cpp
sed -i "s/\"Only rebuild the block database if you are sure that your computer's date and time are correct\")/&.translated/g" src/init.cpp
sed -i 's/" restore from a backup.")/&.translated/g' src/wallet/db.cpp
sed -i 's/" or address book entries might be missing or incorrect.")/&.translated/g' src/wallet/wallet.cpp
echo Special case.
sed -i 's/_(COPYRIGHT_HOLDERS)/&.translated/' src/util/system.cpp test/lint/lint-format-strings.py
-END VERIFY SCRIPT-
e10e1e8db0 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders)
d9ff862f2d CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders)
Pull request description:
The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used.
Implementers such as myself may fail to complete this pattern, and could result in key re-use: https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393
Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned.
Those failure cases appear to be:
`CommitTransaction` failing to get the transaction into the mempool
Belt and suspenders check in `WalletModel::prepareTransaction`
Alternative to https://github.com/bitcoin/bitcoin/pull/15796
ACKs for top commit:
achow101:
ACK e10e1e8db0 Reviewed the diff
stevenroose:
utACK e10e1e8db0
meshcollider:
utACK e10e1e8db0
Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b
The internal bool was only to indicate whether the given label should
be applied as things that are internal should not have a label. To make
this clearer, we change internal to apply_label and invert its usage
so things that have labels set this to true in order to have their labels
applied.
93ce4a0b6f Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow)
8f5b81e6ed Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow)
37a79a4fcc Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow)
16f8096e91 Move KeyOriginInfo to its own header file (Andrew Chow)
d9becff4e1 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow)
a913e3f2fb Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow)
c7797ec655 Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow)
1b699a5083 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow)
Pull request description:
This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is:
```
SigningProvider -> FillableSigningProvider -> CWallet
```
`SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`).
Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure.
ACKs for top commit:
Sjors:
re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.
instagibbs:
utACK 93ce4a0b6f
Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3
1aecdf2063 Move wallet creation out of the createwallet rpc into its own function (Andrew Chow)
Pull request description:
Moves the wallet creation logic from within the `createwallet` rpc and into its own function within wallet.cpp.
ACKs for top commit:
jnewbery:
ACK 1aecdf2063
MarcoFalke:
ACK 1aecdf2063
Sjors:
ACK 1aecdf2 with some suggestions for followup.
Tree-SHA512: 8d26d7ff48db4f8fac12408a5a294f788b7f50a72e7eb4008fb74ff14d7400eb3970f8038a19f989eff55198fc11c0cf86f52231c62b9015eb777132edc8ea88
0d101a340c test: Add test for maxtxfee option (MarcoFalke)
177550101b wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b9714cb wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)
Pull request description:
Follow up to #16257, this PR makes `bumpfee` aware of `-maxtxfee`.
It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.
ACKs for top commit:
MarcoFalke:
re-ACK 0d101a340c, only change is small test fixup
Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
8e7f930828 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2b Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow)
Pull request description:
The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool.
ACKs for top commit:
instagibbs:
re-utACK 8e7f930828
sipa:
ACK 8e7f930828. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
laanwj:
ACK 8e7f930828
Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.
Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'
fa64b947bb util: No translation of `Bitcoin Core` in the copyright (MarcoFalke)
fab85208f6 qt: Run «make translate» in ./src/ (MarcoFalke)
fabe87d2c9 scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke)
fa5e9f157e build: Stop translating PACKAGE_NAME (MarcoFalke)
Pull request description:
Generally the package name is not translated, but the package description is.
E.g. `GIMP` or `Firefox` are always called that way regardless of the system language. However, "`Firefox` webbrowser" or "`GIMP` image manipulation program" are translated.
ACKs for top commit:
hebasto:
ACK fa64b947bb, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 626f811531182d0ba0ef1044930d32726773349bcb49b10261288a86ee6b80a183db30a87d817d5b0d501fad058ac22d6272311716b4f5a154f17c6f391a5a1a
f874e14cd3 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56ff2 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb78d [build] detect std::system or ::wsystem (Sjors Provoost)
Pull request description:
Platforms such as iOs and Universal Windows Platform do not support launching a process through system().
ACKs for top commit:
laanwj:
code review ACK f874e14cd3
Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
5ebc6b0eb2 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8 doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fa wallet: avoid reuse flags (Karl-Johan Alm)
58928098c2 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb2
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb2 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
0db94e55d wallet: Pass WalletBatch to CWallet::UnsetWalletFlag (João Barbosa)
6cb888b37 Apply the batch treatment to CWallet::SetAddressBook via ImportScriptPubKeys (Ben Woosley)
6154a09e0 Move some of ProcessImport into CWallet::Import* (Ben Woosley)
ccb26cf34 Batch writes for importmulti (Andrew Chow)
d6576e349 Have WalletBatch automatically flush every 1000 updates (Andrew Chow)
366fe0be0 Add AddWatchOnlyWithDB, AddKeyOriginWithDB, AddCScriptWithDB functions (Andrew Chow)
Pull request description:
Instead of writing each item to the wallet database individually, do them in batches so that the import runs faster.
This was tested by importing a ranged descriptor for 10,000 keys.
Current master
```
$ time src/bitcoin-cli -regtest -rpcwallet=importbig importmulti '[{"desc": "sh(wpkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*))#3w7php47", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...
real 7m45.29s
```
This PR:
```
$ time src/bitcoin-cli -regtest -rpcwallet=importbig4 importmulti '[{"desc": "pkh([73111820/44h/1h/0h]tpubDDoT2SgEjaU5rerQpfcRDWPAcwyZ5g7xxHgVAfPwidgPDKVjm89d6jJ8AQotp35Np3m6VaysfUY1C2g68wFqUmraGbzhSsMF9YBuTGxpBaW/1/*)#v65yjgmc", "range": [0, 10000], "timestamp": "now", "internal": true, "keypool": false, "watchonly": true}]'
...
real 3.93s
```
Fixes#15739
ACKs for commit 0db94e:
jb55:
utACK 0db94e5
ariard:
Tested ACK 0db94e5
Empact:
re-utACK 0db94e55dc only change is re the privacy of `UnsetWalletFlagWithDB` and `AddCScriptWithDB`.
Tree-SHA512: 3481308a64c99b6129f7bd328113dc291fe58743464628931feaebdef0e6ec770ddd5c19e4f9fbc1249a200acb04aaf62a8d914d53b0a29ac1e557576659c0cc
When writing all of the imported data to the wallet, use a common
WalletBatch object so that batch writes are done and the writes
finish more quickly.
AddKeypoolPubkey is no longer needed so it is also removed
AddWatchOnlyWithDB, AddKeyOriginWithDB, and AddCScriptWithDB add their
respective data to the wallet using the provided WalletBatch instead
of creating a new WalletBatch object every time. This allows for batching
writes to the database.
fa7e311e16 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c2aa scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729242 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)
Pull request description:
This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
* that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
* that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.
ACKs for commit fa7e31:
promag:
utACK fa7e311e16.
jnewbery:
utACK fa7e311e16
Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8
78e407ad0c GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders)
70946e7fee Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders)
Pull request description:
The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types.
New types:
`CScriptID`->`ScriptHash`
`CKeyID`->`PKHash`
ACKs for commit 78e407:
ryanofsky:
utACK 78e407ad0c. Only changes are removing extra CScriptID()s and fixing the test case.
Sjors:
utACK 78e407a
meshcollider:
utACK 78e407ad0c
Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9
486c1eea86 refactoring: remove unused chainActive (James O'Beirne)
631940aab2 scripted-diff: replace chainActive -> ::ChainActive() (James O'Beirne)
a3a609079c refactoring: introduce unused ChainActive() (James O'Beirne)
1b6e6fcfd2 rename: CChainState.chainActive -> m_chain (James O'Beirne)
Pull request description:
This is part of the assumeutxo project:
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change refactors the `chainActive` reference into a `::ChainActive()` call. It also distinguishes `CChainState`'s `CChain` data member as `m_chain` instead of the current `chainActive`, which makes it easily confused with the global data.
The active chain must be obtained via function because its reference will be swapped at some point during runtime after loading a UTXO snapshot.
This change, though lengthy, should be pretty easy to review since most of it is contained within a scripted-diff. Once merged, the parent PR should be easier to review.
ACKs for commit 486c1e:
Sjors:
utACK 486c1ee
promag:
utACK 486c1ee.
practicalswift:
utACK 486c1eea86
Tree-SHA512: 06ed8f9e77f2d25fc9bea0ba86436d80dbbce90a1e8be23e37ec4eeb26060483e60b4a5c4fba679cb1867f61e3921c24abeb9cabdfb4d0a9b1c4ddd77b17456a
b6c748f849 doc: Add release notes for 15730 (João Barbosa)
d3e8458365 rpc: Show scanning details in getwalletinfo (João Barbosa)
90e27abe37 wallet: Track current scanning progress (João Barbosa)
2ee811e693 wallet: Track scanning duration (João Barbosa)
Pull request description:
Closes#15724.
ACKs for commit b6c748:
MarcoFalke:
re-utACK b6c748f849 (Only change since my last review is rebase, adding release notes, and returning false instead of null)
laanwj:
utACK b6c748f849
jonatack:
ACK b6c748f849, only changes appear to be rebase for https://github.com/bitcoin/bitcoin/pull/15730#discussion_r280030617 and release notes.
Tree-SHA512: 8ee98f971c15f66ce8138fc92c55e51abc9faf01866a31ac7ce2ad766aa2bb88559eabee3b5815d645c84cdf1c19dc35ec03f31461e39bc5f6040edec0b87116
Though at the moment ChainActive() simply references `g_chainstate.m_chain`,
doing this change now clears the way for multiple chainstate usage and allows
us to script the diff.
-BEGIN VERIFY SCRIPT-
git grep -l "chainActive" | grep -E '(h|cpp)$' | xargs sed -i '/chainActive =/b; /extern CChain& chainActive/b; s/\(::\)\{0,1\}chainActive/::ChainActive()/g'
-END VERIFY SCRIPT-
422677963a refactor: replace isPotentialtip/waitForNotifications by higher method (Antoine Riard)
edfe9438ca Add WITH_LOCK macro: run code while locking a mutex (Antoine Riard)
Pull request description:
In Chain interface, instead of a isPotentialTip and a WaitForNotifications method, both used only once in CWallet::BlockUntilSyncedToCurrentChain, combine them in a higher WaitForNotificationsUpToTip method. Semantic should be unchanged, wallet wait for pending notifications to be processed unless block hash points to the current chain tip or a descendant.
ACKs for commit 422677:
jnewbery:
ACK 422677963a
ryanofsky:
utACK 422677963a. Only change is adding the cs_wallet lock annotation.
Tree-SHA512: 2834ff0218795ef607543fae822e5cce25d759c1a9cfcb1f896a4af03071faed5276fbe0966e0c6ed65dc0e88af161899c5b2ca358a2d24fe70969a550000bf2
5c759c73b2 [wallet] Move maxTxFee to wallet (John Newbery)
Pull request description:
Closes#15355
Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details.
This is a cleanup. There is no change in behaviour.
Completes #15620
ACKs for commit 5c759c:
MarcoFalke:
utACK 5c759c73b2
ryanofsky:
utACK 5c759c73b2. Changes since last review: updated commit message and an error message and method name.
meshcollider:
utACK 5c759c73b2
Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752
c9e6e7ed7 wallet: add cachable amounts for caching credit/debit values (Karl-Johan Alm)
Pull request description:
This is a refactoring that will make #13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused).
It's also a nice change in general.
Tree-SHA512: 6c876d58bbffd5cb85ef632dea4fd6afed163904bbde5efdb307fa119af178ed3cb5df047255da7e9a9136fed876922f1116fce61a3710f308c72275f9b7d18b
765c0b364d refactor: combine Chain::findFirstBlockWithTime/findFirstBlockWithTimeAndHeight (Antoine Riard)
Pull request description:
As suggested in #14711, pass height to CChain::FindEarliestAtLeast to
simplify Chain interface by combining findFirstBlockWithTime and
findFirstBlockWithTimeAndHeight into one
ACKs for commit 765c0b:
jnewbery:
utACK 765c0b364d. Nice work @ariard!
ryanofsky:
utACK 765c0b364d. Looks good, thanks for implementing the suggestion!
Tree-SHA512: 63f98252a93da95f08c0b6325ea98f717aa9ae4036d17eaa6edbec68e5ddd65672d66a6af267b80c36311fffa9b415a47308e95ea7718b300b685e23d4e9e6ec
This commit moves the maxtxfee setting to the wallet. There is only
one minor behavior change:
- an error message in feebumper now refers to -maxtxfee instead of
maxTxFee.
7a9046e48 [wallet] Refactor CWalletTx::RelayWalletTransaction() (John Newbery)
Pull request description:
Refactor `CWalletTx::RelayWalletTransaction()` function.
This was a suggestion from the wallet-node separation PR: https://github.com/bitcoin/bitcoin/pull/15288#discussion_r256036330, which we deferred until after the main PR was merged.
There are also makes two minor behavior changes:
- no longer assert if fBroadcastTransactions is false. Just return false from the function.
- no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed).
ACKs for commit 7a9046:
promag:
utACK 7a9046e48d.
MeshCollider:
utACK 7a9046e48d
ryanofsky:
utACK 7a9046e48d. No changes at all, just rebase after base PR #15632 was merged
Tree-SHA512: 2ae6214cfadd917a1b3a892c4277e5e57c3eb791e17f67511470e6fbc634d19356554b9f9c55af6b779fdef821914aad59b7cc9e6c13ece145df003bf507d486
Remove last few instances of accesses to node global variables from wallet
code. Also remove accesses to node globals from code in policy/policy.cpp that
isn't actually called by wallet code, but does get linked into wallet code.
This is the last change needed to allow bitcoin-wallet tool to be linked
without depending on libbitcoin_server.a, to ensure wallet code doesn't access
node global state and avoid bugs like
https://github.com/bitcoin/bitcoin/pull/15557#discussion_r267735431
This refactors the CWalletTx::RelayWalletTransaction() function to be
clearer and adds comments. It also makes two minor behavior
changes:
- no longer assert if fBroadcastTransactions is false. Just return false
from the function.
- no longer print the relay message if p2pEnabled is set to false (since
the transaction is not actually relayed).
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
Removes the now-unused Broadcast/ResendWalletTransactions interface from
validationinterface.
The wallet_resendwallettransactions.py needs a sleep added at the start
to make sure that the rebroadcast scheduler is warmed up before the next
block is mined.
fabfb79673 doc: Add release notes for 15596 (MarcoFalke)
fac1a0fe54 wallet: Remove unused GetLegacyBalance (MarcoFalke)
faa3a246e8 scripted-diff: wallet: Rename pcoin to wtx (MarcoFalke)
fae5f874d5 rpc: Document that minconf is an ignored dummy value (MarcoFalke)
Pull request description:
Other RPCs such as `sendtoaddress` don't have this option at all and `sendmany` should by default spend from (lets say) our change.
ACKs for commit fabfb7:
jnewbery:
utACK fabfb79673
ryanofsky:
utACK fabfb79673. Nice writeup! Release notes are only change since previous review.
Tree-SHA512: 2526ead2330be7c2beb78b96bc5e55440566c4a3a809bbbd66f5c9fc517f6890affa5d14005dc102644d49679a374510f9507255e870cf88aaa63e429beef658
ea1a2d8794 [wallet] Remove ResendWalletTransactionsBefore (John Newbery)
f5162458cd [rpc] remove resendwallettransactions RPC (John Newbery)
Pull request description:
Remove resendwallettransactions RPC method
This RPC was added for testing wallet rebroadcasts. Since we now have a real test for wallet rebroadcasts, it's no longer needed.
The call in wallet_basic.py can be removed because wallet_resendwallettransactions.py tests wallet rebroadcast.
ACKs for commit ea1a2d:
MarcoFalke:
re-utACK ea1a2d8794
promag:
utACK ea1a2d8.
Tree-SHA512: 48245d947be1a2d2b8c30d2946105818c454a03b70b63534ecadf2144da64dafe1c9527ea670a5f4d1acd05ccdfc6c9be43ca636ee2ba58a8b7a7b2fc7bc88fd
As suggested in #14711, pass height to CChain::FindEarliestAtLeast to
simplify Chain interface by combining findFirstBlockWithTime and
findFirstBlockWithTimeAndHeight into one
Extend findearliestatleast_edge_test in consequence
d358466de Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b2389 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9f8 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e628 Remove use CValidationInterface in wallet code (Russell Yanofsky)
Pull request description:
This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).
This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:
```diff
- wtx.nTimeReceived = GetAdjustedTime();
+ wtx.nTimeReceived = m_chain->getAdjustedTime();
```
This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.
These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:
* Provide a single place to describe the interface between wallet and node code.
* Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).
Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7
faf369880 wallet: Improve log output for errors during load (Glenn Willen)
Pull request description:
When loading the wallet, display the entire path in error messages, instead of
the name (which, for the default wallet, is the empty string.)
When an exception occurs during wallet loading, display e.what() if possible,
instead of nothing.
Tree-SHA512: 435247628db669579bb694ba4b53ba174fe42c0329fc72f09fc274bb28463ee69f53412abb2a3b45bb8f59f7eb928c0167e397b8d0a514135142192a87294614
When loading the wallet, display the entire path in error messages, instead of
the name (which, for the default wallet, is the empty string.)
When an exception occurs during wallet loading, display e.what() if possible,
instead of nothing.
f7efd87c8f Change brace formatting (Russell Yanofsky)
a1df1b48a8 Remove use of IsInitialBlockDownload in wallet code (Russell Yanofsky)
1106a6fde4 Remove use of uiInterface.LoadWallet in wallet code (Russell Yanofsky)
318f41fb2c circular-dependencies: Avoid treating some .h/.cpp files as a unit (Russell Yanofsky)
d02b34c8a8 Remove use of AcceptToMemoryPool in wallet code (Russell Yanofsky)
e2c8ba9f6e Remove uses of InitMessage/Warning/Error in wallet code (Russell Yanofsky)
c5e59a96a8 Remove uses of GetAdjustedTime in wallet code (Russell Yanofsky)
6d6bcc77c0 Remove use of g_connman / PushInventory in wallet code (Russell Yanofsky)
00dfb2a440 Remove uses of g_connman in wallet code (Russell Yanofsky)
cc3836e8f9 Remove uses of fPruneMode in wallet code (Russell Yanofsky)
cc02c796d3 Remove uses of fee globals in wallet code (Russell Yanofsky)
1fb0a4a04e Remove use of CalculateMemPoolAncestors in wallet code (Russell Yanofsky)
cd32160af0 Remove use of GetTransactionAncestry in wallet code (Russell Yanofsky)
291276f7f4 Remove use of GetCountWithDescendants in wallet code (Russell Yanofsky)
bdc6628683 Remove use of IsRBFOptIn in wallet code (Russell Yanofsky)
80f52a2267 Remove uses of CheckFinalTx in wallet code (Russell Yanofsky)
Pull request description:
This change removes wallet calls to node functions that access global chain and mempool state.
This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.
Tree-SHA512: 40dbaf1f59fb22b32e70b054b30ba5638d638aa3240fa30e0f721d53c721cd6138a7ab4d423a24d7d2fda0b956e68d44c733abc2c9259c3d6c9fd6d4be89aa23
This also changes the uiInterface.LoadWallet signal argument type from
shared_ptr<CWallet> to unique_ptr<interfaces::Wallet> because CWallet is an
internal wallet class that shouldn't be used in non-wallet code (and also can't
be passed across process boundaries).
This commit does not change behavior.
When private keys are disabled, still fetch keys from the keypool
if the keypool has keys. Those keys come from importing them and
adding them to the keypool.
Store the master key fingerprint and derivation path in the
key metadata. hdKeypath is kept to indicate the seed and for
backwards compatibility, but all key derivation path output
uses the key origin info instead of hdKeypath.
Creates new files util/bip32.h and util/bip32.cpp for containing
BIP 32 stuff.
Moves FormatKeyPath from descriptor.cpp to util/bip32.
Adds a wrapper around it to prepent the 'm' for when just the
BIP 32 style keypath is needed.
1951ea434 gui: Show indeterminate progress dialog while opening walllet (João Barbosa)
8847cdaaa gui: Add OpenWalletActivity (João Barbosa)
4c8982a88 interfaces: Avoid interface instance if wallet is null (João Barbosa)
be82dea23 gui: Add thread to run background activity in WalletController (João Barbosa)
6c49a55b4 gui: Add Open Wallet menu (João Barbosa)
32a8c6abf gui: Add openWallet and getWalletsAvailableToOpen to WalletController (João Barbosa)
ab288b4e5 interfaces: Add loadWallet to Node (João Barbosa)
17abc0fd5 wallet: Factor out LoadWallet (João Barbosa)
Pull request description:
The *Open Wallet* menu has all the available wallets currently not loaded. The list of the available wallets comes from `listWalletDir`.
In the future the menu can be replaced by a custom dialog.
<img width="674" alt="screenshot 2019-01-12 at 12 17 02" src="https://user-images.githubusercontent.com/3534524/51073166-ac041480-1664-11e9-8302-be81702bc146.png">
Tree-SHA512: ebfd75eee0c8264863748899843afab67dadb7dff21313c11e3cb5b6108d954978dd1f1ae786bc07580c5a771ea4ab38d18c1643c9b9b3683ed53f0f6c582e38
7687f7873 [wallet] Support creating a blank wallet (Andrew Chow)
Pull request description:
Alternative (kind of) to #14938
This PR adds a `blank` parameter to the `createwallet` RPC to create a wallet that has no private keys initially. `sethdseed` can then be used to make a clean wallet with a custom seed. `encryptwallet` can also be used to make a wallet that is born encrypted.
Instead of changing the version number as done in #14938, a wallet flag is used to indicate that the wallet should be blank. This flag is set at creation, and then unset when the wallet is no longer blank. A wallet becomes non-blank when a HD seed is set or anything is imported. The main change to create a blank wallet is primarily taken from #14938.
Also with this, the term "blank wallet" is used instead of "empty wallet" to avoid confusion with wallets that have balance which would also be referred to as "empty".
This is built on top of #15225 in order to fix GUI issues.
Tree-SHA512: 824d685e11ac2259a26b5ece99c67a7bda94a570cd921472c464243ee356b7734595ad35cc439b34357135df041ed9cba951e6edac194935c3a55a1dc4fcbdea
A blank wallet is a wallet that has no keys, script or watch only things.
A new wallet flag indicating that it is blank will be set when the wallet
is blank. Once it is no longer blank (a seed has been generated, keys or
scripts imported, etc), the flag will be unset.
119d360aab travis: Document whether functional tests are run in the job name (Ben Woosley)
64f28545e3 Revert "travis: Compile trusty with depends for now" (Ben Woosley)
267eac00f9 Prefer boost::optional#get_value_or over #value_or (Ben Woosley)
1971f5ba04 Piecewise construct to avoid invalid construction (Ben Woosley)
Pull request description:
In light of #14979, I realized that only qt 5.5+ was being tested under CI, while compatibility lists 5.2+.
In #15276, Marco added Trusty to CI, building with depends. This changes that build to system libraries, in order to ensure ongoing compatibility with our claimed minimum required versions.
Fixes#14983, previously open as #14998
Tree-SHA512: 6cff5e28c756ecb8bf797c8f6eb77c1944ba61a8dd6d7d4984e63eef384f6429dc79c505da3241c05b9c4db31c72b2a9846c7365aba9280f2e0620e5f3998d07
e6c58d3b01 Do not import private keys to wallets with private keys disabled (Andrew Chow)
b5c5021b64 Refactor importwallet to extract data from the file and then import (Andrew Chow)
1f77f6754c tests: unify RPC argument to cli argument conversion and handle dicts and lists (Andrew Chow)
Pull request description:
Fixes a bug where private keys could be imported to wallets with private keys disabled. Now every RPC which can import private keys checks for whether the wallet has private keys are disabled and errors if it is. Also added an belt-and-suspenders check to `AddKeyPubkeyWithDB` to have it assert that the wallet has private keys enabled.
Tree-SHA512: 5cd04febce9aa2bd9bfd02f312c6ff8705e37278cae59efd3895f6d6e2f1b477aefd297e2dd0860791bdd3d4f3cad8eb1a404f8f3d4e2035b91314ad2c1028ae
14bc2a17dd Trivial: add doxygen-compatible comments relating to BerkeleyEnvironment (Pierre Rochard)
88b1d956fe Tests: add unit tests for GetWalletEnv (Pierre Rochard)
f1f4bb7345 Free BerkeleyEnvironment instances when not in use (Russell Yanofsky)
Pull request description:
Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map, use reference counted shared pointers and remove map entries when the last BerkeleyEnvironment reference goes out of scope.
This change was requested by @TheBlueMatt and makes code that sets up mock databases cleaner. The mock database environment will now go out of scope and be reset on destruction so there is no need to call BerkeleyEnvironment::Reset() during wallet construction to clear out prior state.
This change does affect bitcoin behavior slightly. On startup, instead of same wallet environments staying open throughout VerifyWallets() and OpenWallets() calls, VerifyWallets() will open and close an environment once for each wallet, and OpenWallets() will create its own environment(s) later.
Tree-SHA512: 219d77a9e2268298435b86088f998795e059fdab1d2050ba284a9ab8d8a44961c9b5cf96e94ee521688108d23c6db680e3e3a999b8cb2ac2a8590f691d50668b
3c3e31c3a4 [tests] Add wallet-tool test (João Barbosa)
49d2374acf [tools] Add wallet inspection and modification tool (Jonas Schnelli)
Pull request description:
Adds an offline tool `bitcoin-wallet-tool` for wallet creation and maintenance.
Currently this tool can create a new wallet file, display information on an existing wallet, and run the salvage and zapwallettxes maintenance tasks on an existing wallet. It can later be extended to support other common wallet maintenance tasks.
Doing wallet maintenance tasks in an offline tool makes much more sense (and is potentially safer) than having to spin up a full node.
Tree-SHA512: 75a28b8a58858d9d76c7532db40eacdefc5714ea5aab536fb1dc9756e2f7d750d69d68d59c50a68e633ce38fb5b8c3e3d4880db30fe01561e07ce58d42bceb2b
2bc4c3eaf9 Notify the GUI that the keypool has changed to set the receive button (Andrew Chow)
14bcdbe09c Check for more than private keys disabled to show receive button (Andrew Chow)
Pull request description:
Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. #14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once `sethdseed` is used so that a keypool can be generated and new keys generated. Likewise, with #14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled.
This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool.
Tree-SHA512: eff15a5337f4c64ecd7169414fb47053c04f6a0f0130341b6dd9799ac4d79f451e25284701c668971fca33f0909d5352a474a2c12349375bedfdb59b63077d50
This commit adds wallet-tool, a tool for creating and interacting with
wallet files. Original implementation was by Jonas Schnelli
<dev@jonasschnelli.ch> with modifications by John Newbery
<john@johnnewbery.com>
MSVC files were provided by Chun Kuan Lee <ken2812221@gmail.com>:
build: Add MSVC project files for bitcoin-wallet-tool
44de1561a Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f0264 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb07929 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1d6 Add time methods to the Chain interface (Russell Yanofsky)
700c42b85 Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)
Pull request description:
This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.
This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.
Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8
Whenever the keypool changes (new keys generated, new seed set,
keypool runs out, etc.), notify the GUI that the keypool has changed. The
receive button can then be enabled and disabled as necessary.
b301950df3 Made expicit constructor CTransaction(const CMutableTransaction &tx). (lucash-dev)
faf29dd019 Minimal changes to comply with explicit CMutableTransaction -> CTranaction conversion. (lucash-dev)
Pull request description:
This PR is re-submission of #14156, which was automatically closed by github (glitch?)
Original description:
This PR makes explicit the now implicit conversion constructor `CTransaction(const CMutableTransaction&)` in `transaction.h`.
Minimal changes were made elsewhere to make the code compilable. I'll follow up with other PRs to address individually refactoring functions that should have a `CMutableTransaction` version, or where a `CTransaction` should be reused.
The rationale for this change is:
- Conversion constructors should not be explicit unless there's a strong reason for it (in the opinion of, for example, https://google.github.io/styleguide/cppguide.html, and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Ro-conversion. Let me know your take on this).
- This particular conversion is very costly -- it implies a serialization plus hash of the transaction.
- Even though `CTransaction` and `CMutableTransaction` represent the same data, they have very different use cases and performance properties.
- Making it explicit allows for easier reasoning of performance trade-offs.
- There has been previous performance issues caused by unneeded use of this implicit conversion.
- This PR creates a map for places to look for possible refactoring and performance gains (this benefit still holds if the PR is not merged).
Tree-SHA512: 2427462e7211b5ffc7299dae17339d27f8c43266e0895690fda49a83c72751bd2489d4471b3993075a18f3fef25d741243e5010b2f49aeef4a9688b30b6d0631
Only change in behavior is "Rescan started from block <height>" message
replaced by "Rescan started from block <hash>" message in
ScanForWalletTransactions.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
645e905c32 doc: Add release notes for unloadwallet change to synchronous call (João Barbosa)
c37851de57 rpc: Make unloadwallet wait for complete wallet unload (João Barbosa)
Pull request description:
Currently the `unloadwallet` RPC is asynchronous, it only signals the intent to unload the wallet and then returns the response to the client. The actual unload can happen later and the client has no way to be notified of that.
This PR makes the `unloadwallet` RPC synchronous, meaning that it blocks until the wallet is fully unloaded.
Replaces #14919, fixes#14917.
Tree-SHA512: ad88b980e2f3652809a58f904afbfe020299f3aa6a517f495ba943b8d54d4520f6e70074d6749be8f5967065c0f476e0faedcde64c8b4899e5f99c70f0fd6534
This commit makes the minimal changes necessary to fix compilation once CTransaction(const CMutableTransaction &tx) is made explicit. In each case an explicit call `CTransaction(...)` was added. Shouldn't affect behaviour or performance.
0e75f44a0 Replace CAffectedKeysVisitor with descriptor based logic (Pieter Wuille)
Pull request description:
It seems we don't need a custom visitor pattern anymore to find what keys are affected by a script. Instead, infer the descriptor, and see which keys it expands to.
Tree-SHA512: 8a52f61fb74e8ebfd8d02e759629e423ced6bd7d9a9ee7c4bdd2cca8465bc27b951cc69c8d835244a611ba55c6d22f83b81acef05487cb988c88c0951b797699
bd3b0361d Add stop_block out arg to ScanForWalletTransactions (Ben Woosley)
3002d6cf3 Return a status enum from ScanForWalletTransactions (Ben Woosley)
bb24d6865 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley)
Pull request description:
Return the failed block as an out arg.
Fixes#11450.
/cc #12275
Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9
0fb2e69815 CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change (Gregory Sanders)
b06483c96a Remove stale comment in CalculateMaximumSignedInputSize (Gregory Sanders)
Pull request description:
This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn't know how to sign for.
This regression was added in 6a34ff5335 since BnB coin selection actually cares about this.
The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a "prototype" dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.
I also removed a comment which I believe is stale and misleading.
Tree-SHA512: c7e2be189e524f81a7aa4454ad9370cefba715e3781f1e462c8bab77e4d27540191419029e3ebda11e3744c0703271e479dcd560d05e4d470048d9633e34da16
Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map,
use reference counted shared pointers and remove map entries when the last
BerkeleyEnvironment reference goes out of scope.
This change was requested by Matt Corallo <git@bluematt.me> and makes code that
sets up mock databases cleaner. The mock database environment will now go out
of scope and be reset on destruction so there is no need to call
BerkeleyEnvironment::Reset() during wallet construction to clear out prior
state.
This change does affect bitcoin behavior slightly. On startup, instead of same
wallet environments staying open throughout VerifyWallets() and OpenWallets()
calls, VerifyWallets() will open and close an environment once for each wallet,
and OpenWallets() will create its own environment(s) later.
591203149f wallet: Create IsDatabaseLoaded function (Chun Kuan Lee)
15c93f075a wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. (Chun Kuan Lee)
c456fbd8df Refactor: Move m_db pointers into BerkeleyDatabase (Russell Yanofsky)
Pull request description:
Fix#14538
Fix crash attempting to load the same wallet with different path strings that resolve to the same absolute path. The primary check which prevents loading the same wallet twice is:
6b8d0a2164/src/wallet/db.cpp (L44)
But this check is skipped if both wallet paths resolve to the same absolute path, due to caching here:
6b8d0a2164/src/wallet/db.cpp (L467)
Meanwhile a secondary check for duplicate wallets is not reliable because it based on a literal comparison, instead of comparison using absolute paths:
6b8d0a2164/src/wallet/wallet.cpp (L3853)
This PR fixes the latter check to compare the absolute path of a new wallet being loaded to absolute paths of wallets already loaded, so there should no longer be any way to load the same wallet more than once.
Tree-SHA512: 2fa01811c160b57be3b76c6b4983556a04bbce71a3f8202429987ec020664a062e897deedcd9248bc04e9baaa2fc7b464e2595dcaeff2af0818387bf1fcdbf6f
Accurately reports the last block successfully scanned, replacing a return of
the chain tip, which represented possibly inaccurated data in a race condition.
This commit does not change behavior. All it does is pass new function
parameters.
It is easiest to review this change with:
git log -p -n1 -U0 --word-diff-regex=.
This commit does not change behavior. All it does is pass new function
parameters.
It is easiest to review this change with:
git log -p -n1 -U0 --word-diff-regex=.
