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.

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.

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:

1. 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.
2. 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).
3. The locked coins skip/add can be customizable by the user at selection time.

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.

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

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.

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.

re-ACK 9bb6f042ee157d07ae169e0195d4a660d3208cad

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

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

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

rebased, conflicts solved. Ready to go.

Code review ACK d338712

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?

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

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

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

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