ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
9e95931865 [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) 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.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
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
