510c6532ba Extract ParseDescriptorRange (Ben Woosley)
Pull request description:
So as to be consistently informative when the checks fail, and
to protect against unintentional divergence among the checks.
ACKs for commit 510c65:
meshcollider:
Oh apologies, yes. Thanks :) utACK 510c6532ba
MarcoFalke:
utACK 510c6532ba
sipa:
utACK 510c6532ba
Tree-SHA512: b1f0792bfaa163890a20654a0fc2c4c4a996659916bf5f4a495662436b39326692a1a0c825caafd859e48c05f5dd1865c4f7c28092be5074edda3c94f94f9f8b
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
facfb4111d rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931cf8f rpc: Add getbalances RPC (MarcoFalke)
fad13e925e rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec915 wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)
Pull request description:
This exposes the `CWallet::GetBalance()` struct over RPC.
In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.
ACKs for commit facfb4:
jnewbery:
utACK facfb4111d
Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
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
99e88a372 rpc: Remove dependency on interfaces::Chain in SignTransaction (Antoine Riard)
Pull request description:
Assuming wallet RPCs and node RPCs will go into different processes, signrawtransactionwithkey doesn't need to access Coins via interfaces::Chain, it may use directly utility in node/coins.cpp
Obviously will need rebase after #15638
Tree-SHA512: 42ee8fcbcd38643bbd82210db6f68249bed5ee036a4c930a1db534d0469a133e287b8869c977bf0cc79a7296dde04f72adb74d24e1cd20f4a280f4c2b7fceb74
710a7136f9 rpc: Speedup getaddressesbylabel (João Barbosa)
Pull request description:
Fixes#15447. Same approach of #14984, this change avoids duplicate key check when building the JSON response in memory.
ACKs for commit 710a71:
MarcoFalke:
utACK 710a7136f9
ryanofsky:
utACK 710a7136f9. Just new comments and assert since last review.
Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2
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.
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`
