faad08e59c test: Use NodeClockContext in more tests (MarcoFalke)
fa8fe0941e fuzz: Use NodeClockContext (MarcoFalke)
fa9f434df8 test: Allow time_point in boost checks (MarcoFalke)
Pull request description:
Currently mocktime is written to a global, which may leak between sub-tests (albeit some tests try to reset the mocktime on a best-effort basis). Also, when advancing it, one has to keep a counter variable around.
Fix both issues by using the recently added `NodeClockContext`, which resets the mocktime once it goes out of scope. Also, it has a method to advance the mocktime by a delta.
ACKs for top commit:
achow101:
ACK faad08e59c
seduless:
Tested ACK faad08e59c
frankomosh:
Tested ACK faad08e59c. Ran all relevant tests, all clean. Also verified that the default-constructor call sites in orphanage_tests and addrman_tests behave identically to the explicit `{Now<NodeSeconds>()}` form.
ryanofsky:
Code review ACK faad08e59c but had a question about dropping +1 in one test below.
Tree-SHA512: bd56931970eed02bfcf3f3593ef64a61a8a1d8cc8adf190d6903b35df0fd7e6d865678c7d5bd23ce53d074cb2cf53a0a19212fdeb593b047dac5561859bc86b0
858a0a9c96 test: Add SRD maximum weight tests (Murch)
fe9f53bf0b test: Add SRD success tests (Murch)
2840f041c5 test: Rework SRD insufficient balance test (Murch)
64ab97466f Test: Add new minimum to tested feerates (Murch)
65900f8dc6 test: Init coin selection params with feerate (Murch)
Pull request description:
This transfers the tests from the old coin selection test framework that was based on ignoring fees to the new coin selection framework that tests the coin selection algorithms with realistic feerates and fees.
The PR also includes a minor improvement of the test framework to allow the CoinSelectionParams used by the tests to be initialized with other feerates, and adds some feerates around the new minimum feerate to the tested feerates.
ACKs for top commit:
achow101:
ACK 858a0a9c96
w0xlt:
ACK 858a0a9c96
brunoerg:
reACK 858a0a9c96
Tree-SHA512: cbd7ed3169d225a0b0f751481ca936a10939ff899c345e9469821d46083b04e835d164c50a72f18851544314611c3d596eb4956c1d86903c22e8b4996b8eb861
Also:
- Add weight check to Success cases.
Per the introduction of TRUC transactions, it is more likely that we
will attempt to build transactions of limited weight (e.g., TRUC child
transactions may not exceed 4000 WU). When SRD exceeds the input weight
limit, it evicts the OutputGroup with the lowest effective value before
selecting additional UTXOs: we test that SRD will find a solution that
depends on the eviction working correctly, and that it fails as expected
when no solution is possible.
Previously, SRD had only failure and edge-case coverage, so we add a few
simple coin selection targets that should quickly succeed, including one
that will create a minimal change output.
This refactor is part of the effort to move the coin selection tests to
a framework that can use non-zero, realistic feerates. The insufficient
funds failure case is extended with a few additional similar variants of
the failure.
The default minimum feerate was lowered from 1000 s/kvB to 100 s/kvB.
This adjusts the set of feerates used in the tests to accommodate that
new feerate and to cover other potential special cases:
- zero: 0 s/kvB
- minimum non-zero s/kvB: 1 s/kvB
- just below the new default minimum feerate: 99 s/kvB
- new default minimum feerate: 100 s/kvB
- old default minimum feerate: 1000 s/kvB
- a few non-round realistic feerates around default minimum feerate,
dust feerate, and default LTFRE: 315 s/kvB, 2345 s/kvB, and
10'292 s/kvB
- a high feerate that has been exceeded occassionally: 59'764 s/kvB
- a huge feerate that is extremely uncommon: 1'500'000 s/kvB
The most frequent change to CoinSelectionParams so far seems to be
amendments to the feerate, and the feerate propagates to other members
of the CoinSelectionParams, so we add it as an input parameter.
This also adds the CoinSelectionParams to the BnBFail tests, which
previously were always run with the default CoinSelectionParams.
These are unused and removing them avoids clang warnings like:
src/test/fuzz/deserialize.cpp:42:26: error: variable g_setup set but not used [-Werror,-Wunused-but-set-variable]
This refactor is a follow-up to commit
eeeeb2a0b9 and does not change any
behavior.
However, it is nice to know that no global mocktime leaks from the fuzz
init step to the first fuzz input, or from one fuzz input execution to
the next.
With the clock context, the global is re-set at the end of the context.
779e7825db fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg)
Pull request description:
This PR adds fuzz coverage for the scriptpubkeyman migration (`MigrateToDescriptor`). Note that it's a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:
1) The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s);
2) Mocking would require lots of refactors.
This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls `MigrateToDescriptor`. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for https://github.com/bitcoin/bitcoin/pull/31452, for example.
ACKs for top commit:
frankomosh:
Code Review ACK 779e7825db
marcofleon:
reACK 779e7825db
Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e
This refactor does not change any behavior.
However, it is nice to know that no global mocktime leaks from the fuzz
init step to the first fuzz input, or from one fuzz input execution to
the next.
With the clock context, the global is re-set at the end of the context.
a067ca3410 [doc] coin selection filters by max cluster count, not descendant (glozow)
f7be5fb8fc [refactor] rename variable to clarify it is unused and cluster count (glozow)
Pull request description:
Followup to #33629.
Fix misleading docs and variable names. Namely, `getTransactionAncestry` returns the cluster count, not max descendant count of ancestor set (not worth reimplementing as it is merely a heuristic). No behavior changes - I don’t think much needs to be changed for the first release containing cluster mempool.
Current `CoinEligibilityFilter`s enforce a maximum ancestor count (summed across all outputs, potentially overestimating) and max descendant count across ancestors of the output.
Since #33629, these filters have started using cluster count instead of max desc count across ancestors. The change isn’t dangerous, as the cluster count bounds descendant count as well. Currently, the wallet is essentially enforcing a mixture of both limits - this is good while we are transitioning. Note that the cluster count enforced is 25, not 64, since it's grabbing the node's descendant count limit. While it is not an apples-to-apples comparison, a cluster count limit of 25 helps us avoid busting legacy descendant limits (which will be common on the network for a while).
Potential things for the future, out of scope for this PR:
- When we get rid of the ancestor/descendant config options, `getPackageLimits` can probably be replaced with hard-coded values.
- Change the `OutputGroup`s to track the actual cluster count that results from spending these outputs and merging their clusters.
- Loosen from 25 after that policy is no longer common.
- Clean up `getPackageLimits`.
ACKs for top commit:
achow101:
ACK a067ca3410
ismaelsadeeq:
reACK a067ca3410
rkrux:
crACK a067ca3410
Tree-SHA512: d7cacd5bf668d42e26e8b83e42a42c280929c3bfd554c3db1de605e5939f8b36c14ecfd2839abeb4eec352363df1891b3420a693c250916391ab10a5ce26396b
Move the operator<< overloads used by BOOST_CHECK_* out of the
unit test machinery test/setup_common, into test/util/common.h.
And replace the individual per-type ToString() overloads with
a single concept-constrained template that covers any type
exposing a ToString() method. This is important to not add
uint256.h and transaction_identifier.h dependencies to the
shared test/util/common.h file.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
4b53cbd692 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15c miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fb descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd692
sipa:
ACK 4b53cbd692
darosior:
Other than that, Approach ACK 4b53cbd692. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
24f93c9af7 release note (Pol Espinasa)
331a5279d2 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7
w0xlt:
reACK 24f93c9af7
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.
CoinsResult has a cleaner interface with separate fields
for both these amounts.
2 behavioural changes:
1. no more default assert error if effective value is unset
- previously PreSelectedInputs::Insert() called
COutput::GetEffectiveValue() which assert failed
if the optional was unset.
- now we don't default assert anymore.
* in GUI/getAvailableBalance better not to assert.
* SelectCoins's preselected inputs always contain a
feerate, so effective amount should be set.
explicitly added an assertion to ensure this.
2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
populate CoinsResult's coins map. it's discarded later.
before #25806, set<COutput> was used and would not
contain same COutputs in the set.
now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.
so preserve previous behaviour by making sure values
in the set are also distinct
54d0393058 FUZZ: Test that BnB finds best solution (Murch)
Pull request description:
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
ACKs for top commit:
achow101:
ACK 54d0393058
brunoerg:
ACK 54d0393058
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
BnB’s solution is the input set with the lowest waste score, excluding
any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to
ensure that BnB succeeds.
db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
370c592612/src/wallet/wallet.cpp (L2882-L2885)
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
b189a34557 test: add case where `TOTAL_TRIES` is exceeded yet solution remains (yancy)
Pull request description:
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently named `bnb_test`.
ACKs for top commit:
frankomosh:
Code review ACK b189a34
achow101:
ACK b189a34557
murchandamus:
ACK b189a34557
Tree-SHA512: 1df0b6e29ae219edbeed14cfa97f0ad4688d6bf97ed946719ba3c3b69e004f3dee82991578eb5aceb554914b70c5b68feff9e321283c1fc8bc0fedf08df2cb4c
fab2f3df4b fuzz: Exclude too expensive inputs in descriptor_parse targets (MarcoFalke)
Pull request description:
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example, those will take several seconds (!) and the flamegraph shows that base58 encoding is the cause:
```
curl -fLO 'f5abf41608'
curl -fLO '78cb317546'
FUZZ=mocked_descriptor_parse ./bld-cmake/bin/fuzz ./f5abf41608addcef3538da61d8096c2050235032
FUZZ=descriptor_parse ./bld-cmake/bin/fuzz ./78cb3175467f53b467b949883ee6072e92dbb267
```
This will also break 32-bit fuzzing, see https://github.com/bitcoin/bitcoin/issues/34110#issuecomment-3759461248.
Fix all issues by checking for `HasTooLargeLeafSize`.
Sorry for creating several pull requests to fix this class of issue, but I think this one should be the last one. 😅
ACKs for top commit:
brunoerg:
reACK fab2f3df4b
frankomosh:
re-ACK fab2f3df4b
Tree-SHA512: 4ecf98ec4adc39f6e014370945fb1598cdd3ceba60f7209b00789ac1164b6d20e82a69d71f8419d9a40d57ee3fea36ef593c47fe48b584b6e8344c44f20a15c1
Aside from being more legible, changing the name of `CWallet::Create()`
also validates that every instance where a new wallet is `Create()`'ed
is handled in this branch.
-BEGIN VERIFY SCRIPT-
sed -i 's|\bCreate(|CreateNew(|g' src/wallet/wallet.cpp src/wallet/wallet.h src/wallet/test/util.cpp src/wallet/test/wallet_tests.cpp
-END VERIFY SCRIPT-
faa59b3679 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3 util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430 util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac4800959 util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2 util: Make Expected::value() throw (MarcoFalke)
fa1de1103f util: Add Unexpected::error() (MarcoFalke)
faa109f8be test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b3679
ryanofsky:
Code review ACK faa59b3679. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b3679
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
9c7e4771b1 test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a6854 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f1 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb descriptors: add HavePrivateKeys() (Novo)
Pull request description:
_TLDR:
Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possible to do so.
This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.
### Notes
- The new behaviour is applied to all descriptors including miniscript descriptors
- `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour https://github.com/bitcoin/bitcoin/pull/24361#discussion_r920801352
- Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.
### Relevant PRs
https://github.com/bitcoin/bitcoin/pull/24361https://github.com/bitcoin/bitcoin/pull/32186
### Testing
Functional tests were added to test the new behaviour
EDIT
**`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**
ACKs for top commit:
Sjors:
ACK 9c7e4771b1
achow101:
ACK 9c7e4771b1
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b1
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
There are too many functions in CWallet with names like "Load" and
"Create", disambiguate what CWallet::LoadWallet does by renaming it to
PopulateWalletFromDB.
-BEGIN VERIFY SCRIPT-
sed -i 's|\bLoadWallet()|PopulateWalletFromDB()|g' $(git grep -l 'LoadWallet()' -- ':(exclude)src/wallet/walletdb.cpp')
-END VERIFY SCRIPT-
Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used.
It returns `false` if there is at least one pubkey in the descriptor for which
the provider does not have a private key.
ToPrivateString() behaviour will change in the following commits to only
return `false` if no priv keys could be found for the pub keys in the descriptor.
HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining
if a descriptor is 'watchonly'.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff80
rkrux:
lgtm re-ACK 76c092ff80
brunoerg:
reACK 76c092ff80
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
fa4cb13b52 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f297748 scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52
rkrux:
re-ACK fa4cb13b52
janb84:
ACK fa4cb13b52
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* other bare `std::unordered_set` and `std::map` count calls.
With no remaining violations, enable the `readability-container-contains`
clang-tidy check to prevent future regressions.
d9319b06cf refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554e refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b63 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf
sedited:
ACK d9319b06cf
janb84:
re ACK d9319b06cf
pablomartin4btc:
re-ACK d9319b06cf
ryanofsky:
Code review ACK d9319b06cf. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
BIP 379 allows height and time locks that have no consensus meaning in BIP 68 / BIP 112.
This is used by some protocols like Lightning to encode extra data, but is unsafe when
used unintentionally. E.g. older(65536) is equivalent to older(1).
This commit emits a warning when importing such a descriptor.
It introduces a helper ForEachNode to traverse all miniscript nodes.
Show that `CoinGrider` halts searching when the number of attempts exceeds
`TOTAL_TRIES`. To do so, show that a solution is found, then add one
more entry to the same set of inputs. Since the search orders by
`effective_value`, the solution is constructed such that only values
with the lowest `effective_value` have the least weight. Only the
lowest weight values will not exceed the `max_selection_weight`.
Therefore, `CoinGrinder` will not evaluate all lowest weight solutions
together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently
named `bnb_test`.
