fa29272459 Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47 Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841f 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 fa29272459
theStack:
ACK fa29272459🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
5d4597666d Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b3052739 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74fb16 Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98b59 Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399aba5 Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8acda Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad03657 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 5d4597666d
meshcollider:
Light utACK 5d4597666d
Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059
647b81b709 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 647b81b709 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
a6739cc868 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 a6739cc868
promag:
Code review ACK a6739cc868.
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.
da9caa1ced Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd5 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 da9caa1ced only doxygen improvements since my last review per `git diff d867d7a da9caa1`
MarcoFalke:
review ACK da9caa1ced📯
ryanofsky:
Code review ACK da9caa1ced. Just comment and test tweaks since previous review.
Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
ad57fb756b 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 ad57fb756b
theStack:
utACK ad57fb756b
meshcollider:
Code review ACK ad57fb756b
Tree-SHA512: 99cd7d836bffbdeb3d4e14053f7139cc85a6d42e631a3f9a3058a848042446b364faee127500f5acb374616e6a61ab2bedebfac1ba9bc993b4d6227114c2a6c2
ea0a7ec949 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 ea0a7ec949, 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.
281fd1a4a0 Replace KeyIDHasher with SaltedSipHasher (Andrew Chow)
210b693db6 Add generic SaltedSipHasher (Andrew Chow)
95e61c1cf2 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 281fd1a4a0
jonatack:
ACK 281fd1a4a0, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753
fjahr:
utACK 281fd1a4a0
Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b
faa8f68943 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 faa8f68943
fanquake:
ACK faa8f68943
Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
31b136e580 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee 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 31b136e580
jonatack:
ACK 31b136e580
theStack:
ACK 31b136e580❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
23cac24dd3 tests: Test bitcoin-wallet dump and createfromdump (Andrew Chow)
a88c320041 wallettool: Add createfromdump command (Andrew Chow)
e1e7a90d5f 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 23cac24dd3 only change is rebase and removing useless shared_ptr wrapper 🎼
ryanofsky:
Code review ACK 23cac24dd3. Only changes since last review rebase and changing a pointer to a reference
Tree-SHA512: 2d63cf62baca3d16495aa698dc02f7d889c81b41015e9c92c23c275bb4a690fc176d351c3fd7f310bd6b17f5a936cc9be694cbecd702af741b96c0f530e72fa2
cc3044ccdb 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 cc3044ccdb
Tree-SHA512: 882ff17836ef585a603dc504f3dd21f56f682e49b28a0998f23fd16025826fbb083b7978db3ee70d0e0ff2c86fd6c3fd99a2361e5d45c765fdc5822c5f14c0a7
5021810650 Make CanFlushToDisk a const member function (practicalswift)
281cf99554 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 5021810650
sipa:
utACK 5021810650
theStack:
Code Review ACK 5021810650🟢
Tree-SHA512: 38b7faccc2f16a499f9b7b1b962b49eb58580b2a2bbf63ea49dcc418a5ecc8f21a0972fa953f66db9509c7239af67cfa2f9266423fd220963d091034d7332b96
173cc9b7be test: walettool create descriptors (Ivan Metlushko)
345e88eecf wallettool: add param to create descriptors wallet (Ivan Metlushko)
6d3af3ab62 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 173cc9b7be
ryanofsky:
Code review ACK 173cc9b7be. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later
MarcoFalke:
Concept ACK 173cc9b7be🌠
Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622
fac39c1983 wallet: document that tx in CreateTransaction is purely an out-param (MarcoFalke)
faac31521b 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 fac39c1983
promag:
Code review ACK fac39c1983.
theStack:
Code review ACK fac39c1983
Tree-SHA512: e0c8cffce8d8ee0166b8e1cbfe85ed0657611e26e2af0d69fde70eceaa5d75cbde3eb489af0428fe4fc431360b4c791fb1cc21b8dee7d4c7a4f17df00836229d
f3d870fc22 wallet: List all wallets in non-SQLite or non-BDB builds (Russell Yanofsky)
d70dc89e78 refactor: Consolidate redundant wallet database path and exists functions (Russell Yanofsky)
6a7a63644c refactor: Drop call to GetWalletEnv in wallet salvage code (Russell Yanofsky)
6ee9cbdd18 refactor: Replace ListWalletDir() function with ListDatabases() (Russell Yanofsky)
5aaeb6cf87 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 f3d870fc22
promag:
Tested ACK f3d870fc22. Tested a --without-sqlite build with sqlite wallets.
Tree-SHA512: 029ad21559dbc338b5f351d05113c51bc25bce830f4f4e18bcd82287bc528275347a60249da65b91d252632aeb70b25d057bd59c704bfcaafb9f790bc5b59762
6fa72ceb80 test: add coverage for passing fee rate as a string (Jon Atack)
ce207d6b93 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 6fa72ceb80
achow101:
Code review ACK 6fa72ceb80
promag:
Code review ACK 6fa72ceb80.
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.
