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

furszy commented at 2:49 PM on May 12, 2022: member

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.

laanwj added the label Wallet on May 12, 2022

DrahtBot commented at 5:09 PM on May 13, 2022: contributor

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 commented at 7:21 PM on May 16, 2022: member

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 commented at 9:00 PM on May 16, 2022: member

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 https://github.com/furszy/bitcoin-core/commit/1291ba7d2463c062242b82df879fba8ca29ba627 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 commented at 9:46 PM on May 16, 2022: member

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 force-pushed on May 18, 2022

furszy commented at 1:36 PM on May 18, 2022: member

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 commented at 2:48 PM on May 18, 2022: member

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.

furszy referenced this in commit 18bd0564f6 on May 19, 2022

furszy force-pushed on May 19, 2022

furszy commented at 1:50 PM on May 19, 2022: member

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 (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.

in src/wallet/coincontrol.h:40 in 4be088798d outdated

  38 |      //! If false, only safe inputs will be used
  39 |      bool m_include_unsafe_inputs = false;
  40 | -    //! If false, allows unselected inputs, but requires all selected inputs be used
  41 | +    //! If true, the selection process can add extra unselected inputs from the wallet
  42 | +    //! while requires all selected inputs be used
  43 |      bool fAllowOtherInputs = false;

achow101 commented at 4:40 PM on May 19, 2022:

nit: It would be nice if we could switch to using the new naming scheme. Perhaps a scripted diff?

furszy commented at 5:47 PM on May 19, 2022:

pushed

achow101 commented at 4:40 PM on May 19, 2022: member

ACK 4be088798dbb74380e6b07fb87569d83a1d02f1d

achow101 commented at 4:18 PM on May 23, 2022: member

re-ACK 9bb6f042ee157d07ae169e0195d4a660d3208cad

furszy commented at 1:23 PM on June 6, 2022: member

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

laanwj commented at 10:12 AM on June 8, 2022: member

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

furszy referenced this in commit dd6df7bc4d on Jun 8, 2022

furszy force-pushed on Jun 8, 2022

furszy commented at 1:16 PM on June 8, 2022: member

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

DrahtBot added the label Needs rebase on Jun 16, 2022

wallet: unify “allow/block other inputs“ concept

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).

25749f1df7

furszy referenced this in commit 95216b4c37 on Jun 19, 2022

furszy force-pushed on Jun 19, 2022

wallet: move "use-only coinControl inputs" below the selected 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 #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`.

b4e2d4d4ee

refactor: use GetWalletTx in SelectCoins instead of access mapWallet 8dea74a8ff

scripted-diff: rename fAllowOtherInputs -> m_allow_other_inputs

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

d338712886

furszy force-pushed on Jun 19, 2022

DrahtBot removed the label Needs rebase on Jun 20, 2022

furszy commented at 2:12 AM on June 20, 2022: member

rebased, conflicts solved. Ready to go.

laanwj commented at 7:30 PM on June 20, 2022: member

Code review ACK d33871288636d9e4cdde2e1430de1f4223e2dd3f

laanwj merged this on Jun 20, 2022

laanwj closed this on Jun 20, 2022

beirut-boop commented at 1:28 PM on March 23, 2023: none

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 commented at 1:40 PM on March 23, 2023: member

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

furszy commented at 1:48 PM on March 23, 2023: member

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

beirut-boop commented at 2:07 PM on March 23, 2023: none

@furszy thank you, appreciated! :+1: @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 commented at 2:21 PM on March 23, 2023: member

@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.

furszy deleted the branch on Mar 23, 2023

beirut-boop commented at 2:56 PM on March 23, 2023: none

@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. :+1:

bitcoin locked this on Mar 22, 2024