wallet: unify “allow/block other inputs“ concept #25118

furszy · 2022-05-12T14:49:17Z

Seeking to make the CoinControl options less confusing/redundant.
It should have no functional changes.

The too long to read technical description; remove m_add_inputs, we can use the already existent fAllowOtherInputs flag.

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 internal and external 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).

Changes

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 internal and external coins into 'vCoins' vector if they were manually selected by the user.

——————————————————————————————————

Just as an extra note:
On top of this, I’m working on unifying/untangling further the coin filtering and selection processes so we have less duplicate functionality in both processes.

DrahtBot · 2022-05-13T17:09:59Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

achow101 · 2022-05-16T19:21:48Z

Actually I don't think they are different behavior. They just are set and checked at different times so these two variables appear to be controlling different things.

If m_add_inputs == false, then we only filter for the preset inputs in AvailableCoins. If fAllowOtherInputs == false, we also only filter for the preset inputs in AvailableCoins. In SelectCoins, if fAllowOtherInputs == false, we have an optimization where we just select all of the preset inputs and exit early. For m_add_inputs == false, we don't do the optimization but instead select all of the inputs, do the coin selection algorithms, and arrive at the same selection of all of the preset inputs. So these two options fundamentally do the same thing and could actually be unified.

There is some weird interactions between then, such as when m_add_inputs == false, but fAllowOtherInputs is force set to true in FundTransaction - in this case m_add_inputs "overrides" fAllowOtherInputs, but it just isn't as obvious that it does. This would definitely be cleared up if they were the same variable.

So there is no need to introduce m_use_manual_selection.

furszy · 2022-05-16T21:00:05Z

Hey achow101, thanks for the review!

If m_add_inputs == false, then we only filter for the preset inputs in AvailableCoins. If fAllowOtherInputs == false, we also only filter for the preset inputs in AvailableCoins. In SelectCoins, if fAllowOtherInputs == false, we have an optimization where we just select all of the preset inputs and exit early. For m_add_inputs == false, we don't do the optimization but instead select all of the inputs, do the coin selection algorithms, and arrive at the same selection of all of the preset inputs. So these two options fundamentally do the same thing and could actually be unified.

I came to the same conclusion but.. the reason behind the new flag are all the AvailableCoins checks prior appending the coins to the vCoins vector that are being skipped in the second part of the SelectCoins process (where it loops over the CoinControl manually selected outputs and forcibly adds them to the tx, getting them from the wallet tx map directly instead of looking them inside vCoins..).

In other words, the usage of m_add_inputs=true was telling the wallet: "add this outputs as inputs into the tx; don't care if they are spent, locked or whatever state they are. Just add them".
While HasSelected() + fAllowOtherInputs=false is a "add this outputs as inputs into the tx only if they are in a valid state, not locked, with enough depth, etc".

An easy example of this is the locked coins skip that is inside AvailableCoins but not in the second part of SelectCoins:

The locked coins are, forcibly, selected inside the second part of SelectCoins while they were skipped inside AvailableCoins.. so if I change the current behavior (which I agree that is completely redundant: further notes below) and use the first part of the SelectCoins process: as the locked coins aren't inside vCoins, the process will not be able to select them, throwing an "Insufficient funds" error.

To try it, you can just set the new flag m_use_manual_selection to true at line 609 of rpc/spend.cpp, which without something like furszy@1291ba7 will fail at line 140 of the rpc_psbt.py test (which is the test line that checks that coin locks are ignored for manually selected inputs).

So.. conclusion:

