fd5c996d1609e6f88769f6f3ef0c322e3435b3aa wallet: GetAvailableBalance, remove double walk-through every available coin (furszy)
162d4ad10f28c5fa38551d69ce9b296ab3933c77 wallet: add 'only_spendable' filter to AvailableCoins (furszy)
cdf185ccfb2085e5a4bf82d833392d74b748aeff wallet: remove unused IsSpentKey(hash, index) method (furszy)
4b83bf8dbcf6b8b1c1293575391e90ac7e21b0e0 wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy)
3d8a2822570e3cf4d1bc4f9d59b5dcb0145920ad wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy)
a06fa94ff81e2bccef0316ea5ec4eca0f4de5071 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy)
91902b77202fc636edb3db587cb6e87d9fb9b60a wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy)
9472ca0a65396206b3078bddf98f4c1807be2d82 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy)
4ce235ef8f9a9dddc52d7ab60c8f71bda1d38873 wallet: return 'CoinsResult' struct in `AvailableCoins` (furszy)
Pull request description:
This started in #24845 but grew out of scope of it.
So, points tackled:
1) Avoid extra `GetWalletTx` lookups inside `AvailableCoins -> IsSpentKey`.
`IsSpentKey` was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the `IsSpentKey` function callers already have the wtx available, them can provide the `scriptPubKey` directly.
2) Most of the time, we call `Wallet::AvailableCoins`, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside `Wallet::AvailableCoins` directly.
(the non-spendable coins skip examples are inside `AttemptSelection->GroupOutputs` and `GetAvailableBalance`).
4) Refactored `AvailableCoins` in several ways:
a) Now it will return a new struct `CoinsResult` instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer.
b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment https://github.com/bitcoin/bitcoin/pull/24699#discussion_r854163032).
5) The wallet `IsLockedCoin` and `IsSpent` methods now accept an `OutPoint` instead of a hash:index. Which let me cleanup a bunch of extra code.
6) Speeded up the wallet 'GetAvailableBalance': filtering `AvailableCoins` by spendable outputs only and using the 'AvailableCoins' retrieved `total_amount` instead of looping over all the retrieved coins once more.
-------------------------------------------------------
Side topic, all this process will look even nicer with #25218
ACKs for top commit:
achow101:
ACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa
brunoerg:
crACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa
w0xlt:
Code Review ACK fd5c996d16
Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220
Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.
Note:
This new struct, down the commits line, will contain other `AvailableCoins` useful results.
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
3f8def51d53a078a5ee71ec675b5e06b784147de add 3 new test cases for SelectCoins() (akankshakashyap)
Pull request description:
Three new tests have been added.
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
ACKs for top commit:
achow101:
ACK 3f8def51d53a078a5ee71ec675b5e06b784147de
brunoerg:
ACK 3f8def51d53a078a5ee71ec675b5e06b784147de
Tree-SHA512: 8db6dd942b02a38c99953b801605f98c4c17729768fdfcf7605c5bbdb17509500a39d0a78a4b19aab37812d2994ec7630d2b4e78d1d348f1c27b67588d74e155
f64aa9c411ad78259756a28756ec1eb8069b5ab4 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
ab5af9ca7293239ffc24ea7e23159b8184543f94 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a7147f80cbe84b0742908b0b0faa04d doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa2d8c63bc697c9ebd303965fcccea55 wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0029c4ae7b487e03cd451e1580eb91d wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181161b0365776cd490b63137aaad708a wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca7293239ffc24ea7e23159b8184543f94
josibake:
crACK ab5af9ca72
0xB10C:
ACK ab5af9ca7293239ffc24ea7e23159b8184543f94. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
3ae7791bcaa88f5c68592673b8926ee807242ce7 refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bcaa88f5c68592673b8926ee807242ce7
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
21520b95515676d45145df624f430cdd39db7515 fuzz: add target for coinselection (Martin Zumsande)
Pull request description:
This adds a fuzz target for the coinselection algorithms by creating random `OutputGroup`s and running all three coin selection algorithms for them.
It does not fuzz higher-level wallet logic for selecting eligible coins (as in `SelectCoins()`), thought it probably would make sense to have a fuzz target for that too.
ACKs for top commit:
achow101:
ACK 21520b95515676d45145df624f430cdd39db7515
vasild:
ACK 21520b95515676d45145df624f430cdd39db7515
Tree-SHA512: c763003cf5ff5317f929d3d0b2f06fa739ae41dd642042d9a5c5c96e6cb9b349a6c7aeabc77bc2b846d12c8bcb60e07ee20a9f38539429c65723ab76aeee6b2e
0c12f0116ca802f55f5ab43e6c4842ac403b9889 wallet: Postpone NotifyWalletLoaded() for encrypted wallets (Hennadii Stepanov)
aeee419c6aae085cacd75343c1ce23486b2b8916 wallet, refactor: Add wallet::NotifyWalletLoaded() function (Hennadii Stepanov)
Pull request description:
Fixesbitcoin-core/gui#571.
`CWallet::Create()` notifies about wallet loading too early, that results the notification goes before `DescriptorScriptPubKeyMan`s were created and added to an encrypted wallet.
And `interfaces::Wallet::taprootEnabled()` in ecf692b466/src/qt/receivecoinsdialog.cpp (L100-L102) erroneously returns `false` for just created encrypted descriptor wallets.
ACKs for top commit:
Sjors:
utACK 0c12f0116ca802f55f5ab43e6c4842ac403b9889
achow101:
ACK 0c12f0116ca802f55f5ab43e6c4842ac403b9889
Tree-SHA512: 2694bacd12748cd5f6c95d9d3bf8bcf4502ee67fecd8d057f33236b72069c61401b08f49deb013fc71c3f1e51ae16bdfd827ddcbc2a083d7044589be7a78982e
2ef47ba6c57a12840499a13908ab61aefca6cb55 util/check: stop using lambda for Assert/Assume (Anthony Towns)
7c9fe25c16d48b53a61fa2f6ff77eaf8820cb1f6 wallet: move Assert() check into constructor (Anthony Towns)
Pull request description:
Using a lambda creates a couple of odd namespacing issues, in particular making clang's thread safety analysis less helpful, and confusing gcc when calling member functions. Fix this by not using a lambda.
Fixes#21596Fixes#24654
ACKs for top commit:
MarcoFalke:
ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55 🚢
jonatack:
Tested re-ACK 2ef47ba6c57a12840499a13908ab61aefca6cb55
Tree-SHA512: 4bdbf3215f3d14472df0552362c5eebe8b7eea2d0928a8a41109edd4e0c5f95de6f8220eb2fee8506874e352c003907faf5ef344174795939306a618157b1bae
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
049003fe68a4183f6f20da16f58f10079d1e02df coinselection: Remove COutput operators == and != (Andrew Chow)
f6c39c6adb6cbf9c87f04d3d667701905ef5c0a0 coinselection: Remove CInputCoin (Andrew Chow)
70f31f1a81710aa59e95770de9a84bf58cbce1e8 coinselection: Use COutput instead of CInputCoin (Andrew Chow)
14fbb57b79c664090f6a4e60d7bdfc9759ff4307 coinselection: Add effective value and fees to COutput (Andrew Chow)
f0821230b8de2eec21a869d1edf9e2b9f502de25 moveonly: move COutput to coinselection.h (Andrew Chow)
42e974e15c6deba1d9395a4da9341c9ebec6e8e5 wallet: Remove CWallet and CWalletTx from COutput's constructor (Andrew Chow)
14d04d5ad15ae56df56edee7ca9a202b52037889 wallet: Replace CWalletTx in COutput with COutPoint and CTxOut (Andrew Chow)
0ba4d1916e26e2a5d603edcdb7625463989d25b6 wallet: Provide input bytes to COutput (Andrew Chow)
d51f27d3bb0d6e3ca55bcd23ce53e4fe413a9360 wallet: Store whether a COutput is from the wallet (Andrew Chow)
b799814bbd53736b79495072f3c9e05989a465e8 wallet: Store tx time in COutput (Andrew Chow)
46022953ee2e8113167bafd1fd48a383a578b13c wallet: Remove use_max_sig default value (Andrew Chow)
10379f007fd2c18f4cd24d0a0783d6d929f45556 scripted-diff: Rename COutput member variables (Andrew Chow)
c7c64db41e1718584aa2f30ff27f60ab0966de62 wallet: cleanup COutput constructor (Andrew Chow)
Pull request description:
While working on coin selection code, it occurred to me that `CInputCoin` is really a subset of `COutput` and the conversion of a `COutput` to a `CInputCoin` does not appear to be all that useful. So this PR adds fields that are present in `CInputCoin` to `COutput` and replaces the usage of `CInputCoin` with `COutput`.
`COutput` is also moved to coinselection.h. As part of this move, the usage of `CWalletTx` is removed from `COutput`. It is instead replaced by storing a `COutPoint` and the `CTxOut` rather than the entire `CWalletTx` as coin selection does not really need the full `CWalletTx`. The `CWalletTx` was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to `COutput`'s constructor.
ACKs for top commit:
ryanofsky:
Code review ACK 049003fe68a4183f6f20da16f58f10079d1e02df, just adding comments and removing == operators since last review
w0xlt:
reACK 049003f
Xekyo:
reACK 049003fe68a4183f6f20da16f58f10079d1e02df
Tree-SHA512: 048b4cd620a0415e1d9fe8597257ee4bc64656566e1d28a9bdd147d6d72dc87c3f34a3339fa9ab6acf42c388df7901fc4ee900ccaabc3de790ffad162b544c15
39b1763730177cd7d6a32fd9321da640b0d65e0e Replace use of `ArgsManager` with `DatabaseOptions` (Kiminuo)
Pull request description:
Contributes to #21005.
The goal of this PR is to remove `gArgs` from database classes (i.e. `bdb.h` and `sqlite.h`) so that they can be tested without relying on `gArgs` in tests.
Notes:
* My goal is to enable unit-testing without relying on `gArgs` as much as possible. Global variables are hard to reason about which in turn makes it slightly harder to contribute to this codebase. When the compiler does the heavy lifting for us and allows us only to construct an object (or call a method) with valid parameters, we may also save some time in code reviews. The cost for this is passing an argument which is not for free but the cost is very miniscule compared to benefits, I think.
* GUI code is an exception because it seems fine to have `gArgs` there so I don't plan to make changes in `src/qt` folder, for example.
* My approach to removal of `gArgs` uses is moving from lower levels to upper ones and pass `ArgsManager` as an argument as needed. The approach is very similar to what #20158.
ACKs for top commit:
achow101:
ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e
ryanofsky:
Code review ACK 39b1763730177cd7d6a32fd9321da640b0d65e0e. Just the two small ReadDatabaseArgs and Berkeley open changes that were discussed since the last review
Tree-SHA512: aa066b314db593e46c18698fe8cdd500f558b405dc04e4a9a3ff57b52b5b3a81a6cb090e0e661785d1d02c1bf18958c1f4cd715ff233aab63381e3f80960622d
These operators are used only by the tests in std::mismatch. As
std::mismatch can take a binary predicate, we can use a lambda that
achieves the same instead.
Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.
fa7deaa0464576a229b5a6ab13ad033c16d0dada wallet: Pass FastRandomContext& to coin selection (MarcoFalke)
77773b061cb13229a8afb46f6f3ab89fc70eabe3 wallet: Pass FastRandomContext& to DiscourageFeeSniping (MarcoFalke)
Pull request description:
Passing around a single randomness context shouldn't come with any downsides, but documents better where randomness is used and allows the unit test to be deterministic, if they wish to be so.
ACKs for top commit:
achow101:
ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada
promag:
Code review ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada.
glozow:
light code review ACK fa7deaa0464576a229b5a6ab13ad033c16d0dada
Tree-SHA512: c16287708cc82ce58311710595d0127af42fb156c93fbcaa5bde634ce323d325f4d8c99a74af24423ab22b5ad58163dd771e8b1a0e7d6bff39c9fb2a1cb21bc7
Instead of determining whether the containing transaction is from the
wallet dynamically as needed, just pass it in to COutput and store it.
The transaction ownership isn't going to change.
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
A change to BlockManager::LookupBlockIndex:
- Previously, it was a const member function returning a non-const CBlockIndex*
- Now, there's are const and non-const versions of
BlockManager::LookupBlockIndex returning a CBlockIndex with the same
const-ness as the member function:
(e.g. const CBlockIndex* LookupBlockIndex(...) const)
See next commit for some weirdness that this eliminates.
The range based for-loops are modernize (using auto + destructuring) in
a future commit.
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
