32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28 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 32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28
achow101:
ACK 32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28
theStack:
Code-review ACK 32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28
w0xlt:
Code Review ACK 32e5edc0f4
brunoerg:
crACK 32e5edc0f454c59c8e0d8d86a9abfa9a3f25ca28
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).
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
7832e9438f5c66b88f60676d14e1e11d669eb109 test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379fa088aa7aa03b2f505342a5b3bc3436 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 7832e9438f5c66b88f60676d14e1e11d669eb109
Xekyo:
re-ACK 7832e9438f5c66b88f60676d14e1e11d669eb109
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
e3609cdc01cf992800f28b20b0107b7fdc1f880e 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 e3609cdc01cf992800f28b20b0107b7fdc1f880e
achow101:
ACK e3609cdc01cf992800f28b20b0107b7fdc1f880e
Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns)
d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37e1b2d19e5a053dbfefa30306c1d41a 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 ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
hebasto:
ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e
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.
57fb37c27599fc865f20b42a27bb9c227f384de3 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 57fb37c27599fc865f20b42a27bb9c227f384de3
jonatack:
ACK 57fb37c
ryanofsky:
Code review ACK 57fb37c27599fc865f20b42a27bb9c227f384de3. 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
3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0190c634b5f1c85f749437f4209cc36 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 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
laanwj:
Code review ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
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.
6b636730f4befee39d57fcfd51298f3015dbf563 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 6b636730f4befee39d57fcfd51298f3015dbf563
furszy:
re-ACK 6b636730
Tree-SHA512: 453825123aa10748642c7dd94324ced2d07df0f4fac478b0947a34820b515ae300f75721679a90a164f3127029739df55c4de035c4567e663893c3c6dbdef216
c4e77177276ea2b79c4675cb2678ee2cc757b743 refactor: Change LogPrintLevel order to category, severity (laanwj)
ce920713bf0810614c2c0c994511b50d4f660bce leveldb: Log messages from leveldb with category and debug level (laanwj)
18ec120bb9e1fc9d27d2419da4c580bd3cde7e86 http: Use severity-based logging for messages from libevent (laanwj)
bd971bffb02c7b06aac9a479f7e5ed8f71de2bec 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 c4e77177276ea2b79c4675cb2678ee2cc757b743
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.
6fbb0edac22c63f1b723f731c2601b1d46879a58 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 6fbb0edac22c63f1b723f731c2601b1d46879a58
Xekyo:
re-ACK 6fbb0edac22c63f1b723f731c2601b1d46879a58
w0xlt:
Code Review ACK 6fbb0edac2
furszy:
Looks good, have been touching this area lately, code review ACK 6fbb0eda.
Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
baa3ddc49c46d00e3e0de06e494656f0f00b0ee8 doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658ad93f7b628eb2a3411fec2265d73fb 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 baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
theStack:
re-ACK baa3ddc49c46d00e3e0de06e494656f0f00b0ee8
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.
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
a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836c4339aa7f14fc1c9583d86dd5c388a 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 a4703ce9d79855ac0bd7dc07b71a51245f9aa5f8
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
fa9af218780b7960d756db80c57222e5bf2137b1 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 fa9af218780b7960d756db80c57222e5bf2137b1
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
ac6fbf2c83578129a0397d0d0dc9b1c6bdb30701 tidy: use modernize-use-default-member-init (fanquake)
7aa40f55636be565441a9e0af8de0a346bfa4da2 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
a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0 Add more proper thread safety annotations (Hennadii Stepanov)
8cfe93e3fcf263bf059f738d5e7d9c94901a7c5a Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov)
ca446f2c59720c1575aeeab9c9d636d98ce8528c 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 a55db4ea1cf10e0ab4a6eb5cd1dd3bd95626fba0
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.
4c5ceb040cf50d24201903a9200fb23be88d96fb wallet: CreateTransaction(): return out-params as (optional) struct (Sebastian Falbesoner)
c9fdaa5e3ae09b45be6a5c2d4ee6b1e8cef9d8a8 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 4c5ceb040cf50d24201903a9200fb23be88d96fb
Xekyo:
ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb
w0xlt:
crACK 4c5ceb040c
Tree-SHA512: 27e5348bbf4f698713002d40c834dcda59c711c93207113e14522fc6d9ae7f4d8edf1ef6d214c5dd62bb52943d342878960ca333728828bf39b645a27d55d524
ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67 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 ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67
achow101:
ACK ba10b90915dae6a802ecb0f80f72a1a9ea5a4c67
Tree-SHA512: 1d2fa2db189d3e87f2d0863cf2ab62166094436483f0da16760b1083a4743bf08e476a3277e0d36564213d65dd6f0a1fc16a4bf68d3338c991a14d1de9fc0fee
