fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke)
fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a68820758f9540653920deaeae6abe79
fjahr:
Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
fa292724598c273867bc6dbf311f1440fe2541ba Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47c0de4f3a0c5f5fe5b3ec32f611eefd Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948cac147198e3b685b5dd8cb72dc2911 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e65cae2d26bea3f991717a856e2fb39 refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841fe45bc49ebb6e07ac82a129fa9c40bf Remove unused CDataStream methods (MarcoFalke)
Pull request description:
Using `uint8_t` for raw bytes has a style benefit:
* The signedness is clear from reading the code, as it does not depend on the architecture
Other clean-ups in this pull include:
* Remove unused methods
* Constructor is simplified with `Span`
* Remove `Init()` member in favor of C++11 member initialization
ACKs for top commit:
laanwj:
code review ACK fa292724598c273867bc6dbf311f1440fe2541ba
theStack:
ACK fa292724598c273867bc6dbf311f1440fe2541ba 🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b305273910db0e46798d361413a7e878cb45f7 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74fb1687ae1d47a58c153d09d9afe0b6dc60 Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98b594b873f3d34c8ba63e9b55125d51b5a Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399aba5d27476b61b4865cc39553d03965d57 Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8acda5e594bb9b3b2d989056f9e03ddbdbd Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad036575ec998f8bbe4f10f6206b1c8ad3d23 Remove OutputGroup non-default constructors (Andrew Chow)
Pull request description:
Even after #17458, we still deal with setting fees of an `OutputGroup` and filtering the `OutputGroup` outside of the struct. We currently make all of the `OutputGroup`s in `SelectCoins` and then copy and modify them within each `SelectCoinsMinConf` scenario. This PR changes this to constructing the `OutputGroup`s within the `SelectCoinsMinConf` so that the scenario can be taken into account during the group construction. Furthermore, setting of fees and filtering for effective value is moved into `OutputGroup::Insert` itself so that we don't add undesirable outputs to an `OutputGroup` rather than deleting them afterwards.
To facilitate fee calculation and effective value filtering during `OutputGroup::Insert`, `OutputGroup` now takes the feerates in its constructor and computes the fees and effective value for each output during `Insert`.
While removing `OutputGroup`s in accordance with the `CoinEligibilityFilter` still requires creating the `OutputGroup`s first, we can do that within the function that makes them - `GroupOutput`s.
ACKs for top commit:
Xekyo:
Code review ACK: 5d4597666d
fjahr:
Code review ACK 5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e
meshcollider:
Light utACK 5d4597666d589e39354e0d8dd5b2afbe1a5d7d8e
Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059
647b81b70938dc4dbcf32399c56f78be395c721a wallet, rpc: add listdescriptors command (Ivan Metlushko)
Pull request description:
Looking for concept ACKs
**Rationale**: allow users to inspect the contents of their newly created descriptor wallets.
Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it:
* add an option to export xprv
* with #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes
The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes.
**Output example:**
```json
[
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h",
"timestamp": 1296688602,
"active": false,
"range": [
0,
999
],
"next": 0
}
]
```
ACKs for top commit:
jonatack:
re-ACK 647b81b70938dc4dbcf32399c56f78be395c721a rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet)
achow101:
re-ACK 647b81b
Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
a6739cc86827759c543bf81f5532ec46e40549c3 rpc: Add specific error code for "wallet already loaded" (Wladimir J. van der Laan)
Pull request description:
Add a separate RPC error code for "wallet already loaded" to avoid having to match on message to detect this.
Requested by shesek for rust-bitcoinrpc.
If concept ACKed needs:
- [ ] Release note
- [x] A functional test (updated the existing test to make it pass, I think this is enough)
ACKs for top commit:
jonasschnelli:
Code Review ACK a6739cc86827759c543bf81f5532ec46e40549c3
promag:
Code review ACK a6739cc86827759c543bf81f5532ec46e40549c3.
Tree-SHA512: 9091872e6ea148aec733705d6af330f72a02f23b936b892ac28f9023da7430af6332418048adbee6014305b812316391812039e9180f7f3362d11f206c13b7d0
This commit addresses #20809.
We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid.
da9caa1cedd69702aea44cb44b2fd0a2d6d56916 Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd532ac18b8b355ea93d25581a2c15654 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo)
Pull request description:
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
ACKs for top commit:
jonatack:
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 only doxygen improvements since my last review per `git diff d867d7a da9caa1`
MarcoFalke:
review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
ryanofsky:
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916. Just comment and test tweaks since previous review.
Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
ad57fb756b1c2df625790bd9c296ec28daa93740 wallet: Add BerkeleyDB version sanity check at init time (Wladimir J. van der Laan)
Pull request description:
Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation.
This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected.
ACKs for top commit:
decryp2kanon:
utACK ad57fb7
achow101:
ACK ad57fb756b1c2df625790bd9c296ec28daa93740
theStack:
utACK ad57fb756b1c2df625790bd9c296ec28daa93740
meshcollider:
Code review ACK ad57fb756b1c2df625790bd9c296ec28daa93740
Tree-SHA512: 99cd7d836bffbdeb3d4e14053f7139cc85a6d42e631a3f9a3058a848042446b364faee127500f5acb374616e6a61ab2bedebfac1ba9bc993b4d6227114c2a6c2
ea0a7ec949f0f7e212f0d8819f7a54cad2258bdd Remove deprecated bumpfee behavior (Andrew Chow)
Pull request description:
Removes the deprecation message, behavior, and test.
This was marked for removal in 22.0.
ACKs for top commit:
promag:
ACK ea0a7ec949f0f7e212f0d8819f7a54cad2258bdd, maybe add need release notes tag.
Tree-SHA512: d1626906849f6ee37213c32e5f8c1433ad8fb7beabcd88f5801b1964b322171a2341bdfbd9a3a5ab39b2fd9d9c6a05f73298583423a73cab1275653105c03e8e
Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation.
This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected.
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.
281fd1a4a032cded7f9ea9857e3e99fc793c714b Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db66e7c5b618014b5a287aee15af00045 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2a91d041c8025306ba36f0ea2806894 Move Hashers to util/hasher.{cpp/h} (Andrew Chow)
Pull request description:
There are existing `SaltedOutPointHasher` and `SaltedTxidHasher` classes used for `std::unordered_map` and `std::unordered_set` that could be useful in other places in the codebase. So we these to their own `saltedhash.{cpp/h}` file. An existing `KeyIDHasher` is moved there too. Additionally, `ScriptIDHasher`, `SaltedPubkeyHasher`, and `SaltedScriptHasher` are added so that they can be used in future work.
`KeyIDHasher` and `ScriptIDHasher` are not salted so that equality comparisons of maps and sets keyed by `CKeyID` and `CScriptID` will actually work.
Split from #19602 (and a few other PRs/branches I have).
ACKs for top commit:
laanwj:
Code review ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b
jonatack:
ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753ea
fjahr:
utACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b
Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
faa8f68943615785a2855676cf96e0e96f3cc6bd Replace boost::variant with std::variant (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency
ACKs for top commit:
fjahr:
Code review ACK faa8f68943615785a2855676cf96e0e96f3cc6bd
fanquake:
ACK faa8f68943615785a2855676cf96e0e96f3cc6bd
Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 Don't declare de facto const member functions as non-const (practicalswift)
Pull request description:
_Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_
Changes in this PR:
* Don't declare de facto const member functions as non-const
* Don't declare de facto const reference variables as non-const
Awards for finding candidates for the above changes go to:
* `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html))
* `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/))
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`.
ACKs for top commit:
ajtowns:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
jonatack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
theStack:
ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 ❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
23cac24dd3f2aaf88aab978e7ef4905772815cd2 tests: Test bitcoin-wallet dump and createfromdump (Andrew Chow)
a88c320041bd1cd1786b2dfd9ab698a67c2a57c6 wallettool: Add createfromdump command (Andrew Chow)
e1e7a90d5f0616a46ffadd62a9f1c65406cca6b4 wallettool: Add dump command (Andrew Chow)
Pull request description:
Adds two commands to the `bitcoin-wallet` tool: `dump` and `createfromdump`. These commands will be useful for a wallet storage migration in the future. It is also generally useful to have a storage agnostic dump like this. These commands are similar to BDB's `db_dump` and `db_load` tools. This can also be useful for manual construction of a wallet file for tests.
`dump` outputs every key-value pair from the wallet as comma separated hex. Each key-value pair is on its own line with the key and value in hex separated by a comma. This is output to the file specified by the new `-dumpfile` option.
`createfromdump` takes a file produced by `dump` and creates a new wallet file with exactly the records specified in that file.
A new option `-dumpfile` is added to the wallet tool. When used with `dump`, the records will be written to the specified file. When used with `createfromdump`, the file is read and the key-value pairs constructed from it. `createfromdump` requires `-dumpfile`.
A simple round-trip test is added to the `tool_wallet.py`.
This PR is based on #19334,
ACKs for top commit:
Sjors:
re-utACK 23cac24
MarcoFalke:
re review ACK 23cac24dd3f2aaf88aab978e7ef4905772815cd2 only change is rebase and removing useless shared_ptr wrapper 🎼
ryanofsky:
Code review ACK 23cac24dd3f2aaf88aab978e7ef4905772815cd2. Only changes since last review rebase and changing a pointer to a reference
Tree-SHA512: 2d63cf62baca3d16495aa698dc02f7d889c81b41015e9c92c23c275bb4a690fc176d351c3fd7f310bd6b17f5a936cc9be694cbecd702af741b96c0f530e72fa2
cc3044ccdbefa9fae58d1762477e377883b39c5e fix misleading comment about call to non-existing function (pox)
Pull request description:
The comment seems to be describing the subsequent call to `SyncTransaction` but refers to it as `SyncNotifications`, which is not any function currently in the codebase.
It's best to just remove the "what" aspect of the comment and focus on the "why", which also reduces the risk of similar documentation errors in the future, in case the function ever gets renamed, for example.
ACKs for top commit:
laanwj:
ACK cc3044ccdbefa9fae58d1762477e377883b39c5e
Tree-SHA512: 882ff17836ef585a603dc504f3dd21f56f682e49b28a0998f23fd16025826fbb083b7978db3ee70d0e0ff2c86fd6c3fd99a2361e5d45c765fdc5822c5f14c0a7
5021810650afc3073c2af6953ff046ad4d27a1fc Make CanFlushToDisk a const member function (practicalswift)
281cf995547f7683a9e9186bc6384a9fb6035d10 Do not run functions with necessary side-effects in assert() (practicalswift)
Pull request description:
Do not run functions with necessary side-effects in `assert()`.
ACKs for top commit:
laanwj:
Code review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc
sipa:
utACK 5021810650afc3073c2af6953ff046ad4d27a1fc
theStack:
Code Review ACK 5021810650afc3073c2af6953ff046ad4d27a1fc 🟢
Tree-SHA512: 38b7faccc2f16a499f9b7b1b962b49eb58580b2a2bbf63ea49dcc418a5ecc8f21a0972fa953f66db9509c7239af67cfa2f9266423fd220963d091034d7332b96
173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12 test: walettool create descriptors (Ivan Metlushko)
345e88eecf1b28607d5da3af38e19794a8a115ce wallettool: add param to create descriptors wallet (Ivan Metlushko)
6d3af3ab627096a824cb6a7ca1ebeddc7530361c wallettool: pass in DatabaseOptions into MakeWallet (Ivan Metlushko)
Pull request description:
Rationale: expose and promote descriptor wallets in more places; make cli tool more consistent with `createwallet` rpc.
Add `-descriptors` parameter which is off by default. When specified it will create a new descriptors wallet with sqlite backend, which is consistent with `createwallet` rpc.
This PR is based on a suggestion from **ryanofsky** https://github.com/bitcoin/bitcoin/pull/19137#discussion_r516779603
Example:
```
$ ./src/bitcoin-wallet -wallet=fewty -descriptors create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: sqlite
Descriptors: yes
Encrypted: no
HD (hd seed available): yes
Keypool Size: 6000
Transactions: 0
Address Book: 0
```
```
$ ./src/bitcoin-wallet -wallet=fewty create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: bdb
Descriptors: no
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2000
Transactions: 0
Address Book: 0
```
ACKs for top commit:
achow101:
ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12
ryanofsky:
Code review ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later
MarcoFalke:
Concept ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12 🌠
Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622
fac39c198324715565897f4240709340477af0bf wallet: document that tx in CreateTransaction is purely an out-param (MarcoFalke)
faac31521bb7ecbf999541cf918d3750ff589de4 Remove unused and confusing CTransaction constructor (MarcoFalke)
Pull request description:
The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it.
ACKs for top commit:
laanwj:
Code review ACK fac39c198324715565897f4240709340477af0bf
promag:
Code review ACK fac39c198324715565897f4240709340477af0bf.
theStack:
Code review ACK fac39c198324715565897f4240709340477af0bf
Tree-SHA512: e0c8cffce8d8ee0166b8e1cbfe85ed0657611e26e2af0d69fde70eceaa5d75cbde3eb489af0428fe4fc431360b4c791fb1cc21b8dee7d4c7a4f17df00836229d
f3d870fc2271bf45e0269e5ae135bced1a26f620 wallet: List all wallets in non-SQLite or non-BDB builds (Russell Yanofsky)
d70dc89e78ee6355e0bc37cc36cfc04ef7a86885 refactor: Consolidate redundant wallet database path and exists functions (Russell Yanofsky)
6a7a63644cd2fc56538d323cc0d5c1d7945247fd refactor: Drop call to GetWalletEnv in wallet salvage code (Russell Yanofsky)
6ee9cbdd18a70894f89dd268c276d5eb47a34827 refactor: Replace ListWalletDir() function with ListDatabases() (Russell Yanofsky)
5aaeb6cf877055c47fa2bbd2ea5e8d3d2033933b MOVEONLY: Move IsBDBFile, IsSQLiteFile, and ListWalletDir (Russell Yanofsky)
Pull request description:
This PR does not change behavior when bitcoin is built normally with both the SQLite and BDB libraries. It just makes non-SQLite and non-BDB builds more similar to the normal build. Specifically:
- It makes wallet directory lists always include all wallets so wallets don't appear missing depending on the build.
- It now triggers specific "Build does not support SQLite database format" and "Build does not support Berkeley DB database format" errors if a wallet can't be loaded instead of the more ambiguous and scary "Data is not in recognized format" error.
Both changes are implemented in the last commit. The previous commits are just refactoring cleanups that make the last commit possible and consolidate and reduce code.
ACKs for top commit:
achow101:
ACK f3d870fc2271bf45e0269e5ae135bced1a26f620
promag:
Tested ACK f3d870fc2271bf45e0269e5ae135bced1a26f620. Tested a --without-sqlite build with sqlite wallets.
Tree-SHA512: 029ad21559dbc338b5f351d05113c51bc25bce830f4f4e18bcd82287bc528275347a60249da65b91d252632aeb70b25d057bd59c704bfcaafb9f790bc5b59762
6fa72ceb8021c3b5aea62f6cfe92665c29212923 test: add coverage for passing fee rate as a string (Jon Atack)
ce207d6b93d35bc02fcd2dd28f1fd95869261d43 wallet, bugfix: allow send to take string fee rate values (Jon Atack)
Pull request description:
RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage.
ACKs for top commit:
MarcoFalke:
review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
achow101:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923
promag:
Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923.
Tree-SHA512: 735f9269cb1b81953764b5283449c0b154bd62de034225be5bcedc515c84faf767fe8fe0741008679fe412922c847b00d116cb11aab775236b779c847ba87167
Rewrite OutputGroups so that the logic is easier to follow and
understand.
There is a slight behavior change as OutputGroups will be grouped by
scriptPubKey rather than CTxDestination as before. This should have no
effect on users as all addresses are a CTxDestination. However by using
scriptPubKeys, we can correctly group outputs which fall into the
NoDestination case. But we also shouldn't have any NoDestination
outputs.
These calls are no longer needed after edc316020e8270dafc5e31465d532baebdafd3dd
from #19098 which started instantiating BasicTestingSetup.m_node.chain
Patch from MarcoFalke <falke.marco@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/19425#discussion_r526701954
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
