1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
57ff5a42ab doc: specify minimum HWI version (Sjors Provoost)
03308b2bfa rpc: don't require wallet for enumeratesigners (Sjors Provoost)
Pull request description:
HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0
As of #16546 we already rely on features that are in 2.0 and not in the previous 1.* releases:
* `--chain` param
This shouldn't be a problem, because HWI 2.0 has been released before we release v22.
Misc improvements:
* document that HWI 2.0 is required
* drop wallet requirement for `enumeratesigners`
ACKs for top commit:
achow101:
Code Review ACK 57ff5a42ab
Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.
Does not change behavior.
Make sure that all fee calculations use the same feerate.
coin_selection_params.effective_fee is the variable we use for all fee
calculations, so get rid of remaining nFeeRateNeeded usages and just
directly set coin_selection_params.effective_fee.
Does not change behavior.
During each loop of CreateTransaction, instead of constantly getting a
new feerate, use the feerate that we have already fetched for all
fee calculations. Thix fixes a race condition where the feerate required
changes during each iteration of the loop.
This commit changes behavior as the "Fee estimation failed" error will
now take priority over "Signing transaction failed".
9048c58e10 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky)
14f3d9b908 refactor: Add RPC server ExecuteCommands function (Russell Yanofsky)
6158a6d397 refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky)
Pull request description:
This change is needed to fix the `rpc_help.py` test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275
The [`CRPCTable::dumpArgMap`](16b784d953/src/rpc/server.cpp (L492)) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR #10102 because wallet RPC functions aren't directly accessible from the node process.
Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
MarcoFalke:
re-ACK 9048c58e10👑
Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
7c90c67b7e rpc: refactor rpc wallet functions to take references instead of pointers (fanquake)
4866934008 rpc: remove calls to CWallet.get() (fanquake)
Pull request description:
This is a rebased #18592.
> This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here https://github.com/bitcoin/bitcoin/issues/18590
> It seems that this PR is indirectly related to this issue: https://github.com/bitcoin/bitcoin/pull/13063#discussion_r186740049
> Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".
> Fixes https://github.com/bitcoin/bitcoin/issues/18590
ACKs for top commit:
MarcoFalke:
ACK 7c90c67b7e🐧
ryanofsky:
Code review ACK 7c90c67b7e. Changes easy to review with `--word-diff-regex=. -U0`
Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
fa4e088cba wallet: Mark replaced tx to not be in the mempool anymore (MarcoFalke)
Pull request description:
The wallet does not mark the replaced tx as out-of-mempool. This causes failures in user scripts, because later RPCs may depend on this state change from `bumpfee`.
For example, the following might fail on current master:
```
txid = sendtoaddress(...)
bumpfee(txid)
abandontransaction(txid) # fails because txid is still marked as "in mempool"
```
Fixes#18831
ACKs for top commit:
meshcollider:
utACK fa4e088cba
ryanofsky:
Code review ACK fa4e088cba, and previous ACK faeedff5c87091fd83d2fb2b29eb49c948363f29 is also still valid in case there's a preference for the original fix
Tree-SHA512: 9858f40f5fb5a43a7b584b5c4268b6befa82e6a84583be5206fe721bcb6c255e8d35479d347d0b9aed72703df49887c02b14ab680e8efdd28b90dd6b93d9439a
48a0319bab Add a test that selects too large if BnB is used (Andrew Chow)
3e69939b78 Fail if maximum weight is too large (Andrew Chow)
51e2cd322c Have CalculateMaximumSignedTxSize also compute tx weight (Andrew Chow)
Pull request description:
Currently the `Transaction too large` is calculated on the transaction that is returned from `CreateTransaction`. This does not make sense for when `CreateTransaction` is being used for `fundrawtransaction` as no signing occurs so the final returned transaction is missing signatures. Thus users may successfully fund a transaction but fail to broadcast it after it has been fully signed.
So instead we should figure out whether the transaction we are funding will be too large after it is signed. We can do this by having `CalculateMaximumSignedTxSize` also return the transaction weight and then comparing that weight against the maximum weight.
ACKs for top commit:
instagibbs:
ACK 48a0319bab
meshcollider:
utACK 48a0319bab
Xekyo:
utACK with nits 48a0319bab
Tree-SHA512: 1700c60b07f67e2d5c591c5ccd131ac9f1861fab3def961c3c9c4b3281ec1063fe8e4f0f7f1038cac72692340856406bcee8fb45c8104d2ad34357a0ec878ac7
This simplifies code and adds a less cumbersome interface for accessing
address used information than CWallet AddDestData / EraseDestData /
GetDestData methods.
There is no change in behavior. Lower-level walletdb DestData methods
are also still available and not affected by this change. If there is
interest in consolidating destdata logic more and making it internal to
walletdb, #18608 could be considered as a followup.
Stop giving GUI access to destdata rows in database. Replace with narrow
API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with
other destdata like address-used status.
Note: No user-visible behavior is changing in this commit. New
CWallet::SetAddressReceiveRequest() implementation avoids a bug in
CWallet::AddDestData() where a modification would leave the previous
value in memory while writing the new value to disk. But it doesn't
matter because the GUI doesn't currently expose the ability to modify
receive requests, only to add and erase them.
6bfbc97d71 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019e76 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)
Pull request description:
Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.
Fixes#21104
ACKs for top commit:
jonatack:
ACK 6bfbc97d71
meshcollider:
utACK 6bfbc97d71
kristapsk:
ACK 6bfbc97d71. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.
Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
de6b389d5d tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152 wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5d
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5d modulo a few minor comments
fjahr:
Code review ACK de6b389d5d
meshcollider:
Tested ACK de6b389d5d
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
9305862f71 wallet: load flags before everything else (Sjors Provoost)
Pull request description:
Load and set wallet flags before processing other records. That way we can take them into account while processing those other records.
Suggested here:
https://github.com/bitcoin/bitcoin/pull/16546#discussion_r572334983
ACKs for top commit:
laanwj:
Code review ACK 9305862f71
gruve-p:
ACK 9305862f71
achow101:
ACK 9305862f71
Tree-SHA512: 7104523e369ce3c670571fe5e8b52c67b9ca92b8e36a2da5eb6f9f8bf8ed0544897007257204b68f6f371d682b3ef0d0635d36e6e8416ac74af1999d9fbc869c
fa650ca7f1 Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f1: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad 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 fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
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.
This commit 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.
No change in behavior. Just remove a little bit of code, reduce macro usage,
remove duplicative functions, and make BDB and SQLite implementations more
consistent with each other.
No observable change in behavior. This just avoids a redundant environment
lookup. Motivation is to be able to simplify the GetWalletEnv implementation in
an upcoming commit.
This commit does not change to any code and behavior. It it is easily reviewed
with the --color-moved=dimmed_zebra git diff option.
Motivation for this change is to:
- Consolidate redundant functions
IsBDBFile /ExistsBerkeleyDatabase / SplitWalletPath, and
IsSQLiteFile / ExistsSQLiteDatabase in the next commits
- Detect SQLite wallets consistently regardless whether bitcoin is built with
SQLite support in the next commits
- Avoid attempting to open SQLite databases with the BDB library when bitcoin
is built without SQLite support in the next commits
1e62350ca2 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca2: patch looks correct!
MarcoFalke:
review ACK 1e62350ca2
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
9b74461fa2 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa)
021feb3187 refactor: Drop redudant CWallet::GetDBHandle (João Barbosa)
Pull request description:
ACKs for top commit:
achow101:
Code Review ACK 9b74461fa2
meshcollider:
utACK 9b74461fa2
ryanofsky:
Code review ACK 9b74461fa2. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like
Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
629a9299b2 Move WalletImpl from interfaces/wallet.cpp to wallet/interfaces.cpp (Russell Yanofsky)
2a26771d81 Move ChainImpl from interfaces/chain.cpp to node/interfaces.cpp (Russell Yanofsky)
12bd0fc9d7 Move NodeImpl from interfaces/node.cpp to node/interfaces.cpp (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
Move `NodeImpl` from `interfaces/node.cpp` to `node/interfaces.cpp`
Move `ChainImpl` from `interfaces/chain.cpp` to `node/interfaces.cpp`
Move `WalletImpl` from `interfaces/wallet.cpp` to `wallet/interfaces.cpp`
No changes to any classes (can review with `git diff --color-moved=dimmed_zebra`)
Motivation for this change is to move node and wallet code to respective directories where it might fit in better than `src/interfaces/`, but also to remove all unnecessary code from `src/interfaces/` to unblock #19160 review, which has been hung up partially because of code organization. Building on top of this PR, #19160 should now be able to organize interface implementations more understandably in `src/node/` `src/wallet/` `src/ipc/` and `src/init/` directories instead of having so much functionality all in `src/interfaces/`
ACKs for top commit:
promag:
Code review ACK 629a9299b2.
MarcoFalke:
review ACK 629a9299b2🔺
Tree-SHA512: 87c2b8fd51519bbd4e5ad3539a79debcf88c3bf021eb28c63f3f555186538b62a0c4cc1a3f07cfb4ff13aea8b0b2fdde505d81f22a5e5fd12a6e375b55a92ab8
Our max weight check in CreateTransaction only worked if the transaction
was fully signed. However if we are funding a transaction, it is
possible that the tx weight will be too large for a standard tx. In that
case, we should also fail. So we use the tx weight returned by
CalculateMaximumSignedTxSize and check against the limit for those
transactions.
89bdad5b25 RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint (Luke Dashjr)
Pull request description:
Allow specifying the `wallet_name` param to `unloadwallet` on RPC wallet endpoints, so long as it matches the endpoint wallet.
ACKs for top commit:
jonatack:
ACK 89bdad5b25
MarcoFalke:
review ACK 89bdad5b25
Tree-SHA512: efb399c33f7b5596870a26a8680f453ca47aa7a6db4e550f9435d13044f1c4bad0ae11e8f0205213409d08b75c4188c3be782e54aafab1f65b97eb8cf5c252a9
3eb6f8b2e6 wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e3571 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce8 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e6 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
fa69c2c784 wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Equating `0==None` and `""==None` is confusing, unneeded and undocumented
ACKs for top commit:
jonatack:
ACK fa69c2c784
achow101:
ACK fa69c2c784
Sjors:
tACK fa69c2c784 modulo `unset`
Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
b1f59d55d9 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)
Pull request description:
Just documentation clarifications from #20448
ACKs for top commit:
MarcoFalke:
review ACK b1f59d55d9
jonatack:
re-ACK b1f59d55d9 per `git diff e8303a0 b1f59d5`
Tree-SHA512: ac068b0aa7ceed49496367fdd9425b59dbba18b56e89b26afc22a6c8ece51f0b92a169cacd55740b1cadab2b32f4f8e8700e609066ab7e59d3b53c7891da585e
d52f502b1e Fix mock SQLiteDatabases (Andrew Chow)
99309ab3e9 Allow disabling BDB in configure with --without-bdb (Andrew Chow)
ee47f11f73 GUI: Force descriptor wallets when BDB is not compiled (Andrew Chow)
71e40b33bd RPC: Require descriptors=True for createwallet when BDB is not compiled (Andrew Chow)
6ebc41bf9c Enforce salvage is only for BDB wallets (Andrew Chow)
a58b719cf7 Do not compile BDB things when USE_BDB is defined (Andrew Chow)
b33af48210 Include wallet/bdb.h where it is actually being used (Andrew Chow)
Pull request description:
Adds a `--without-bdb` option to `configure` which disables the compilation of the BDB stuff. Legacy wallets will not be created when BDB is not compiled. A legacy-sqlite wallet can be loaded, but we will not create them.
Based on #20156 to resolve the situation where both `--without-sqlite` and `--without-bdb` are provided. In that case, the wallet is disabled and `--disable-wallet` is effectively set.
ACKs for top commit:
laanwj:
Code review ACK d52f502b1e
Tree-SHA512: 5a92ba7a542acc2e27003e9d4e5940e0d02d5c1f110db06cdcab831372bfd83e8d89c269caff31dd5bff062c1cf5f04683becff12bd23a33be731676f346553d
A check to raise an error on zero-fee txns was mistakenly extended in commit
a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include
fundrawtransaction and walletcreatefundedpsbt.
This commit overrides zero fee rate checking for these two RPCs, not only for
the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new
fee_rate (sat/vB) arg.
7486e2771e Tests: Unit test related to WalletDB ReadKeyValue (Bushstar)
32def8d1c2 Catch ios_base::failure specifically (Peter Bushnell)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/2950 a hash of the pubkey and private was added to speed up key import, this was made backwards compatible by reading the hash in a try block with an ellipses catch all in case the hash was not present.
CDataStream::read() specifically throws std::ios_base::failure, backwards compatibility expects only that error to be thrown, if something else gets thrown we should not be catching it. The change in this commit is to catch that exception only. If any other exception is thrown other than std::ios_base::failure it will be caught by the wider try block and an error written to the log and/or console.
CDataStream::read() throwing std::ios_base::failure.
2c364fde42/src/streams.h (L191)
Wider catch statements that pick up all others exceptions other than ios_base::failure.
2c364fde42/src/wallet/walletdb.cpp (L425)2c364fde42/src/wallet/walletdb.cpp (L430)
ACKs for top commit:
laanwj:
Code review ACK 7486e2771e
Tree-SHA512: 5364bf935af8ec603bf5b8fef8c23b5cdaa4fe3506090cff988413221f2eaa99f7a91929afb42a35f8881ce2328744a0d32052da51ca0a5b2e65b6809e97f604
It is unnecessary to upgrade to FEATURE_HD_SPLIT if this feature is
already supported by the wallet. Because upgrading to FEATURE_HD_SPLIT
actually requires upgrading to FEATURE_PRE_SPLIT_KEYPOOL, users would
accidentally be upgraded to FEATURE_PRE_SPLIT_KEYPOOL instead of nothing
being done.
Fixes the issue described at
https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
05e82d86b0 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack)
9a670b4f07 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack)
be481b72e2 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack)
449b730579 wallet: provide valid values if invalid estimate mode passed (Jon Atack)
6da3afbaee wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack)
173b5b5fe0 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack)
7f9835a05a wallet: remove fee rates from conf_target helps (Jon Atack)
b7994c01e9 wallet: add fee_rate unit warnings to bumpfee (Jon Atack)
410e471fa4 wallet: remove redundant bumpfee fee_rate checks (Jon Atack)
a0d4957473 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack)
e21212f01b wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack)
6112cf20d4 wallet: add CFeeRate ctor doxygen documentation (Jon Atack)
3f72791613 wallet: fix bug in RPC send options (Jon Atack)
Pull request description:
This PR builds on #11413 and #20220 to address #19543.
- replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs
- allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument
- fix a bug in the experimental send RPC described in https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 where args were not being passed correctly into the options values
- update the feerate error message units for these RPCs from BTC/kB to sat/vB
- update the test coverage, help docs, doxygen docs, and some of the RPC examples
- other changes to address the excellent review feedback
See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309
ACKs for top commit:
achow101:
re-ACK 05e82d8
MarcoFalke:
review ACK 05e82d86b0 did not test and found a few style nits, which can be fixed later 🍯
Xekyo:
tACK 05e82d86b0
Sjors:
utACK 05e82d86b0
Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
5f9c0b6360 wallet: Remove -upgradewallet from dummywallet (MarcoFalke)
a314271f08 test: Remove unused wallet.dat (MarcoFalke)
bf7635963c tests: Test specific upgradewallet scenarios and that upgrades work (Andrew Chow)
4b418a9dec test: Add test_framework/bdb.py module for inspecting bdb files (Andrew Chow)
092fc43485 tests: Add a sha256sum_file function to util (Andrew Chow)
0bd995aa19 wallet: upgrade the CHDChain version number when upgrading to split hd (Andrew Chow)
8e32e1c41c wallet: remove nWalletMaxVersion (Andrew Chow)
bd7398cc62 wallet: have ScriptPubKeyMan::Upgrade check against the new version (Andrew Chow)
5f720544f3 wallet: Add GetClosestWalletFeature function (Andrew Chow)
842ae3842d wallet: Add utility method for CanSupportFeature (Andrew Chow)
Pull request description:
This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.
The `nWalletMaxVersion` member variable has been removed as it made `CanSupportFeature` unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old `-upgradewallet` option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the `upgradewallet` RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. `nWalletMaxVersion` was only relevant for upgrades to `FEATURE_WALLETCRYPT` and `FEATURE_COMPRPUBKEY` both of which are incredibly old features. So for such wallets, the behavior of `upgradewallet` will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that `FEATURE_WALLETCRYPT` indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.
`CanSupportFeature` would previously indicate whether we could upgrade to `nWalletMaxVersion` not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into `ScriptPubKeyMan::Upgrade` the version we are upgrading to and checking against that. By removing `nWalletMaxVersion` we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.
`nWalletMaxVersion` was also the version that was being reported by `getwalletinfo` which meant that the version reported was not always consistent across restarts as it depended on whether `upgradewallet` was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to `upgradewallet`, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.
Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.
Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to `upgradewallet`, `UpgradeKeyMetadata` is now being tested too.
Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function `sha256sum_file` has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new `bdb.py` module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file `dbinc/db_page.h` describe it. This module just dumps all of the fields into a dict.
ACKs for top commit:
MarcoFalke:
approach ACK 5f9c0b6360
laanwj:
Code review ACK 5f9c0b6360
jonatack:
ACK 5f9c0b6360, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the `wallet_upgradewallet.py` test runs green both before and after running `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`
Tree-SHA512: 7c4ebf420850d596a586cb6dd7f2ef39c6477847d12d105fcd362abb07f2a8aa4f7afc5bfd36cbc8b8c72fcdd1de8d2d3f16ad8e8ba736b6f4f31f133fe5feba
24d2d3341d QA: wallet_multiwallet: Check that recursive symlink directory and wallet.dat loops are ignored (Luke Dashjr)
69f59af54d Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Luke Dashjr)
Pull request description:
Previously, an exception would be thrown, which could kill the node in some circumstances.
Includes test changes to cause failure.
Review with `?w=1`
ACKs for top commit:
hebasto:
re-ACK 24d2d3341d, rebased only since my [previous](https://github.com/bitcoin/bitcoin/pull/19502#pullrequestreview-520552944) review.
promag:
Tested ACK 24d2d3341d, test change fails on master.
meshcollider:
utACK 24d2d3341d
Tree-SHA512: f701f81b3aa3d3e15cee52ac9e7c31a73c0d8166e56bf077235294507cbcee099829fedc432a1c4b6d8780885f4e37897b44b980b08125771de3c849c000499e
5e146022da wallet: fix scanning progress calculation for single block range (Sebastian Falbesoner)
Pull request description:
If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the `getwalletinfo` RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes#20297.
The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via
```bash
#!/bin/bash
while true
do
bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done
```
and at the same time perform some `getwalletinfo` RPCs.
On the master branch, this leads to frequent invalid responses (tested on mainchain):
```
$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}
```
(note that missing value for "progress" in the JSON result).
On the PR branch, the behaviour doesn't occur anymore.
ACKs for top commit:
MarcoFalke:
review ACK 5e146022da
promag:
Core review ACK 5e146022da.
Tree-SHA512: f0e6aad5a6cd08b36c5fe820fff0ef26663229b39169a4dbe757f3c795a41cf5c69c9dc90efe7515675ae1059307f8971123781a0514d10704123a6f28b125ab
Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
If the blockchain is rescanned for a single block (i.e. start and stop hashes
are equal, and with that also the estimated verification progress) the progress
calculation could lead to a NaN value caused by a division by zero, resulting in
an invalid JSON result for the getwalletinfo RPC. Fixed by setting the progress
to zero in that special case.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
538be4219a wallet: fix importdescriptor silent fail (Ivan Metlushko)
Pull request description:
Currently `importdescriptor` command will successfully import a descriptor with hardened derivations into a watch-only wallet while silently failing to expand the descriptor to fill the cache. This leads to a broken wallet state and failure to load such wallet due to missing cache on subsequent restart.
ACKs for top commit:
laanwj:
Code review ACK 538be4219a
achow101:
ACK 538be4219a
meshcollider:
utACK 538be4219a
Tree-SHA512: 4bdd0ab4437d55b3f1a79c3a300a0b186089155c020fe220a73d0cce274de47d90371d88918d39fd795f9fccf8db328f1e322d29a6062f9ce94a1c254398f004
bd93fc9945 Fix change detection of imported internal descriptors (Andrew Chow)
Pull request description:
Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.
ACKs for top commit:
laanwj:
Code review ACK bd93fc9945
meshcollider:
utACK bd93fc9945
promag:
Code review ACK bd93fc9945.
Tree-SHA512: 8fa9e364be317627ec171eedffdb505976c0e7f1e55bc7e8cfdffa3aeea5db24d231f55166602cd0e97a5ba621acc871de0a765c75d0c65678f83e93c3b657c5
58cfbc38e0 Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli)
Pull request description:
I expect that there are many users with load on startup wallet definitions in `bitcoin.conf` or via startup CLI argument.
With the new `settings.json` r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate `-wallet` entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in `settings.json` due to the unload/load.
Steps to reproduce
* create wallet (if via RPC set `load_on_startup` or unloadwallet/loadwallet then set `load_on_startup`).
* stop bitcoin
* start bitcoind again with same `--wallet=mywallet`
I guess it is acceptable to skip duplicates.
ACKs for top commit:
achow101:
Tested ACK 58cfbc38e0
meshcollider:
Code review ACK 58cfbc38e0
ryanofsky:
Code review ACK 58cfbc38e0. Changes since previous review: rebased, tweaked warning message, squashed/fixed test
Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb
nWalletMaxVersion was used to allow an upgrade to a version only
when the new feature was used. This makes sense for the old
-upgradewallet startup option. But because upgradewallet is now a RPC,
putting off the version bump like this does not make sense. Instead,
immediately upgrading to the given version number makes sense.
Instead of using CanSupportFeature and relying on nWalletMaxVersion,
take the new version we are upgrading to and use IsSupportedFeature
with that and the previous wallet version.
0be29000c0 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be406 wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c005083 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa603 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1 wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f84 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1 wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c0
meshcollider:
Code review + functional test run ACK 0be29000c0
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
bbb42a6896 RPC: createwallet: Nicer error message if descriptor wallet requested and sqlite support not compiled in (Luke Dashjr)
6608fec332 GUI: Create Wallet: Nicely disable descriptor wallet checkbox if sqlite support not compiled in (Luke Dashjr)
7b54d768e1 Make sqlite support optional (compile-time) (Luke Dashjr)
Pull request description:
As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21.
Potential follow-up PRs after this:
* Make BDB support optional
* Nicer error messages when user tries to load an unsupported wallet
* Don't compile descriptor wallet code if sqlite disabled
ACKs for top commit:
jonasschnelli:
Tested ACK bbb42a6896
achow101:
ACK bbb42a6896
Sjors:
re-utACK bbb42a6896
hebasto:
ACK bbb42a6896, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
Tree-SHA512: 500209dd1971310fab8ae51543343ce0ba91f088ccccff6109b4cc27547cd5532289dca6cb7dac2a7d7c59cdf3c8f5aacc31e9b0f912e38cea52ec26b97100bd
If there is no terminating zero within the 16 magic bytes, the buffer would be
over-read in the std::string constructor. Fixed by using the "from buffer"
variant of the ctor (that also takes a size) rather than the "from c-string"
variant.
624bab00dd test: add coverage for getwalletinfo format field (Jon Atack)
5e737a0092 rpc, wallet: Expose database format in getwalletinfo (João Barbosa)
Pull request description:
Support for sqlite based wallets was added in #19077. This PR adds the `format` key in `getwalletinfo` response, that can be `bdb` or `sqlite`.
ACKs for top commit:
jonatack:
Tested ACK 624bab00dd
laanwj:
Code review ACK 624bab00dd.
MarcoFalke:
doesn't hurt ACK 624bab00dd
hebasto:
ACK 624bab00dd, tested on Linux Mint 20 (x86_64).
meshcollider:
utACK 624bab00dd
Tree-SHA512: a81f8530f040f6381d33e073a65f281993eccfa717424ab6e651c1203cbaf27794dcb7175570459e7fdaa211565bc060d0a3ecbe70d2b6f9c49b8d5071e4441c
95fedd33a2 refactor: Clean up -Wlogical-op warning (maskoficarus)
Pull request description:
This is a quick patch that fixes#19912 . This change prevents a -Wlogical-op warning that occurs because we're treating a const int value as a boolean. There's no sense checking if a non-zero constant has a value, so I've removed the check.
#18836 also addresses the same warning, but has a larger scope and will require more review. This pull request will act as a patch to prevent this compile warning until 18836 is merged.
ACKs for top commit:
MarcoFalke:
review ACK 95fedd33a2
hebasto:
ACK 95fedd33a2, tested on Linux Mint 20 (x86_64):
Tree-SHA512: 13a9d7f7cb472f4c22a01ca2f9771a75267ff769bdae9d0dc6b2c7f3b05369f6dfa859be2b172b39c15ede6c44cddf556380b3565e42850faa65ccd3fe6e175b
3333077823 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
cccc752569 rpc: Properly deserialize txs with witness before signing (MarcoFalke)
Pull request description:
Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If `try_no_witness` decoding is attempted, this will lead to rare intermittent failures.
Fixes#18803
ACKs for top commit:
achow101:
ACK 3333077823
ajtowns:
ACK 3333077823
Tree-SHA512: 73f8a5cdfe03fb0e68908d2fa09752c346406f455694a020ec0dd1267ef8f0a583b8e84063ea74aac127106dd193b72623ca6d81469a94b3f5b3c766ebf2c42b
0e2a5e448f tests: dumping and minimizing of script assets data (Pieter Wuille)
4567ba034c tests: add generic qa-asset-based script verification unit test (Pieter Wuille)
f06e6d0345 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille)
3c226639eb tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille)
206fb180ec --- [TAPROOT] Tests --- (Pieter Wuille)
d7ff237f29 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille)
e9a021d7e6 Make Taproot spends standard + policy limits (Pieter Wuille)
865d2c37e2 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille)
72422ce396 Implement Tapscript script validation rules (BIP 342) (Johnson Lau)
330de894a9 Use ScriptExecutionData to pass through annex hash (Pieter Wuille)
8bbed4b7ac Implement Taproot validation (BIP 341) (Pieter Wuille)
0664f5fe1f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille)
5de246ca81 Implement Taproot signature hashing (BIP 341) (Johnson Lau)
9eb590894f Add TaggedHash function (BIP 340) (Pieter Wuille)
450d2b2371 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille)
5d62e3a68b refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille)
8bd2b4e784 refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille)
107b57df9f scripted-diff: put ECDSA in name of signature functions (Pieter Wuille)
f8c099e220 --- [TAPROOT] Refactors --- (Pieter Wuille)
Pull request description:
This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
See the list of commits [below](https://github.com/bitcoin/bitcoin/pull/19953#issuecomment-691815830). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.
This is a successor to https://github.com/bitcoin/bitcoin/pull/17977 (see discussion following [this comment](https://github.com/bitcoin/bitcoin/pull/17977#issuecomment-682285983)), and will have further changes squashed/rebased. The history of this PR can be found in #19997.
ACKs for top commit:
instagibbs:
reACK 0e2a5e448f
benthecarman:
reACK 0e2a5e4
kallewoof:
reACK 0e2a5e448f
jonasnick:
ACK 0e2a5e448f almost only looked at bip340/libsecp related code
jonatack:
ACK 0e2a5e448f modulo the last four commits (tests) that I plan to finish reviewing tomorrow
fjahr:
reACK 0e2a5e448f
achow101:
ACK 0e2a5e448f
Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e
MakeWalletDatabase no longer has a default DatabaseFormat. Instead
callers, like CWallet::Create, need to specify the database type to
create if the file does not exist. If it exists and NONE is given, then
CreateWalletDatabase will try to autodetect the type.
Rewrite uses the VACUUM command which does exactly what we want. A
specific advertised use case is to compact a database and ensure that
any deleted data is actually deleted.
sqlite3 recommends that sqlite3_initialize be called when the
application starts, and sqlite3_shutdown when it stops. Since we don't
always use sqlite3, we initialize it when a SQLiteDatabse is constructed
(calling sqlite3_initialize after initialized is a no-op). We call
sqlite3_shutdown when we see that there are no databases opened. The
number of open databases is tracked by an atomic g_dbs_open.
We never need to open database in read-only mode as it's controlled
separately for every batch.
Also we can safely create database if it doesn't exist already
because require_existing option is verified in MakeDatabase
before creating a new WalletDatabase instance.
This adds a `TxoutType::WITNESS_V1_TAPROOT` for P2TR outputs, and permits spending
them in standardness rules. No corresponding `CTxDestination` is added for it,
as that isn't needed until we want wallet integration. The taproot validation flags
are also enabled for mempool transactions, and standardness rules are added
(stack item size limit, no annexes).
Changes the no wallet is loaded rpc error message to be clearer that no
wallet is loaded and how the user can load or create a wallet. Also
changes the error code from METHOD_NOT_FOUND to RPC_WALLET_NOT_FOUND as
that makes more sense.
f471a3be00 scripted diff: Improve invalid vout value rpc error message (Nima Yazdanmehr)
Pull request description:
Since the `vout` value can start at `0`, the error message for *negative* values can be improved to something like: `vout cannot be negative`.
ACKs for top commit:
fanquake:
ACK f471a3be00
promag:
Code review ACK f471a3be00.
Tree-SHA512: fbdee3d0ddd5b58eb93934a1217b44e125a9ad39e672b1f35c7609c6c5fcf45ae1b731d3d6135b7225d98792dbfc34a50907b8c41274a5b029d7b5c59f886560
Instead of hacking OutputGroup::m_ancestors to discourage the inclusion
of partial groups via the eligibility filter, add a parameter to the
eligibility filter that indicates whether we want to include the group.
Then for those partial groups, don't return them in GroupOutputs if we
indicate they aren't desired.
-BEGIN VERIFY SCRIPT-
r() { sed -i 's/vout must be positive/vout cannot be negative/g' $1 }
r $(git grep -l 'vout must be positive')
-END VERIFY SCRIPT-
69cf5d4eeb [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
This commit fixes#19912 by removing a check that always returned true. That check was causing a -Wlogical-op warning because it treated a constant int as though it were a boolean.
OutputGroup will handle the fee and effective value computations
inside of Insert. It now needs to take the effective feerate and long
term feerates as arguments to its constructor.
f7b331ea85 rpc: add brackets to ConstructTransaction (Sjors Provoost)
d813d26f06 [rpc] send: various touch-ups (Sjors Provoost)
0fc1c685e1 [rpc] send: fix parsing replaceable option (Sjors Provoost)
efc9b85e6f Mark send RPC experimental (Sjors Provoost)
Pull request description:
Followup based on #16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough).
I marked the RPC as experimental so we can tweak it a bit over the next release cycle.
ACKs for top commit:
meshcollider:
utACK f7b331ea85
fjahr:
utACK f7b331ea85
kallewoof:
ACK f7b331ea85
Tree-SHA512: 82dd8ac76a6558872db3f5249d4d6440469400aaa339153bc627d1ee673a91ecfadecb486bc1939ba87ebbd80e26ff29698e93e358599f3d26fde0e526892afe
72a1d5c6f3 validation: Remove review-only comments + assertions (Carl Dong)
3756853b15 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong)
485899a93c style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong)
3f5b5f3f6d validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong)
f8d4975ab3 validation: Move PruneOneBlockFile to BlockManager (Carl Dong)
74f73c783d validation: Pass in chainman to UnloadBlockIndex (Carl Dong)
4668ded6d6 validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong)
Pull request description:
This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods:
- `~CMainCleanup`
- `CChainState::FlushStateToDisk`
- `UnloadBlockIndex`
The remaining direct uses of `g_chainman` are as follows:
1. In initialization codepaths:
- `AppTests`
- `AppInitMain`
- `TestingSetup::TestingSetup`
2. `::ChainstateActive`
3. `LookupBlockIndex`
- Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR
ACKs for top commit:
MarcoFalke:
re-ACK 72a1d5c6f3👚
jnewbery:
utACK 72a1d5c6f3
Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3
fa14f57fbc Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke)
Pull request description:
This is the last part split out from #18531 to just touch some RPC methods. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tACK fa14f57fbc
ryanofsky:
Code review ACK fa14f57fbc. Just straightforward replacements except code moved in `addnode`, and displatching updated in `bumpfee_helper`
Tree-SHA512: e07af150f1d95a88e558256ce197a6b7dc6cd722a6d6c13c75d944c49c2e2441f8b8237e9f94b03db69fa18f9bda627b0781d5e1da70bf5415e09b38728a8cb1
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
d26f0648f1 Tell users how to load or create a wallet when no wallet is loaded (Andrew Chow)
1bee1e6269 Do not create default wallet (Andrew Chow)
Pull request description:
Instead of automatically creating and loading a default wallet, users should instead explicitly create their wallet or load it on start.
Builds on #19754 which provides the `load_on_startup` behavior for the GUI.
ACKs for top commit:
jnewbery:
Manual test and very light code review ACK d26f0648f1
ryanofsky:
Code review ACK d26f0648f1. Just suggested changes to first commit (reusing MakeWalletDatabase and adding release notes), no changes to second commit
jonatack:
ACK d26f0648f1 light code review, debug build, ran tests, did manual testing with testnet, rebased on master, on linux debian.
Tree-SHA512: 091d785aef64736f7df661c576e815a87f3d029cfa32f3a75ba86fc25795f10b022ab3ae15c5b61a10b8cee16f5650f15cd79cbd6127e5e3ccbef631966d3c30
