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

instagibbs commented at 7:00 pm on March 16, 2023: member

and when no inputs are pre-selected.

triggered via:

walletcreatefundedpsbt ‘[]’ ‘[{“data”: “deadbeef”}]’ 0 ‘{“fee_rate”: “0”}’

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

DrahtBot commented at 7:00 pm on March 16, 2023: contributor

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.

instagibbs renamed this:
~~Fix fund transaction case at 0-value, 0-fee~~
Fix fund transaction crash when at 0-value, 0-fee
on Mar 16, 2023

instagibbs renamed this:
~~Fix fund transaction crash when at 0-value, 0-fee~~
RPC: Fix fund transaction crash when at 0-value, 0-fee
on Mar 16, 2023

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

fanquake requested review from josibake on Mar 19, 2023

fanquake requested review from furszy on Mar 19, 2023

in src/wallet/spend.cpp:921 in d7cc503843

916@@ -917,6 +917,12 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
917     const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.m_subtract_fee_outputs ? 0 : coin_selection_params.tx_noinputs_size);
918     CAmount selection_target = recipients_sum + not_input_fees;
919 
920+    // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
921+    // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways

josibake commented at 11:05 am on March 20, 2023:

grammar nit:

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

josibake approved

josibake commented at 11:11 am on March 20, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27271/commits/d7cc503843769b789dc5f031b4ddc75d555ba980

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.

maflcko commented at 12:23 pm on March 20, 2023: member

Does this need backport?

fanquake commented at 12:25 pm on March 20, 2023: member

Does this need backport?

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

furszy commented at 2:58 pm on March 20, 2023: member

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, 187ce369ff2409d08de6af5cd2144b4d3e714488 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.

0walletcreatefundedpsbt(inputs=[],
1                       outputs=[{"data": "deadbeef"}],
2                       locktime=0,
3                       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 https://github.com/furszy/bitcoin-core/commit/07451919c2f233e82ab7eb021a3aec5a08b35e0a), 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 commented at 4:27 pm on March 20, 2023: member

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 commented at 10:12 pm on March 20, 2023: member

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 commented at 11:59 am on March 21, 2023: member

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:

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

furszy commented at 12:11 pm on March 21, 2023: member

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:
0if (selection_target == 0 && !coin_control.HasSelected() && !add_inputs)

Yep, re-check the “To Summarize” part of my first message :) #27271#pullrequestreview-1348743895. That is what the commit that shared there is doing: https://github.com/furszy/bitcoin-core/commit/07451919c2f233e82ab7eb021a3aec5a08b35e0a

josibake commented at 12:34 pm on March 21, 2023: member

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:
0if (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 commented at 12:51 pm on March 21, 2023: member

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 commented at 2:19 pm on March 22, 2023: member

@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 commented at 2:49 pm on March 22, 2023: member

Crashes sucks code ACK d7cc5038

achow101 commented at 4:46 pm on March 22, 2023: member

ACK d7cc503843769b789dc5f031b4ddc75d555ba980

achow101 merged this on Mar 22, 2023

achow101 closed this on Mar 22, 2023

sidhujag referenced this in commit 197d9c6fc1 on Mar 23, 2023

bitcoin locked this on Mar 21, 2024

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

Reviews