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.
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
fad7c33342 refactor: Add handleNotifications method to wallet (MarcoFalke)
fa46ac3127 bench: Add wallet_balance benchmarks (MarcoFalke)
Pull request description:
ACKs for commit fad7c3:
ryanofsky:
utACK fad7c33342. I might squash or rearrange the commits to avoid adding code in one commit that just gets deleted in the next one. But overall this looks good and the cleanup is nice.
Tree-SHA512: 231faac168cbe9bb0ab4bf10ac1d5b042c610364406d75061fba27f1e9d16c71867e74cc4606e9f42659aa980d7133c00e29fcc18bbba7da2fa7a80178b3246c
faf400077d scripted-diff: Bump copyright headers in test, bench (MarcoFalke)
fa821904bf scripted-diff: Rename test_bitcoin to test/setup_common (MarcoFalke)
fa8685d49e test: Use test_bitcoin setup in bench, Add test utils (MarcoFalke)
666696b673 test: Have segwit always active in (Basic)TestingSetup (MarcoFalke)
Pull request description:
Now that the fuzz tests can use the BasicTestingSetup [1], do the same for bench.
Also move some duplicate code to a common "test/util" module.
[1]: fuzz: Link BasicTestingSetup (shared with unit tests) #15504
ACKs for commit faf400:
jonatack:
ACK faf400077d
Tree-SHA512: 8ac5692e72cf50e460958f291643ae6b8bb04d5c1331ed50dce9eb4e9457e5a925144c532c42b360a26707e11eeece74aab27db8c76ab9a429b9dd7167e7cdc4
b4338c151d [rpc] Remove the addresses field from the getaddressinfo return object (John Newbery)
Pull request description:
The "addresses" field was confusing because it refered to public keys
using their P2PKH address. It was included in the return object when
needed for backward compatibility. Remove that compatibility now that
the -deprecatedrpc=validateaddress option has been removed.
New applications should use the 'embedded'->'address' field for P2SH or
P2WSH wrapped addresses, and 'pubkeys' for inspecting multisig
participants.
ACKs for commit b4338c:
jonatack:
ACK b4338c151d. Tests [gist](https://gist.github.com/jonatack/31915e290bb1be39b9769dc9357385ca).
Tree-SHA512: 2c207510e565df600428838bfc6db5211fa06aaace365e31cbd74f1d2376b598675cb90df2fc1440858d49b22095aaa9d6b9ce3de0aff22417fe72cc6a6a321f
184f8785f wallet_bumpfee.py: add test for change key preservation (Gregory Sanders)
d08becff8 add functional tests for feerate bumpfee with adding inputs (Gregory Sanders)
0ea47ba7b generalize bumpfee to add inputs when needed (Gregory Sanders)
Pull request description:
When targeting a feerate using `bumpfee`, call a new function that directly uses `CWallet::CreateTransaction` and coin control to get the desired result. This allows us to get a superset of previous behavior, with an arbitrary RBF bump of a transaction provided it passes the preconditional checks and spare confirmed utxos are available.
Note(s):
0) The coin selection will use knapsack solver for the residual selection.
1) This functionality, just like knapsack coin selection in general, will hoover up negative-value inputs when given the chance.
2) Newly added inputs must be confirmed due to current Core policy. See error: `replacement-adds-unconfirmed`
3) Supporting this with `totalFee` is difficult since the "minimum total fee" option in `CreateTransaction` logic was (rightly)taken out in #10390 .
ACKs for commit 184f87:
jnewbery:
utACK 184f8785f7
Tree-SHA512: fb6542bdfb2c6010e328ec475cf9dcbff4eb2b1a1b27f78010214534908987a5635797196fa05edddffcbcf2987335872dc644a99261886d5cbb34a8f262ad3e
The "addresses" field was confusing because it refered to public keys
using their P2PKH address. It was included in the return object when
needed for backward compatibility. Remove that compatibility now that
the -deprecatedrpc=validateaddress option has been removed.
New applications should use the 'embedded'->'address' field for P2SH or
P2WSH wrapped addresses, and 'pubkeys' for inspecting multisig
participants.
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).
Moves the following wallet load functions to a new wallet/load unit in
the libbitcoin_wallet library. All other functions in wallet/init remain
in libbitcoin_server:
- `VerifyWallets`
- `LoadWallets`
- `StartWallets`
- `FlushWallets`
- `StopWallets`
- `UnloadWallets`
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`
rpc/rawtransaction.cpp moves to libbitcoin_server since it should not be
accessed by non-node libraries. The utility following utility methods
move to their own unit rpc/rawtransaction_util since they need to be
accessed by non-node libraries:
- `ConstructTransaction`
- `TxInErrorToJSON`
- `SignTransaction`
CheckTransaction is a context-free function that does not require access
to the blockchain or mempool. Move it from src/consensus/tx_verify in
lib_server to a new unit src/consensus/tx_check in lib_consensus so that
it can be called by non-server libraries.
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.
b5d398772 Take non-importing keys into account for spendability warning in descriptor import (Pieter Wuille)
6e597001a Import all origin info in importmulti; even for non-importing pubkeys (Pieter Wuille)
9a93c91c8 Keep full pubkeys in FlatSigningProvider::origins (Pieter Wuille)
Pull request description:
This fixes#15743 and #15742.
Since #15263, pubkeys are no longer imported for non-PKH (or WPKH, or any wrapped form of those) outputs, as that would incorrectly mark outputs to single-key versions of multisig policies as watched.
As a side effect, this change also caused origin info not to be imported anymore for multisig policies.
Fix this by plumbing through the full pubkey information for origins in FlatSigningProvider, and then importing all origin info we have in `importmulti` (knowing more never hurts, and additional origin information has no negative consequences like importing the pubkeys themselves).
ACKs for commit b5d398:
MeshCollider:
utACK b5d3987724
Tree-SHA512: 37caa2be8d01b8baa12f70a58eaa7c583f5f0afbe012e02936dd8790dc5dc852f880b77258b34ddb68cae30c029585f2d1c4f5d00015380557a1e8b471e500f3
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
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.
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
bb6195e34 refactor: Remove unused function (practicalswift)
Pull request description:
Last use removed in cad5dd2368.
Tree-SHA512: f65bf8f77b9aadbfba39bd80076a4d773eddf685a8a90ef2db549552a3d0ccd426ce3920b2f71954703f64d840fa88349957996d1f64a9c4d3f27a99b4da70e7
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.