I agree with you and have been working on it on a local branch. Unifying and cleaning most of these redundancies (thus why found #25005). And added the manual flag here in order to not add a bulk of changes that are required to keep the same current behavior (explained above) without entering into the rabbit hole.

Getting deeper, some of the rabbit hole changes, linked to this work, that I have been working on locally (WIP) are:

The SelectCoins process shouldn't be searching coins in the wallet. That is exactly what AvailableCoins is already doing --> It only needs to be focused on the coins selection algorithm.
The wallet shouldn't be looping across the entire wallet's tx map if the user wants to select an specific set of inputs and nothing else --> it can just select them right away (skipping all the AvailableCoins checks using the manual selection flag).
The locked coins skip/add can be customizable by the user at selection time.

achow101 · 2022-05-16T21:46:16Z

In other words, the usage of m_add_inputs=true was telling the wallet: "add this outputs as inputs into the tx; don't care if they are spent, locked or whatever state they are. Just add them".
While HasSelected() + fAllowOtherInputs=false is a "add this outputs as inputs into the tx only if they are in a valid state, not locked, with enough depth, etc".

I don't think this is a useful distinction.

The only ways to manually set inputs are through send, fundrawtransaction, walletcreatefundedpsbt, and the GUI. Those 3 RPCs all use the same FundTransaction function which currently does fAllowOtherInputs = true, so they always add the specified inputs regardless of state. The GUI only allows users to preset input that are shown in a list - a list which is created by doing AvailableCoins (with the addition of locked coins, but the GUI does not allow selecting locked coins). So those inputs already only consists of coins that are in a valid state, and hence any additional checks for those valid states are redundant. So HasSelected + fAllowOtherInputs=false is not materially useful and we could just unify both parameters to what m_add_inputs=false does (with the optimization to skip running the selection algos) and users would not observe any difference.

furszy · 2022-05-18T13:36:46Z

In other words, the usage of m_add_inputs=true was telling the wallet: "add this outputs as inputs into the tx; don't care if they are spent, locked or whatever state they are. Just add them".
While HasSelected() + fAllowOtherInputs=false is a "add this outputs as inputs into the tx only if they are in a valid state, not locked, with enough depth, etc".

I don't think this is a useful distinction.

The only ways to manually set inputs are through send, fundrawtransaction, walletcreatefundedpsbt, and the GUI. Those 3 RPCs all use the same FundTransaction function which currently does fAllowOtherInputs = true, so they always add the specified inputs regardless of state. The GUI only allows users to preset input that are shown in a list - a list which is created by doing AvailableCoins (with the addition of locked coins, but the GUI does not allow selecting locked coins). So those inputs already only consists of coins that are in a valid state, and hence any additional checks for those valid states are redundant. So HasSelected + fAllowOtherInputs=false is not materially useful and we could just unify both parameters to what m_add_inputs=false does (with the optimization to skip running the selection algos) and users would not observe any difference.

I think that, with different words, we are saying the same thing and heading into the same direction.

Let’s try to split the GUI from the RPC flow.

There is a small misunderstanding there; the distinction that made was merely to describe how the current process is working:

You can provide locked and already spent inputs to walletcreatefundedpsbt which are (1) skipped on the AvailableCoins process (not appended to vCoins), and then (2) added into the tx in the second part of SelectCoins (which basically skips all the coin checks that are performed inside AvailableCoins). And this is even being used/tested inside rpc_psbt.py.

This was happening because of the entangled m_add_inputs=false + fAllowOtherInputs=true sets.

So, essentially, in the current flow in master, if you provide inputs to walletcreatefundedpsbt, those inputs are not being fetched inside AvailableCoins, they are fetched and appended to the result inside SelectCoins.

Now, based on that, the point that tried to make in my previous comment is that in order to unify both values into fAllowOtherInputs without adding the m_use_manual_selection flag, we need to add two more commits 5c81571 and 0561612: The reason is that locked coins and spent coins are not in vCoins at SelectCoins time (they are skipped inside AvailableCoins) so they cannot be selected in the first part of SelectCoins (which differs from the second part, as it does not search for the user selected outputs inside the wallet).

Update Summary:

Pushed the m_use_manual_selection removal and added the two commits 5c81571 and 0561612 that are a requirement in order to keep the same functionality for walletcreatefundedpsbt.

Otherwise the command will not support the selection of locked and spent coins (note: whether we accept or not the selection of spent coins on walletcreatefundedpsbt shouldn’t be part of this PR discussion. It’s what we currently have in master and this PR is focused on cleaning the weird/bad flows entanglements first without introducing any behavior change. I could tackle that one later on a follow-up PR).

Now.. if we are sync up to this point we can move onto the final topic, there is still one more behavior change that I need to cover and test after the m_use_manual_selection flag removal :) . Because, as we aren’t going to use the second part of SelectCoins anymore after this changes, the flow isn’t adding the coin control selected external outputs to the selection result anymore (The coin_control.GetExternalOutput stuff inside SelectCoins).

