| Age | Commit message (Collapse) | Author |
|---|
|
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
|
|
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
3ee6d0788ec1b90f7c39c9644dba4011f7cf5db4 test: add more wallet conflicts assertions (S3RK)
3b98bf9c43ece060d57d7ae31624d4a8220de266 Revert "Add to spends only transcations from me" (S3RK)
Pull request description:
This reverts commit d04566415e16ae685af066384f346dff522c068f from #22929.
This commit was based on invalid assumption that `mapTxSpends` should contain only outgoing txs and broke wallet conflicts feature.
ACKs for top commit:
achow101:
ACK 3ee6d0788ec1b90f7c39c9644dba4011f7cf5db4
Tree-SHA512: bf5a77ced6bac57d5eb85771d9189c53e1edc295d179ed5a1bdce18e365794a9101b4cecf35387b27f67260db3b47f7214e7876e490494529b748cceeb95632d
|
|
|
|
If the user specifies an input size, allow it to override any input size
calculations during coin selection.
|
|
This reverts commit d04566415e16ae685af066384f346dff522c068f.
|
|
|
|
|
|
Needed for a later commit
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
|
|
encapsulating a coin selection solution
05300c14392facf38330eb4fcd8e695a838b76f3 Use SelectionResult in SelectCoins (Andrew Chow)
9d9b101d2019d8237546eedd022e74519feb07bb Use SelectionResult in AttemptSelection (Andrew Chow)
bb50850a447bdf461ffb76d47d4a4db904fce324 Use SelectionResult for waste calculation (Andrew Chow)
e8f7ae5eb3c682d1a80b503f71e06ce76af1b65c Make an OutputGroup for preset inputs (Andrew Chow)
51a9c00b4de707e0a6a1a68ca6f8e38d86c72d94 Return SelectionResult from SelectCoinsSRD (Andrew Chow)
0ef6184575e77b17f5ec6d7ca086900aca79f6d7 Return SelectionResult from KnapsackSolver (Andrew Chow)
60d2ca72e3f4c56433c63b929a88e7a2def06399 Return SelectionResult from SelectCoinsBnB (Andrew Chow)
a339add471717623915cd1a846ade4dab2c89deb Make member variables of SelectionResult private (Andrew Chow)
cbf0b9f4ff438865a71c7ceb0a543c18a34f41f0 scripted-diff: Use SelectionResult in coin selector tests (Andrew Chow)
9d1d86da04d5d4768975338841285e90b01130b8 Introduce SelectionResult struct (Andrew Chow)
94d851d28cb909a8f1f8ab795f1d9fc74bebfc7f Fix bnb_search_test to use set equivalence for (Andrew Chow)
Pull request description:
Instead of returning a set of selected coins and their total value as separate items, encapsulate both of these, and other variables, into a new `SelectionResult` struct. This allows us to have all of the things relevant to a coin selection solution be in a single object. `SelectionResult` enables us to implement the waste calculation in a cleaner way.
All of the coin selection functions (`SelectCoinsBnB`, `KnapsackSolver`, `AttemptSelection`, and `SelectCoins`) are changed to use a `SelectionResult` as the output parameter.
Based on #22009
ACKs for top commit:
laanwj:
Code review ACK 05300c14392facf38330eb4fcd8e695a838b76f3
Tree-SHA512: e4dbb4d78a6cda9c237d230b19e7265591efac5a101a64e6970f0654e2c4f93d13bb5d07b98e8c7b8d37321753dbfc94c28c3a7810cb1c59b5bc29b08a8493ef
|
|
This removes the ability to set an offset in the SpanReader constructor,
as the current code is broken. All call sites use pos=0, so it is actually
unused. If future call sites need it, SpanReader{a, b, c, d} is equivalent
to SpanReader{a, b, c.subspan(d)}.
It also removes the ability to deserialize from SpanReader directly from
the constructor. This too is unused, and can be more idiomatically
simulated using (SpanReader{a, b, c} >> x >> y >> z) instead of
SpanReader{a, b, c, x, y, z}.
|
|
Replace setCoinsRet and nValueRet with SelectionResult
|
|
Returns a std::optional<SelectionResult> from KnapsackSolver instead of
using out parameters for the inputs set and selected value.
|
|
Removes coins_out and value_ret has SelectCoinsBnB return a
std::optional<SelectionResult>
|
|
|
|
Replace the CoinSet actual_selection with a SelectionResult
expected_result. We don't use the SelectionResult functions yet, but
will soon.
-BEGIN VERIFY SCRIPT-
sed -i 's/CoinSet actual_selection/SelectionResult expected_result(CAmount(0))/' src/wallet/test/coinselector_tests.cpp
sed -i 's/actual_selection/expected_result.m_selected_inputs/' src/wallet/test/coinselector_tests.cpp
sed -i 's/expected_result.m_selected_inputs.clear/expected_result.Clear/' src/wallet/test/coinselector_tests.cpp
-END VERIFY SCRIPT-
|
|
2c35a93b3cc19dc71d5664f9f61c24a04f419e35 Generalize/simplify VectorReader into SpanReader (Pieter Wuille)
Pull request description:
Originally written for #21590 (safegcd-based MuHash inverses), but then found a better way that removed the need for it, so I'm submitting it independently.
ACKs for top commit:
MarcoFalke:
re-ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35 🖨
shaavan:
ACK 2c35a93b3cc19dc71d5664f9f61c24a04f419e35
Tree-SHA512: 959e3251e0cfe20e13a50639b617c9dc2a561d613a0884d983c93d15dacb6d2305d760aa933d18ba055cef8a1651a344bcb6b3f93051ecf26d3f2efc5779efa4
|
|
|
|
destinations to the address book
3d71d16d1eb4173c70d4c294559fc2365e189856 test: listtranscations with externally generated addresses (S3RK)
d04566415e16ae685af066384f346dff522c068f Add to spends only transcations from me (S3RK)
9f3a622b1cea37e452560f2f82d8e82d3b48a73a Automatically add labels to detected receiving addresses (S3RK)
c1b99c088c54eb101c0a28a67237965576ccf5ad Return used destinations from ScriptPubKeyMan::MarkUnusedAddresses (S3RK)
03840c20640685295a65ed8c82456e877f668b9b Add CWallet::IsInternalScriptPubKeyMan (S3RK)
456e350926adde5dabdbc85fc0f017fb29bdadb3 wallet: resolve ambiguity of two ScriptPubKey managers providing same script (S3RK)
Pull request description:
This PR fixes certain use-cases when **send-to-self** transactions are missing from `listtransactions` output.
1. When a receiving address is generated externally to the wallet
(e.g. same wallet running on two nodes, or by 3rd party from xpub)
2. When restoring backup with lost metadata, but keypool gap is not exceeded yet
When the block is connected or tx added to mempool we already mark used keys. This PR extends this logic to determine whether the destination is a receiving one and if yes add it to the address book with empty label.
Works both for legacy and descriptors wallets.
- For legacy it uses the internal flag from the keypool entry. Caveat: because we don't know which script type would be used we add all possible destinations for such keys.
- For descriptor wallets it uses internal flag for the script pub key manager. Caveat: it only works for active descriptors.
fixes #19856
fixes #20293
ACKs for top commit:
laanwj:
Code review ACK 3d71d16d1eb4173c70d4c294559fc2365e189856
Tree-SHA512: 03fafd5548ead0c4ffe9ebcc9eb2849f1d2fa7270fda4166419b86877d4e57dcf04460e465fbb9c90b42031f3c05d1b83f1b67a9f82c2a42980825ed1e7b52e6
|
|
named arguments (tests only)
fa00447442f22a24e5ca5fc538d0bf7bef575544 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke)
fae13c39896898aef2281433af143c22d8b3a3b4 doc: Use clang-tidy comments in crypto_tests (MarcoFalke)
Pull request description:
Incorrect named args are source of bugs, like #22979.
To allow them being checked by `clang-tidy`, use a format it can understand.
ACKs for top commit:
shaavan:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
rajarshimaitra:
ACK https://github.com/bitcoin/bitcoin/pull/23546/commits/fa00447442f22a24e5ca5fc538d0bf7bef575544
jonatack:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
fanquake:
ACK fa00447442f22a24e5ca5fc538d0bf7bef575544
Tree-SHA512: 4d23a8363da81dfea21a4cd8516ab5e0dc70119e4d503f3f240f38573218b2c2e84083b97e956c62942d78b2f17490f8b3b2e8077d257644fda1d901e2b80507
|
|
f13a22a631efe01e1fbae4ae78a4901d14ebda3c wallet: Call load handlers without cs_wallet locked (João Barbosa)
Pull request description:
Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
ACKs for top commit:
meshcollider:
re-utACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
ryanofsky:
Code review ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c. Only change since last review is adding a lock order comment
jonatack:
ACK f13a22a631efe01e1fbae4ae78a4901d14ebda3c
Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
|
|
d8ee8f3cd32bbfefec931724f5798cbb088ceb6f refactor: Make CWalletTx sync state type-safe (Russell Yanofsky)
Pull request description:
Current `CWalletTx` state representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form.
For example, it is possible to call `setConflicted` without setting a conflicting block hash, or `setConfirmed` with no transaction index. And it's possible update individual `m_confirm` and `fInMempool` data fields without setting an overall consistent state that can be serialized and handled correctly.
Fix this without changing behavior by using `std::variant`, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible.
This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.
ACKs for top commit:
laanwj:
re-ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f
jonatack:
Code review ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f
Tree-SHA512: b9f15e9d99dbdbdd3ef7a76764e11f66949f50e6227e284126f209e4cb106af6d55e9a9e8c7d4aa216ddc92c6d5acc6f4aa4746f209bbd77f03831b51a2841c3
|
|
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
|
-BEGIN VERIFY SCRIPT-
perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/test ./src/wallet/test )
-END VERIFY SCRIPT-
|
|
|
|
Current CWalletTx state representation makes it possible to set
inconsistent states that won't be handled correctly by wallet sync code
or serialized & deserialized back into the same form.
For example, it is possible to call setConflicted without setting a
conflicting block hash, or setConfirmed with no transaction index. And
it's possible update individual m_confirm and fInMempool data fields
without setting an overall consistent state that can be serialized and
handled correctly.
Fix this without changing behavior by using std::variant, instead of an
enum and collection of fields, to represent sync state, so state
tracking code is safer and more legible.
This is a first step to fixing state tracking bugs
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
by adding an extra margin of safety that can prevent new bugs from being
introduced as existing bugs are fixed.
|
|
|
|
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
|
11115169a14d0d0be5b7b1c3f6fdc9673a9098d9 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca66627e4c76adecc824f96da220af69c fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b8d6fe8c610f170a13675c756aef58f refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f26a80daf4320eee046c9572e85fe3e build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)
Pull request description:
Initial sketch to fuzz descriptor wallets. Can be improved in the future.
ACKs for top commit:
mjdietzx:
Code review ACK 1111516
Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
|
|
|
|
The bug was introduced in dcd6eeb64adb2b532f5003cbb86ba65b3c08a87b.
|
|
54011e7aa274bdc1b921440cc8b4623aa1e0d89e refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2de737151bc4fb216221bf49cb53ce6 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa274bdc1b921440cc8b4623aa1e0d89e
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
|
|
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
|
|
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
|
|
For BnB, we only want to check that sets are equivalent with their
values, whereas in knapsack we care about the outpoints.
|
|
|
|
For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
still some wallet tests that test legacy wallet things. Those remain
unchanged.
|
|
The subtract fee from outputs assumes that the leftover input amount
will be dropped to fees. However this only happens if that amount is
less than the cost of change. In the event that it is higher than the
cost of change, the leftover amount will actually become a change
output. To avoid this scenario, force a change type which has a high
cost of change.
|
|
|
|
|
|
To avoid issues with test data leaking across tests cases, the global
vCoins and testWallet are removed from coinselector_tests and all of the
relevant functions reworked to not need them.
|
|
|
|
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
|
|
9d0379cea6c164610d05287ae6dd4e66f35b92b3 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63a67fa020fb1ef527b9095a35ab77a5 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5bc997f2de1f55ca7a9babc3d7619329 [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6c164610d05287ae6dd4e66f35b92b 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
|
|
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
|
|
|
|
There are two more cases where waste can be 0, when:
- (Fee - LTF) == -Change Cost
- (Fee - LTF) == -Excess
Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
|
|
|
