fa3f15f2dd refactor: Avoid copies in FlatSigningProvider Merge (MacroFake)
Pull request description:
`Merge` will create several copies unconditionally:
* To initialize the args `a`, and `b`
* `ret`, which is the merge of the two args
So change the code to let the caller decide how many copies they need/want:
* `a`, and `b` must be explicitly moved or copied by the caller
* `ret` is no longer needed, as `a` can be used for it in place "for free"
ACKs for top commit:
achow101:
ACK fa3f15f2dd
furszy:
looks good, ACK fa3f15f2
ryanofsky:
Code review ACK fa3f15f2dd. Confirmed that all the places `std::move` was added the argument actually did seem safe to move from. Compiler enforces that temporary copies are explicitly created in non-move cases.
Tree-SHA512: 7c027ccdea1549cd9f37403344ecbb76e008adf545f6ce52996bf95e89eb7dc89af6cb31435a9289d6f2eea1c416961b2fb96348bc8a211d550728f1d99ac49c
90a5dfa509 RPC/Mining: Clean out pre-Segwit miner compatibility code (Luke Dashjr)
Pull request description:
This is dead code post-Segwit.
ACKs for top commit:
achow101:
ACK 90a5dfa509
Tree-SHA512: 5970aa3548d2a7da7c6e83fb9b910529faab10251b115122cec833bb7d3a54c7cb0714c1a873807be04c7817bb827c7ece1e20e8fa4c907aa58688487d0ec44d
8cd21bb279 refactor: improve readability for AttemptSelection (josibake)
f47ff71761 test: only run test for descriptor wallets (josibake)
0760ce0b9e test: add missing BOOST_ASSERT (josibake)
db09aec937 wallet: switch to new shuffle, erase, push_back (josibake)
b6b50b0f2b scripted-diff: Uppercase function names (josibake)
3f27a2adce refactor: add new helper methods (josibake)
f5649db9d5 refactor: add UNKNOWN OutputType (josibake)
Pull request description:
This PR is to address follow-ups for #24584, specifically:
* Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult`
* Add missing `BOOST_ASSERT` to unit test
* Ensure functional test only runs if using descriptor wallets
* Improve readability of `AttemptSelection` by removing triple-nested if statement
Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.
ACKs for top commit:
achow101:
ACK 8cd21bb279
aureleoules:
ACK 8cd21bb279.
LarryRuane:
Concept, code review ACK 8cd21bb279
furszy:
utACK 8cd21bb2. Left a small, non-blocking, comment.
Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
a6b0c1fcc0 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot)
0fd2d14454 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot)
55f98d087e rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot)
b724476158 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot)
55a82eaf91 wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot)
Pull request description:
Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends).
It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit.
I'll add this to the output of `listunspent` too if this gets a few Concept ACKs.
ACKs for top commit:
instagibbs:
ACK a6b0c1fcc0
achow101:
re-ACK a6b0c1fcc0
Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
It's useful for an external application tracking coins to not be limited
by our change detection. For instance, for a watchonly wallet with two
descriptors a transaction from one to the other would be considered a
change output and not be included in the result (if the address was not
generated by this wallet).
This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.
Calling ActiveHeight() and ActiveTip() subsequently without holding the
::cs_main lock over both calls may result in a height that does not
correspond to the tip due to a race.
Fix this by holding the lock.
ActiveTip() is *not* thread-safe, as the required ::cs_main lock will be
released as ActiveChainstate() returns.
ActiveTip() is an alias for ActiveChainstate().m_chain.Tip(), so m_chain
may be involved in a data-race (UB).
If not accepting I2P connections, then do not create
`CConnman::m_i2p_sam_session`.
When opening a new outbound I2P connection either use
`CConnman::m_i2p_sam_session` like before or create a temporary one and
store it in `CNode` for destruction later.
and destroy it when `CNode::m_sock` is closed.
I2P transient sessions are created per connection (i.e. per `CNode`) and
should be destroyed when the connection is closed. Storing the session
in `CNode` is a convenient way to destroy it together with the connection
socket (`CNode::m_sock`).
An alternative approach would be to store a list of all I2P sessions in
`CConnman` and from `CNode::CloseSocketDisconnect()` to somehow ask the
`CConnman` to destroy the relevant session.
fea75ad3ca refactor: Drop `boost/algorithm/string/replace.hpp` dependency (Hennadii Stepanov)
857526e8cb test: Add test case for `ReplaceAll()` function (Hennadii Stepanov)
Pull request description:
A new implementation of the `ReplaceAll()` seems enough for all of our purposes.
ACKs for top commit:
adam2k:
ACK Tested fea75ad3ca
theStack:
Code-review ACK fea75ad3ca
Tree-SHA512: dacfffc9d2bd1fb9f034baf8c045b1e8657b766db2f0a7f8ef7e25ee6cd888f315b0124c54aba7a29ae59186b176ef9868a8b709dc995ea215c6b4ce58e174d9
f6a916683d Add functional test for block announcements during initial headers sync (Suhas Daftuar)
05f7f31598 Reduce bandwidth during initial headers sync when a block is found (Suhas Daftuar)
Pull request description:
On startup, if our headers chain is more than a day behind current time, we'll pick one peer to sync headers with until our best headers chain is caught up (at that point, we'll try to sync headers with all peers).
However, if an INV for a block is received before our headers chain is caught up, we'll then start to sync headers from each peer announcing the block. This can result in doing a big headers sync with many (if not all) of our peers simultaneously, which wastes bandwidth.
This PR would reduce that overhead by picking (at most) one new peer to try syncing headers with whenever a new block is announced, prior to our headers chain being caught up.
ACKs for top commit:
LarryRuane:
ACK f6a916683d
ajtowns:
ACK f6a916683d
mzumsande:
ACK f6a916683d
dergoegge:
Code review ACK f6a916683d
achow101:
ACK f6a916683d
Tree-SHA512: 0662000bd68db146f55981de4adc2e2b07cbfda222b1176569d61c22055e5556752ffd648426f69687ed1cc203105515e7304c12b915d6270df8e41a4a0e1eaa
4584d300a4 GUI: remove now unneeded 'm_balances' field from overviewpage (furszy)
050e8b1391 GUI: 'getAvailableBalance', use cached balance if the user did not select UTXO manually (furszy)
96e3264a82 GUI: use cached balance in overviewpage and sendcoinsdialog (furszy)
321335bf02 GUI: add getter for WalletModel::m_cached_balances field (furszy)
e62958dc81 GUI: sendCoinsDialog, remove duplicate wallet().getBalances() call (furszy)
Pull request description:
As per the title says, we are recalculating the entire wallet balance on different situations calling to `wallet().getBalances()`, when should instead make use of the wallet model cached balance.
This has the benefits of (1) not spending resources calculating a balance that we already have cached, and (2) avoid blocking the main thread for a long time, in case of big wallets, walking through the entire wallet's tx map more than what it's really needed.
Changes:
1) Fix: `SendCoinsDialog` was calling `wallet().getBalances()` twice during `setModel`.
2) Use the cached balance if the user did not select any UTXO manually inside the wallet model `getAvailableBalance` call.
-----------------------
As an extra note, this work born in [#25005](https://github.com/bitcoin/bitcoin/pull/25005) but grew out of scope of it.
ACKs for top commit:
jarolrod:
ACK 4584d300a4
hebasto:
re-ACK 4584d300a4, only suggested changes and commit message formatting since my [recent](https://github.com/bitcoin-core/gui/pull/598#pullrequestreview-1071268192) review.
Tree-SHA512: 6633ce7f9a82a3e46e75aa7295df46c80a4cd4a9f3305427af203c9bc8670573fa8a1927f14a279260c488cc975a08d238faba2e9751588086fea1dcf8ea2b28
fcc0c8448b qt: Update source translations for string freeze (laanwj)
Pull request description:
Make sure transifex source translations match the ones in the source code after today's string freeze.
It looks like only one message was added: "Too many external signers
found".
ACKs for top commit:
hebasto:
ACK fcc0c8448b, `make -C src translate` produces zero diff.
jarolrod:
ACK fcc0c8448b
Tree-SHA512: 2c2383a6f1ad086e86b7c7d35b9a06c30090a8c9517f93912b64957ec58c95c982d5340f189028717dc876e14ba2344750e3f5d81b522b0cf3d7f4586b8e0cc6
When we have preselected inputs the coin selection search target is reduced
by the sum of (effective) values. This causes incorrect m_target value.
Create separate instance of SelectionResult for all the preselected inputs and
set the target equal to the sum of (effective) values. Target for preselected
SelectionResult is equal to the delta for the search target. To get the final
SelectionResult with accurate m_target we merge both SelectionResult instances.
SelectionResult::m_target should be equal to actual selection target.
Selection target is the sum of all recipient amounts plus non input fees.
So we need to remove change_fee from the m_target. It's safe because change
target is always greater than the change fee, so we can always cover fees
if change output is created.
292b1a3e9c GetExternalSigner(): fail if multiple signers are found (amadeuszpawlik)
Pull request description:
If there are multiple external signers, `GetExternalSigner()` will
just pick the first one in the list. If the user has two or more
hardware wallets connected at the same time, he might not notice this.
This PR adds a check and fails with suitable message, forcing the user to disconnect all but one external signer, so that there is no ambiguity as to which external signer was used.
ACKs for top commit:
Sjors:
tACK 292b1a3e9c
achow101:
ACK 292b1a3e9c
Tree-SHA512: e2a41d3eecc607d4f94e708614bed0f3545f7abba85f300c5a5f0d3d17d72c815259734accc5ca370953eacd290f27894ba2c18016f5e9584cd50fa1ec2fbb0b
If our headers chain is behind on startup, then if a block is found we'll try
to catch up from all peers announcing the block, in addition to our initial
headers-sync peer. This commit changes behavior so that in this situation,
we'll choose at most one peer announcing a block to additionally sync headers
from.
Plus, calculate the cached balance right when the wallet model, so the wallet widgets don't need to redo the same balance calculation multiple times when they are waiting for the model balance polling timer.
----------------------------------------------------------------------
test wise: `WalletTests` now need to trigger the walletModel balance changed manually. So the model updates its internal state and can be used by the widgets.
This is because the test does not start the balance polling timer, in the same way as does not initialize several parts of the GUI workflow. All the objects (wallet, models, views, etc) that are used on this test are manually created instead of using the `WalletController` class flow.
Rationale is that this unit test is focused on verifying the GUI widgets/views behavior only: update the presented information, etc. when they receive different signals and/or function calls from outside (in other words, focus is on the signal slots/receiver side). It's not about whether the wallet balance polling timer is functioning as expected or not (which we definitely create a new test case for it in a follow-up work).
Inside setModel, we call 'wallet().getBalances()', to set the view balance,
right before calling 'updateDisplayUnit' which calls 'wallet().getBalances()'
internally and re-sets the view balance again.
Move TopUp() responsibility from the wallet class to each scriptpubkeyman.
So each spkm can decide to call it or not after perform the basic checks
for the new destination request.
Reason:
We were calling it twice in the following flows for descriptor wallets:
A) CWallet::GetNewDestination:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again.
B) CWallet::GetReservedDestination:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
daabd41211 net: simplify GetLocalAddress() (Vasil Dimov)
Pull request description:
There is no need to use two variables `ret` and `addr` of the same type
`CService` and assign one to the other in a strange way like
`ret = CService{addr}`.
ACKs for top commit:
jarolrod:
ACK daabd41211
aureleoules:
ACK daabd41211.
w0xlt:
ACK daabd41211
Tree-SHA512: 4bbd3746bc30fbc05bb32b58bb122c938acd849c0f72f1d3e8170557c1999ec26a888e06e874c3fc22562a2becddc7d817db7d174e0e1b383e8d74c39aa1e898
9376a6dae4 refactor: make active_chain_tip a reference (Aurèle Oulès)
Pull request description:
This PR fixes a TODO introduced in #21055.
Makes `active_chain_tip` argument in `CheckFinalTxAtTip` function a reference instead of a pointer.
ACKs for top commit:
dongcarl:
ACK 9376a6dae4
Tree-SHA512: c36d1769e0b9598b7f79334704b26b73e958d54caa3bd7e4eff954f3964fcf3f5e3a44a5a760497afad51b76e1614c86314fe035e4083c855e3574a620de7f4d
acda7e8686 [coin selection] consolidate m_change_target and m_min_change_target (glozow)
Pull request description:
These values are both intended for the same thing. Their divergence seems to be the result of an incomplete rename.
ACKs for top commit:
achow101:
ACK acda7e8686
Xekyo:
ACK acda7e8686
furszy:
ACK acda7e86
aureleoules:
ACK acda7e8686.
Tree-SHA512: 4b86171af5d893f7172373bb404bad12c49588ad1e22eb0544c242173f4bc4dede2ff1270c93c9f02f503ab8d9f66b841a8319d0ecb5e896d0fe8727cf03dbf4