I was leaving that work for a follow-up PR (have a branch locally that removes/refactor all the output search functionality out from SelectCoins), but.. let’s see if I can add a small version of it here and prepare test coverage for it as well (we currently don't have coverage for this stuff).

achow101 · 2022-05-18T14:48:40Z

Okay, I think we're on the same page now.

I think it's actually sufficient to drop the iteration of vCoins in the HasSelected + fAllowOtherInputs = true if-block (https://github.com/bitcoin/bitcoin/blob/629e250cbdee84c20d362da845d7aacfb84ddabe/src/wallet/spend.cpp#L429L444) and move it to after we go through vPresetInputs (after https://github.com/bitcoin/bitcoin/blob/629e250cbdee84c20d362da845d7aacfb84ddabe/src/wallet/spend.cpp#L447L492). This would mean that we will add all pre-selected inputs regardless of spent or locked, and do all of the necessary lookups to compute their sizes and fees. This would obviate the need for additional changes to AvailableCoins.

…block below the selected outputs lookup. Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins. Full explanation is inside bitcoin#25118 comments but brief summary is: `vCoins` at `SelectCoins` time could not be containing the user manually selected input because, even when they were selected by the user, the current `AvailableCoins` flow is skipping 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`.

furszy · 2022-05-19T13:50:27Z

Yeah achow101 that is an option as well 👍🏼.

I briefly thought on it at the beginning but.. as the end goal is decouple/remove the whole lookup responsibility from SelectCoins ( 8000 flow wise, it should had never been there), I headed into that direction directly.. the performance and flow improvements were very attractive to pack in a single PR hehe.

But yeah.. I agree with you. Let's go with that option in this PR so it's properly scoped to unify the fAllowOtherInputs/m_add_inputs concepts. Then will keep moving forward towards the end goal on following up PRs. One step at time.

Changes pushed.

achow101

ACK 4be0887

src/wallet/coincontrol.h

achow101 · 2022-05-23T16:18:13Z

re-ACK 9bb6f04

furszy · 2022-06-06T13:23:52Z

hey @Sjors (sorry for the ping, didn't find you on IRC), you might be interested on this.

laanwj · 2022-06-08T10:12:27Z

i get a build error with this merged to master:

…/bitcoin/src/wallet/test/coinselector_tests.cpp: In member function ‘void wallet::coinselector_tests::bnb_search_test::test_method()’:
…/bitcoin/src/wallet/test/coinselector_tests.cpp:395:22: error: ‘class wallet::CCoinControl’ has no member named ‘fAllowOtherInputs’; did you mean ‘m_allow_other_inputs’?
  395 |         coin_control.fAllowOtherInputs = true;
      |                      ^~~~~~~~~~~~~~~~~
      |                      m_allow_other_inputs

…ected inputs lookup Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins. Full explanation is inside bitcoin#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`.

furszy · 2022-06-08T13:16:48Z

great, thanks @laanwj 👌🏼. Rebased on master, silent issue fixed.

Seeking to make the `CoinControl` option less confusing/redundant. In bitcoin#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).

…ected inputs lookup Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins. Full explanation is inside bitcoin#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`.

…lookup Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins. Full explanation is inside bitcoin#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`.

-BEGIN VERIFY SCRIPT- sed -i 's/fAllowOtherInputs/m_allow_other_inputs/g' -- $(git grep --files-with-matches 'fAllowOtherInputs') -END VERIFY SCRIPT-

furszy · 2022-06-20T02:12:40Z

rebased, conflicts solved. Ready to go.

laanwj · 2022-06-20T19:30:35Z

Code review ACK d338712

beirut-boop · 2023-03-23T13:28:20Z

Noticing some new irrational behavior in a forked coin control and this is the most significant work on coin control pulled from upstream. @furszy any change you could glance over the screenshots and assess the probability of this being an upstream issue?

fanquake · 2023-03-23T13:40:47Z

Sorry, this is not a support forum for forks of this project.

furszy · 2023-03-23T13:48:22Z

@beirut-boop not because of this PR, but check #26699.

beirut-boop · 2023-03-23T14:07:30Z

@furszy thank you, appreciated! 👍

@fanquake I understand but the implication would be that there could be a bug in this project. If @furszy had responded differently after a quick glance, my next step would be to create a test case using Bitcoin Qt.

furszy · 2023-03-23T14:21:59Z

@fanquake I understand but the implication would be that there could be a bug in this project. If @furszy had responded differently after a quick glance, my next step would be to create a test case using Bitcoin Qt.

What fanquake pointed out, which I agree, is about linking other repositories issues here. It isn't the best because we don't know what you have there (nor people are going to get deeper trying to find it). Time is a scarce resource for everyone.

But happy to help if you create an issue running bitcoin-core and describing your problematic next time.

Also, happy to get more eyes on #26699. Feel welcome to write your feedback there.

beirut-boop · 2023-03-23T14:56:26Z

@furszy I understand, concur, and have removed the link from my comment.

Time is a scarce resource for everyone.

I agree. That's why I thought a quick assessment on the observed issue with the coin control classes before spending time setting up a Bitcoin testnet wallet with the intention of taking up more of your time to look into it was a good approach. I'm not affiliated with either project, but rely on both. The project is different but the coin control classes are identical, so I assumed I was helping everyone.

Apologies. I will not cross contaminate the comment section again in the future. 👍

laanwj added the Wallet label May 12, 2022

This was referenced May 13, 2022

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Merged

wallet: do not count wallet utxos as external #24649

Merged

furszy force-pushed the 2022_coinControl_unify_allowOtherInputs branch from 9e84363 to 0561612 Compare May 18, 2022 12:21

furszy force-pushed the 2022_coinControl_unify_allowOtherInputs branch from 0561612 to 4be0887 Compare May 19, 2022 13:49

achow101 reviewed May 19, 2022

View reviewed changes

src/wallet/coincontrol.h Outdated Show resolved Hide resolved

This was referenced May 19, 2022

wallet: Improve AvailableCoins performance by reducing duplicated operations #24699

Merged

wallet: avoid mixing different OutputTypes during coin selection #24584

Merged

rpc: Filter inputs by type during CoinSelection #25183

Closed

This was referenced Jun 2, 2022

wallet: re-activate "AmountWithFeeExceedsBalance" error #25269

Open

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

furszy force-pushed the 2022_coinControl_unify_allowOtherInputs branch 2 times, most recently from 1dd1016 to 68dab89 Compare June 8, 2022 13:16

DrahtBot added the Needs rebase label Jun 16, 2022

furszy force-pushed the 2022_coinControl_unify_allowOtherInputs branch from 68dab89 to d5c5edb Compare June 19, 2022 23:31

furszy added 3 commits June 19, 2022 20:32

refactor: use GetWalletTx in SelectCoins instead of access mapWallet

8dea74a

scripted-diff: rename fAllowOtherInputs -> m_allow_other_inputs

d338712

-BEGIN VERIFY SCRIPT- sed -i 's/fAllowOtherInputs/m_allow_other_inputs/g' -- $(git grep --files-with-matches 'fAllowOtherInputs') -END VERIFY SCRIPT-

furszy force-pushed the 2022_coinControl_unify_allowOtherInputs branch from d5c5edb to d338712 Compare June 19, 2022 23:33

DrahtBot removed the Needs rebase label Jun 20, 2022

laanwj merged commit bc28ca3 into bitcoin:master Jun 20, 2022

furszy deleted the 2022_coinControl_unify_allowOtherInputs branch March 23, 2023 14:54

furszy mentioned this pull request Apr 3, 2023

Mark "too-long-mempool-chain error msg" completed NYDIG/bitcoin-task-bounty#32

Merged

bitcoin locked and limited conversation to collaborators Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: unify “allow/block other inputs“ concept #25118

wallet: unify “allow/block other inputs“ concept #25118

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wallet: unify “allow/block other inputs“ concept #25118

wallet: unify “allow/block other inputs“ concept #25118

Uh oh!

Conversation

Uh oh!

Changes

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Update Summary:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!