RPC: Fix fund transaction crash when at 0-value, 0-fee #27271

instagibbs · 2023-03-16T19:00:10Z

and when no inputs are pre-selected.

triggered via:

walletcreatefundedpsbt '[]' '[{"data": "deadbeef"}]' 0 '{"fee_rate": "0"}'

DrahtBot · 2023-03-16T19:00:15Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake, furszy, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

josibake

ACK d7cc503

Left a small grammar nit for the comment. Also, it would be really nice to have the src/wallet/spend.cpp change in one commit and the functional test change in a separate commit. Makes it much easier to verify that the functional test actually catches the bug, although not really a big deal here since the change is so small.

To test, I deleted your change to src/wallet/spend.cpp and verified that the functional test does fail.

josibake · 2023-03-20T11:05:56Z

src/wallet/spend.cpp

+    // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
+    // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways


grammar nit:

Suggested change

// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)

// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways

// This can only happen if the feerate is 0, the requested destinations are value of 0 (e.g OP_RETURN),

// and there are no pre-selected inputs. This will result in a 0-input transaction, which is consensus-invalid

maflcko · 2023-03-20T12:23:31Z

Does this need backport?

fanquake · 2023-03-20T12:25:22Z

Does this need backport?

I think it's not-super-required but could be nice if we do.

furszy

Good catch.

I agree to fail when the user disallowed automatic coin selection (add_inputs=false) but not seeing why we should fail when it's enabled. The wallet can craft the tx by selecting any available coin.

Coin selection wise, when target=0, bnb fails, knapsack should return the first UTXO above the target (so the smallest one in the wallet) and SRD pick a random one.

The problem is that knapsack has a bug, and it returns an empty selection (while it thinks that is a valid one..), so then the process crashes at waste calculation because there are no inputs on an apparent valid selection result.

Happily, 187ce36 indirectly fixes part of it too (which is being introduced in #26720).

In the future, with that bugfix, plus a little change in SeletCoins to not return an empty selection when there are no preset inputs, we can enable automatic coin selection for 0 target lookups (which I think that is the right path to follow as Coin Selection must never return an invalid selection while it thinks that is a valid one; worst case scenario should be fail with an error message).

Also, the crash can happen without customizing the feerate, when there is only an OP_RETURN output with SFFO enabled.

e.g.

walletcreatefundedpsbt(inputs=[],
                       outputs=[{"data": "deadbeef"}],
                       locktime=0,
                       options={"subtractFeeFromOutputs": [0]})

To summarize:

What I would say to do here is fix and test that we don't crash when add_inputs=false for feerate=0 and SFFO (in order to do this, could squash furszy@0745191), then fix the cases when add_inputs=true post #26720 when knapsack will no longer return an empty selection as a valid result.

What do you think?

josibake · 2023-03-20T16:27:52Z

Good catch.

I agree to fail when the user disallowed automatic coin selection (add_inputs=false) but not seeing why we should fail when it's enabled. The wallet can craft the tx by selecting any available coin.

What scenario are you envisioning where we have a selection_target of 0, a fee rate of 0, and no pre-selected inputs that we would then want to call SelectCoins? With how it currently works, this would lead to SelectCoins assuming we DID have pre-selected and trying to continue, which is definitely wrong: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L604-L609

I agree with the points you raised as other areas we can improve the wallet (i.e Knapsack bug, SelectCoins not returning empty sets), but I think this error check is still necessary even with those other changes.

If the selection target is zero, the fee rate is zero, and there are no preset inputs, I'd prefer to NOT run SelectCoins in that case, which is what this PR does.

Also, the crash can happen without customizing the feerate, when there is only an OP_RETURN output with SFFO enabled.

this feels like a separate bug that we should catch waaaay before calling CreateTransactionInternal. If you have all 0-valued outputs and then say "pay for the fee from this/these outputs", that's a non-sensical request which we should be able to catch right away and alert the user

furszy · 2023-03-20T22:12:03Z

What scenario are you envisioning where we have a selection_target of 0, a fee rate of 0, and no pre-selected inputs that we would then want to call SelectCoins? With how it currently works, this would lead to SelectCoins assuming we DID have pre-selected and trying to continue, which is definitely wrong: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L604-L609

That is the "little change" that needs to be done that I mentioned above.

The simplest scenario is what is currently being tested here. Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.
Force the user to provide an input to craft a single op_return output tx isn't the best, coin selection is an error-prone process if you do it by hand (users will probably pick one at random, one that is not under their best interests). Better to let the wallet pick the best UTXO for the transaction that the user wants to create.

josibake · 2023-03-21T11:59:22Z

Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.

ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).

Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend we merge this change now and once the other changes are complete, we can update this check to something like:

if (selection_target == 0 && !coin_control.HasSelected() && !add_inputs)

furszy · 2023-03-21T12:11:35Z

Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.

ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).

Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend we merge this change now and once the other changes are complete, we can update this check to something like:
if (selection_target == 0 && !coin_control.HasSelected() && !add_inputs)

Yep, re-check the "To Summarize" part of my first message :) #27271 (review).
That is what the commit that shared there is doing: furszy@0745191

josibake · 2023-03-21T12:34:43Z

Single op_return output, automatic coin selection enabled. The wallet just need to select an UTXO to craft the tx.

