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
b16f93cadd script/sign: remove needless IsSolvable() utility (Antoine Poinsot)
c232ef20c0 outputtype: remove redundant check for uncompressed keys in AddAndGetDestinationForScript (Antoine Poinsot)
Pull request description:
Now that we have descriptors there is no need to try to sign for a scriptPubKey using dummy signatures, and using a mocked verification of this witness against the interpreter, just to make sure we know how to spend such a Script. Just try to infer a solvable descriptor: any scriptPubKey that we can sign for can be inferred as such.
This came up in #24149 but i think it's worth it on its own.
ACKs for top commit:
instagibbs:
ACK b16f93cadd
achow101:
re-ACK b16f93cadd
furszy:
ACK b16f93ca, only change is the `IsSolvable` helper function removal.
Tree-SHA512: 137068157ce90210b710b1bf9ac3c400e2ff5af1112f892094b69875ea473d6a899f52adb51e5030cb907dee517602059cd1661107808558efa5de842ba12b41
Instead of providing our destination (private key) to the I2P proxy when
creating the session, ask it to generate one for us and do not save it
on disk.
70a55c059b psbt: Avoid unsigned int overflow in PSBT_IN_TAP_BIP32_DERIVATION (Andrew Chow)
Pull request description:
Fixes #25749
ACKs for top commit:
instagibbs:
ACK 70a55c059b
darosior:
re-utACK 70a55c059b
jonatack:
Review ACK 70a55c059b, this should avoid the issue reported in https://github.com/bitcoin/bitcoin/issues/25749
Tree-SHA512: 6bb58e1cda9a5baa50fcd24f818b5b27ed94f0d33da3f71f6e457618176611bf2a84e1864e9a48d9303c301252bc4c1dee8b19a67dd713e849fb9442851ca341
fb9faffae3 extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670ce1 descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e761 (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a5ac descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c22bd spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot)
Pull request description:
We would previously silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.
An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke.
Fixes #25751.
ACKs for top commit:
achow101:
re-ACK fb9faffae3
instagibbs:
utACK fb9faffae3
Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
d8b26abed9 build: move raw rule into Makefile.am (fanquake)
Pull request description:
The same rule is used by the tests and benchmarks to generate headers,
and currently causes #25501. Just deduplicate the code into Makefile.am.
Fixes: #25501.
ACKs for top commit:
hebasto:
ACK d8b26abed9, tested on Ubuntu 22.04, the moved code was verified using `git diff --color-moved=dimmed-zebra HEAD~1..HEAD`.
jarolrod:
tACK d8b26abed9
Tree-SHA512: 249813318c92f992a89002fb9b96e70fca6ca97b2136ba0a7f5cc312e9abe24fbbe9a8faddb3bc1c0d775ae901bc91eab63ba564810bb2e3b9d56a2b1a107eb1
Use {Chain,}TestingSetup's existing {Chainstate,Block}Manager and avoid
unnecessarily creating a local one.
This also helps reduce the code diff for a later commit where we change
{Chainstate,Block}Manager's constructor signature.
it was pointed out by a few reviewers that the code block at the end
of attempt selection was difficult to follow and lacked comments.
refactor to get rid of triple nested if statement and improve
readibility.
switch to new methods, remove old code. this also
updates the Size, All, and Clear methods to now use
the coins map.
this commit is not strictly a refactor because previously
coin selection was never run over the UNKNOWN type until the last
step when being run over all. now that we are iterating over each,
it is run over UNKNOWN but this is expected to be empty most of the time.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
add Shuffle, Erase, and Add to CoinsResult struct
add a helper function for mapping TxoutType to OutputType
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
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}`.
