111ea3ab71 wallet: refactor GetNewDestination, use BResult (furszy)
22351725bc send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca162 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33d1f Introduce generic 'Result' class (furszy)
Pull request description:
Based on a common function signature pattern that we have all around the sources:
```cpp
bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
// do something...
if (error) {
error_string = "something bad happened";
return false;
}
result = goodResult;
return true;
}
```
Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.
Obtaining in this way cleaner function signatures and removing boilerplate code:
```cpp
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
// do something...
if (error) return "something bad happened";
return goodResult;
}
```
Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:
Before:
```cpp
Obj result_obj;
std::string error_string;
if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
LogPrintf("Error: %s", error_string);
}
return result_obj;
```
Now:
```cpp
BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
if (!op_res) {
LogPrintf("Error: %s", op_res.GetError());
}
return op_res.GetObjResult();
```
### Initial Implementation:
Have connected this new concept to two different flows for now:
1) The `CreateTransaction` flow. --> 7ba2b87c
2) The `GetNewDestination` flow. --> bcee0912
Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).
Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).
ACKs for top commit:
achow101:
ACK 111ea3ab71
w0xlt:
reACK 111ea3ab71
theStack:
re-ACK 111ea3ab71
MarcoFalke:
review ACK 111ea3ab71🎏
Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
230a2f4cc3 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky)
a89ddfbe22 wallet: Save wallet scan progress (w0xlt)
Pull request description:
Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from scratch on the next load.
This PR changes this and the progress is saved right after checking a block.
Close https://github.com/bitcoin/bitcoin/issues/25010
ACKs for top commit:
furszy:
re-ACK 230a2f4
achow101:
ACK 230a2f4cc3
ryanofsky:
Code review ACK 230a2f4cc3. Only change since last review is tweaking whitespace and adding log print
Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32
If we are given a PSBT that is missing one or more input UTXOs, our
PrecomputedTransactionData will be incorrect and missing information
that it should otherwise have, and therefore we may not produce a
signature when we should. To avoid this problem, we can do the
precomputation after we have set the UTXOs the wallet is able to set for
the PSBT.
Also adds a test for this behavior.
98ea43d5e9 test: add tests for negative waste during coin selection (ishaanam)
Pull request description:
#25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.
ACKs for top commit:
achow101:
ACK 98ea43d5e9
glozow:
light code review ACK 98ea43d5e9, good to have tests for negative waste
Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
d54c5c8b1b wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84e wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)
Pull request description:
Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`
ACKs for top commit:
achow101:
ACK d54c5c8b1b
theStack:
Code-review ACK d54c5c8b1b
Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)
Pull request description:
### Context
The wallet's `m_address_book` field is being accessed directly from several places across the sources.
### Problem
Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.
### Solution
Encapsulate `m_address_book` access inside the wallet.
-------------------------------------------------------
Extra Note:
This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).
ACKs for top commit:
achow101:
ACK d69045e291
theStack:
ACK d69045e291✅
w0xlt:
ACK d69045e291
Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
140d942634 wallet: don't add change fee to target if subtracting fees from output (S3RK)
Pull request description:
Change fee is payed by the recipient, so we don't need to add it to our target for coin selection.
ACKs for top commit:
achow101:
ACK 140d942634
ishaanam:
ACK 140d942634
furszy:
Code review ACK 140d9426
Tree-SHA512: b5efd0264c47ecee9204a3fd039bad24c69f9e614c6e1d9bb240ee5be6356b175aa074f3be123e6cfb8becd4d7bd1028eebe18801662cc69d19413d8d5a9dd5c
Instead of calling GetCachableAmount twice, which will result in
iterating through all the transaction txins/txouts and calling
GetDebit/GetCredit (which lock cs_wallet), just merge the filters and do
it once.
c318211ddd walletdb: fix last client version update (furszy)
bda8ebe608 wallet: don't read db every time that a new WalletBatch is created (furszy)
Pull request description:
Found it while was working on #25297.
We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not.
As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached.
ACKs for top commit:
achow101:
ACK c318211ddd
w0xlt:
reACK c318211ddd
Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
25e4762ae7 wallet: more accurate tx_noinputs_size (S3RK)
Pull request description:
Rationale: more accurate non-input fee estimation for txs with >=253 inputs
ACKs for top commit:
laanwj:
Concept and code review ACK 25e4762ae7
achow101:
ACK 25e4762ae7
furszy:
Code review ACK 25e4762a. left a small nit.
Tree-SHA512: bda8fad725d32ad3e13c007fa56ddb6679ac1a32098ddb08d9a114054acfa681cb66cd703ac675297f731cb381b09067a99a4efa31320140bbdd03f0cfdc81af
This reverts commit 9b5950db86.
Waste can be negative. At feerates lower than long_term_feerate this
means that a waste of 0 may be a suboptimal solution and this causes the
search to exit prematurely.
Only when the feerate is equal to the long_term_feerate would achieving
a waste of 0 indicate that we have achieved an optimal solution,
because it would mean that the excess is 0. It seems unlikely
that this would ever occur outside of test cases, and even then we
should prefer solutions with more inputs over solutions with fewer
according to previous decisions—but solutions with more inputs are found
later in the branch exploration.
The "optimization" described in #18257 and implemented in #18262 is
therefore a premature exit on a suboptimal solution and should be reverted.
e673d8b475 bench: Enable loading benchmarks depending on what's compiled (Andrew Chow)
4af3547eba bench: Use mock wallet database for wallet loading benchmark (Andrew Chow)
49910f255f sqlite: Use in-memory db instead of temp for mockdb (Andrew Chow)
a1080802f8 walletdb: Create a mock database of specific type (Andrew Chow)
7c0d34476d bench: reduce the number of txs in wallet for wallet loading bench (Andrew Chow)
f85b54ed27 bench: Add transactions directly instead of mining blocks (Andrew Chow)
d94244c4bf bench: reduce number of epochs for wallet loading benchmark (Andrew Chow)
817c051364 bench: use unsafesqlitesync in wallet loading benchmark (Andrew Chow)
9e404a9831 bench: Remove minEpochIterations from wallet loading benchmark (Andrew Chow)
Pull request description:
`minEpochIterations` is probably unnecessary to set, so removing it makes the runtime much faster.
ACKs for top commit:
Rspigler:
tACK e673d8b475
furszy:
Code review ACK e673d8b4, nice PR.
glozow:
Concept ACK e673d8b475. For each commit, verified that there was a performance improvement without negating the purpose of the bench, and made some effort to verify that the code is correct.
Tree-SHA512: 9337352ef846cf18642d5c14546c5abc1674b4975adb5dc961a1a276ca91f046b83b7a5e27ea6cd26264b96ae71151e14055579baf36afae7692ef4029800877
When filling a PSBT, we search the listed pubkeys in order to determine
whether the current DescriptorScriptPubKeyMan could sign the transaction
even if it is not watching the scripts. With Taproot, the taproot
pubkeys need to be searched as well.
Currently, the wallet scan progress is not saved.
If it is interrupted, it will be necessary to start from
scratch on the next load.
With this change, progress is saved every 60 seconds.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Co-authored-by: Jon Atack <jon@atack.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
32e5edc0f4 wallet: avoid extra wtx lookup in AddToSpends (furszy)
Pull request description:
As `AddToSpends` is only called from `AddToWallet` and `LoadToWallet`, places where we insert the wtx into the wallet map, we can directly feed `AddToSpends` with the `wtx` and remove another extra lookup.
ACKs for top commit:
laanwj:
Code review ACK 32e5edc0f4
achow101:
ACK 32e5edc0f4
theStack:
Code-review ACK 32e5edc0f4
w0xlt:
Code Review ACK 32e5edc0f4
brunoerg:
crACK 32e5edc0f4
Tree-SHA512: e9fb8df44c3e3fa26c107d261bf78e45014b4755890a64817f2be62ee6b7751f5dd2813a18dcb103a21ddba1422f9d2d59c4bf186f08314e634365d36b01be8f
Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins.
Full explanation is inside #25118 comments but brief summary is:
`vCoins` at `SelectCoins` time could not be containing the manually selected input because, even when they were selected by the user, the current `AvailableCoins` flow skips locked and spent coins.
Extra note: this is an intermediate step to unify the `fAllowOtherInputs`/`m_add_inputs` concepts. It will not be a problem anymore in the future when we finally decouple the wtx-outputs lookup process from `SelectCoins` and don't skip the user's manually selected coins in `AvailableCoins`.
Seeking to make the `CoinControl` option less confusing/redundant.
In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: Search the wtxs-outputs and append all the `CoinControl` selected outpoints to the selection result (skipping all the available output checks). Nothing else.
Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector).
As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not.
So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped coins into 'vCoins' vector if they were manually selected by the user (follow-up commits).
fd5c996d16 wallet: GetAvailableBalance, remove double walk-through every available coin (furszy)
162d4ad10f wallet: add 'only_spendable' filter to AvailableCoins (furszy)
cdf185ccfb wallet: remove unused IsSpentKey(hash, index) method (furszy)
4b83bf8dbc wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy)
3d8a282257 wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy)
a06fa94ff8 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy)
91902b7720 wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy)
9472ca0a65 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy)
4ce235ef8f 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 fd5c996d16
brunoerg:
crACK fd5c996d16
w0xlt:
Code Review ACK fd5c996d16
Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220
The value was only being updated launching releases with higher version numbers
and not if the user launched a previous release.
Co-authored-by: MacroFake <falke.marco@gmail.com>
7832e9438f test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379f wallet: do not count wallet utxos as external (S3RK)
Pull request description:
Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.
Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.
ACKs for top commit:
achow101:
re-ACK 7832e9438f
Xekyo:
re-ACK 7832e9438f
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
e3609cdc01 doc: Update importaddress mention incompatibility with descriptor wallet (BrokenProgrammer)
Pull request description:
This is related to #25363 and offers a small update to the error messages from `EnsureLegacyScriptPubKeyMan` and `EnsureConstLegacyScriptPubKeyMan` to mention that they only are compatible with legacy wallets.
The RPC documentation for `importaddress` is also updated to mention this as well as guide the user to the alternative `importdescriptors` for cases when using descriptor wallets.
I'm thinking that we can introduce a "porting guide" document mentioned in #25363 in a separate PR since I would have to make myself more familiar with the subject before being able to tackle that.
ACKs for top commit:
laanwj:
Code review ACK e3609cdc01
achow101:
ACK e3609cdc01
Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows:
GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance.
Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.
So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.
Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally.
This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.
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.
57fb37c275 wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error. (furszy)
Pull request description:
Two points for `CWallet::CommitTransaction`:
1) The extra wtx lookup:
As we are calling to `AddToWallet` first, which returns the recently added/updated wtx pointer, there is no need to look up the wtx again few lines later. We can just use it.
2) The db write error:
`AddToWallet` can only return a nullptr if the db write fails, which inside `CommitTransaction` translates to an exception throw cause. We expect everywhere that `CommitTransaction` always succeed.
------------------------------------------------
Extra note:
This finding generated another working path for me :)
It starts with the following question: why are we returning a nullptr from `AddToWallet` if the db write failed without removing the recently added transaction from the wallet's map?..
Can led to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
-- I'm writing it here to gather some feedback first and not forget it, will create a follow-up PR in the coming days 🚜 --
ACKs for top commit:
achow101:
ACK 57fb37c275
jonatack:
ACK 57fb37c
ryanofsky:
Code review ACK 57fb37c275. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway
Tree-SHA512: 80e59c01852cfbbc70a5de1a1c2c59b5e572f9eaa08c2175112cb515256e63fa04c7942f92a513b620d6b06e66392029ebe8902287c456efdbee58a7a5ae42da
3a9b9bb38e test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes#25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e
laanwj:
Code review ACK 3a9b9bb38e
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
1) `Wallet::AddToWallet` is already returning the pointer to the inserted `CWalletTx`, so there is no need to look it up in the map again.
2) `Wallet::AddToWallet` can only return a nullptr if the db `writeTx` call failed. Which should be treated as an error.
6b636730f4 tracing: fix `coin_selection:aps_create_tx_internal` calling logic (Sebastian Falbesoner)
Pull request description:
According to the documentation, the tracepoint `coin_selection:aps_create_tx_internal` "Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes."
Currently it is only called if the second call to `CreateTransactionInternal` succeeds, i.e. the third parameter is always `true` and we don't get notified in the case that it fails. This PR fixes this by moving the tracepoint call and the `use_aps` boolean variable outside the if body.
ACKs for top commit:
achow101:
ACK 6b636730f4
furszy:
re-ACK 6b636730
Tree-SHA512: 453825123aa10748642c7dd94324ced2d07df0f4fac478b0947a34820b515ae300f75721679a90a164f3127029739df55c4de035c4567e663893c3c6dbdef216
c4e7717727 refactor: Change LogPrintLevel order to category, severity (laanwj)
ce920713bf leveldb: Log messages from leveldb with category and debug level (laanwj)
18ec120bb9 http: Use severity-based logging for messages from libevent (laanwj)
bd971bffb0 logging: Unconditionally log levels >= WARN (laanwj)
Pull request description:
Log messages from leveldb and libevent libraries in the severity+level based log format introduced in bitcoin/bitcoin#24464.
Example of messages before:
```
2022-05-24T18:11:57Z [libevent] libevent: event_add: event: 0x55da963fcc10 (fd 10), EV_READ call 0x7f1c7a254620
2022-05-24T18:11:57Z [libevent] libevent: Epoll ADD(1) on fd 10 okay. Old events were 0; read change was 1 (add); write change was 0 (none); close change was 0 (none)
2022-05-24T18:12:08Z leveldb: Generated table #609127@1: 6445 keys, 312916 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609128@1: 5607 keys, 268548 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609129@1: 189 keys, 9384 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609130@1: 293 keys, 13818 bytes
```
Example of messages after:
```
2022-05-24T17:59:52Z [libevent:debug] event_add: event: 0x5652f44dac10 (fd 10), EV_READ call 0x7f210f2e6620
2022-05-24T17:59:52Z [libevent:debug] Epoll ADD(1) on fd 10 okay. Old events were 0; read change was 1 (add); write change was 0 (none); close change was 0 (none)
2022-05-24T17:59:53Z [leveldb:debug] Recovering log #1072
2022-05-24T17:59:53Z [leveldb:debug] Level-0 table #1082: started
2022-05-24T17:59:53Z [leveldb:debug] Level-0 table #1082: 193 bytes OK
2022-05-24T17:59:53Z [leveldb:debug] Delete type=3 #1070
2022-05-24T17:59:53Z [leveldb:debug] Delete type=0 #1072
```
The first commit changes it so that messages with level Warning and Error are always logged independent of the `-debug` setting. I think this is useful to make sure warnings and errors, which tend to be important, are not lost. In the future this should be made more configurable.
Last commit changes LogPrintLevel argument order to category, severity: This is more consistent with the other functions, as well as with the logging output itself. If we want to make this change, we should do it before it's all over the place.
ACKs for top commit:
jonatack:
Tested ACK c4e7717727
Tree-SHA512: ea48a1517f8c1b23760e59933bbd64ebf09c32eb947e31b8c2696b4630ac1f4303b126720183e2de052bcede3a17e475bbf3fbb6378a12b0fa8f3582d610930d
Messages with level `WARN` or higher should be logged even when
the category is not provided with `-debug=`, to make sure important
warnings are not lost.
6fbb0edac2 Set effective_value when initializing a COutput (ishaanam)
Pull request description:
Previously in COutput, effective_value was initialized as the absolute value of the txout and the fee as 0. effective_value along with the fee was 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 and the 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.
ACKs for top commit:
achow101:
re-ACK 6fbb0edac2
Xekyo:
re-ACK 6fbb0edac2
w0xlt:
Code Review ACK 6fbb0edac2
furszy:
Looks good, have been touching this area lately, code review ACK 6fbb0eda.
Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
baa3ddc49c doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)
Pull request description:
Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.
If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).
ACKs for top commit:
achow101:
ACK baa3ddc49c
theStack:
re-ACK baa3ddc49c
w0xlt:
ACK baa3ddc49c
Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
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.
If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
3f8def51d5 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 3f8def51d5
brunoerg:
ACK 3f8def51d5
Tree-SHA512: 8db6dd942b02a38c99953b801605f98c4c17729768fdfcf7605c5bbdb17509500a39d0a78a4b19aab37812d2994ec7630d2b4e78d1d348f1c27b67588d74e155
a4703ce9d7 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner)
Pull request description:
Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK a4703ce9d7
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
fa9af21878 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af21878
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
ac6fbf2c83 tidy: use modernize-use-default-member-init (fanquake)
7aa40f5563 refactor: use C++11 default initializers (fanquake)
Pull request description:
Refactor and then enable [`modernize-use-default-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html) in our `clang-tidy` job.
Top commit has no ACKs.
Tree-SHA512: 536b406f20639f8c588fe9e96175ec60c7bb825506b2670b562370b2f572801c24203c483443be3c199e1b958c0765d4532e57c57a4e78689162a1dd422d844f
a55db4ea1c Add more proper thread safety annotations (Hennadii Stepanov)
8cfe93e3fc Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov)
ca446f2c59 Add proper thread safety annotation to `CachedTxGetAvailableCredit()` (Hennadii Stepanov)
Pull request description:
In non-test/benchmarking code, there are three cases of the `NO_THREAD_SAFETY_ANALYSIS` annotation which are accompanied with `TODO` comments.
This PR adds proper thread safety annotations instead of `NO_THREAD_SAFETY_ANALYSIS`.
ACKs for top commit:
laanwj:
Code review ACK a55db4ea1c
Tree-SHA512: 806d72eebc1edf088bfa435c8cd11465be0de6789798dd92abd008425516768acb864a73d834a49d412bb10f7fccfb47473f998cb72739dab6caeef6bcfaf191
According to the documentation, the tracepoint
`coin_selection:aps_create_tx_internal` "Is called when the second
`CreateTransactionInternal` with Avoid Partial Spends enabled completes."
Currently it is only called if the second call to
`CreateTransactionInternal` succeeds, i.e. the third parameter is always
`true` and we don't get notified in the case that it fails.
Fix this by introducing a boolean variable for the result of the call
and moving the tracepoint call outside the if body.
4c5ceb040c wallet: CreateTransaction(): return out-params as (optional) struct (Sebastian Falbesoner)
c9fdaa5e3a wallet: CreateTransactionInternal(): return out-params as (optional) struct (Sebastian Falbesoner)
Pull request description:
The method `CWallet::CreateTransaction` currently returns several values in the form of out-parameters:
* the actual newly created transaction (`CTransactionRef& tx`)
* its required fee (`CAmount& nFeeRate`)
* the position of the change output (`int& nChangePosInOut`) -- as the name suggests, this is both an in- and out-param
By returning these values in an optional structure (which returns no value a.k.a. `std::nullopt` if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don't have to create dummy variables for results that they are not interested in.
Note that the names of the replaced out-variables were kept in `CreateTransactionInternal` to keep the diff minimal. Also, the fee calculation data (`FeeCalculation& fee_calc_out`) would be another candidate to put into the structure, but `FeeCalculation` is currently an opaque data type in the wallet interface and I think it should stay that way.
As a potential follow-up, I think it would make sense to also do the same refactoring for `CWallet::FundTransaction`, which has a very similar parameter structure.
Suggested by laanwj in https://github.com/bitcoin/bitcoin/pull/20588#issuecomment-739838428.
ACKs for top commit:
achow101:
re-ACK 4c5ceb040c
Xekyo:
ACK 4c5ceb040c
w0xlt:
crACK 4c5ceb040c
Tree-SHA512: 27e5348bbf4f698713002d40c834dcda59c711c93207113e14522fc6d9ae7f4d8edf1ef6d214c5dd62bb52943d342878960ca333728828bf39b645a27d55d524
ba10b90915 Wallet: Ensure m_attaching_chain is set before registering for signals (Luke Dashjr)
Pull request description:
Avoids a race where chainStateFlushed could be called before rescanning began, yet rescan gets interrupted or fails
Followup for #24984 avoiding a race between registering and setting the flag.
ACKs for top commit:
mzumsande:
Code Review ACK ba10b90915
achow101:
ACK ba10b90915
Tree-SHA512: 1d2fa2db189d3e87f2d0863cf2ab62166094436483f0da16760b1083a4743bf08e476a3277e0d36564213d65dd6f0a1fc16a4bf68d3338c991a14d1de9fc0fee