ah, I think I see your point. In the case of a 0-valued OP_RETURN, and fee rate of 0, and automatic coin-selection enabled, you would want the wallet to pick a single UTXO (which gets sent entirely to the change output).
Given that there are a few moving pieces to enable the use case you are talking about (fixing Knapsack, SelectCoins, etc), I'd recommend we merge this change now and once the other changes are complete, we can update this check to something like:
if (selection_target == 0 && !coin_control.HasSelected() && !add_inputs)
Yep, re-check the "To Summarize" part of my first message :) #27271 (review). That is what the commit that shared there is doing: furszy@0745191

We are proposing different things: I am saying don't add the check for !m_allow_other_inputs now, and instead merge the PR as is. Once we have fixed coin selection to handle this edge case, we add the check for !m_allow_other_inputs and update the error message in a separate PR.

With what you are proposing in furszy@0745191, the node will still crash after this PR is merged if someone passes add_inputs: True

furszy · 2023-03-21T12:51:01Z

k sure. I didn't suggest that because the error message will make people select the input/s manually etc. Which, I'm not so sure that it's better than a crash (at least the crash prevent them from making mistakes).

But.. I'm not that strong over that point. So, all good, either way works for me. The coin selection fixes are already up anyway, matter of continue moving forward until all the problems are corrected, so this can be enabled.

instagibbs · 2023-03-22T14:19:55Z

@furszy is that an ACK? :)

I don't think crashing is acceptable. Let's fix the coin selection functionality later, and make sure we're not crashing LN nodes or whatever.

furszy

Crashes sucks code ACK d7cc503

achow101 · 2023-03-22T16:46:12Z

ACK d7cc503

… 0-fee d7cc503 Fix fund transaction case at 0-value, 0-fee (Greg Sanders) Pull request description: and when no inputs are pre-selected. triggered via: walletcreatefundedpsbt '[]' '[{"data": "deadbeef"}]' 0 '{"fee_rate": "0"}' ACKs for top commit: achow101: ACK d7cc503 josibake: ACK bitcoin@d7cc503 furszy: Crashes sucks code ACK d7cc503 Tree-SHA512: 3f5e10875666aaf52c11d6a38b951aa75d0cbe684cc7f904e199f7a864923bf31d03a654687f8b746cae0eebb886a799bff2c6d200699438480d4c0ff8785f3a

Fix fund transaction case at 0-value, 0-fee

d7cc503

instagibbs changed the title ~~Fix fund transaction case at 0-value, 0-fee~~ Fix fund transaction crash when at 0-value, 0-fee Mar 16, 2023

instagibbs changed the title ~~Fix fund transaction crash when at 0-value, 0-fee~~ RPC: Fix fund transaction crash when at 0-value, 0-fee Mar 16, 2023

DrahtBot added the RPC/REST/ZMQ label Mar 16, 2023

instagibbs mentioned this pull request Mar 16, 2023

Pseudo ephemeral anchors bitcoin-inquisition/bitcoin#23

Merged

dergoegge mentioned this pull request Mar 16, 2023

Increase fuzz coverage in the wallet #27272

Closed

fanquake requested review from josibake and furszy March 19, 2023 12:16

josibake approved these changes Mar 20, 2023

View reviewed changes

furszy reviewed Mar 20, 2023

View reviewed changes

furszy reviewed Mar 22, 2023

View reviewed changes

achow101 merged commit fc7c21f into bitcoin:master Mar 22, 2023

bitcoin locked and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RPC: Fix fund transaction crash when at 0-value, 0-fee #27271

RPC: Fix fund transaction crash when at 0-value, 0-fee #27271

Uh oh!

instagibbs commented Mar 16, 2023

Uh oh!

DrahtBot commented Mar 16, 2023 •

edited

Loading

Uh oh!

josibake left a comment

Uh oh!

josibake Mar 20, 2023

Uh oh!

maflcko commented Mar 20, 2023

Uh oh!

fanquake commented Mar 20, 2023

Uh oh!

furszy left a comment

Uh oh!

josibake commented Mar 20, 2023 •

edited

Loading

Uh oh!

furszy commented Mar 20, 2023 •

edited

Loading

Uh oh!

josibake commented Mar 21, 2023

Uh oh!

furszy commented Mar 21, 2023

Uh oh!

josibake commented Mar 21, 2023

Uh oh!

furszy commented Mar 21, 2023

Uh oh!

instagibbs commented Mar 22, 2023 •

edited

Loading

Uh oh!

furszy left a comment

Uh oh!

achow101 commented Mar 22, 2023

Uh oh!

Uh oh!

		// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
		// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways

RPC: Fix fund transaction crash when at 0-value, 0-fee #27271

RPC: Fix fund transaction crash when at 0-value, 0-fee #27271

Uh oh!

Conversation

instagibbs commented Mar 16, 2023

Uh oh!

DrahtBot commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

josibake Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 20, 2023

Uh oh!

fanquake commented Mar 20, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

josibake commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented Mar 21, 2023

Uh oh!

furszy commented Mar 21, 2023

Uh oh!

josibake commented Mar 21, 2023

Uh oh!

furszy commented Mar 21, 2023

Uh oh!

instagibbs commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 22, 2023

Uh oh!

Uh oh!

DrahtBot commented Mar 16, 2023 •

edited

Loading

josibake commented Mar 20, 2023 •

edited

Loading

furszy commented Mar 20, 2023 •

edited

Loading

instagibbs commented Mar 22, 2023 •

edited

Loading