bumpfee: allow send coins back to yourself

furszy commented at 3:13 pm on March 3, 2023: member

Simple example:

User_1 sends 0.1 btc to user_2 on a low fee transaction.
After few hours, the tx is still in the mempool, user_2 is not interested anymore, so user_1 decides to cancel it by sending coins back to himself.
User_1 has the bright idea of opening the explorer and copy the change output address of the transaction. Then call bumpfee providing such output (in the “outputs” arg).

Currently, this is not possible. The wallet fails with “Unable to create transaction. Transaction must have at least one recipient” error. The error reason is because we discard the provided output from the recipients list and set it inside the coin control so the process adds it later (when the change is calculated). But.. there is no later if the tx has no outputs.

DrahtBot commented at 3:13 pm on March 3, 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	ishaanam, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Accept options as named-only parameters by ryanofsky)
#26467 (bumpfee: Allow the user to choose which output is change by achow101)

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.

kristapsk commented at 3:31 pm on March 3, 2023: contributor

Not the scope of this PR, but related - in GUI this should be implemented as another context menu item “Cancel transaction”.

in src/wallet/feebumper.cpp:260 in c707bac6b7 outdated

254+        }
255+
256+        // Add change as recipient with SFFO flag enabled, so fees are deduced from it and no other output is created.
257+        recipients.emplace_back(CRecipient{GetScriptForDestination(new_coin_control.destChange), output_value, /*fSubtractFeeFromAmount=*/true});
258+        new_coin_control.destChange = CNoDestination();
259+    }

ishaanam commented at 0:18 am on March 16, 2023:

In 7cb4dc157a711210e18f3c9b492150b6eb984b30 " bumpfee: enable send coins back to yourself "

While this certainly looks correct, I think there is more concise way of doing this within the outputs loop before this one. I’ve implemented it here if you would like to take a look: https://github.com/ishaanam/bitcoin/commit/5b1017c1ea11c8fe2351a9110e4c6ddad6e77f8a. The advantage of this would be that the sanity check would not be required and imo it would be a bit more readable.

furszy commented at 5:35 pm on March 21, 2023:

Thanks ishaanam. At least for me, it doesn’t look right to check the vector size while we loop over it. It is also not correct to use outputs.size() as outputs will be empty if the user didn’t customize them.

ishaanam commented at 6:22 pm on March 21, 2023:

It is also not correct to use outputs.size() as outputs will be empty if the user didn’t customize them.

In what scenario would we need to make the change output the sole recipient if the user didn’t customize the outputs? My understanding was that this is only an issue because of the addition of the “outputs” vector.

furszy commented at 6:25 pm on March 21, 2023:

a send-to-self, single change output tx that is bumped.

ishaanam commented at 0:19 am on March 16, 2023: contributor

Concept ACK, this seems like a common use case of the outputs argument. Additionally, it would be good to use be able to re-use the change address of the original tx instead of running getnewaddress, since that would effectively be wasting an address.

in src/wallet/feebumper.cpp:257 in 7cb4dc157a outdated

252+            errors.emplace_back(Untranslated("Unable to create transaction. Transaction must have at least one recipient"));
253+            return Result::INVALID_PARAMETER;
254+        }
255+
256+        // Add change as recipient with SFFO flag enabled, so fees are deduced from it and no other output is created.
257+        recipients.emplace_back(CRecipient{GetScriptForDestination(new_coin_control.destChange), output_value, /*fSubtractFeeFromAmount=*/true});

achow101 commented at 11:22 pm on March 17, 2023:

In 7cb4dc157a711210e18f3c9b492150b6eb984b30 “bumpfee: enable send coins back to yourself”

output_value is not guaranteed to match what the user had actually specified for the single output.

furszy commented at 6:13 pm on March 21, 2023:

oops, fixed. And added coverage for it.

furszy force-pushed on Mar 21, 2023

achow101 commented at 6:18 pm on March 22, 2023: member

ACK c260d3fbf4cf8be07176840f39125abcb3d59c1b

furszy requested review from ishaanam on Mar 31, 2023

in src/wallet/feebumper.cpp:257 in 15ecc354a4 outdated

254+            errors.emplace_back(Untranslated("Unable to create transaction. Transaction must have at least one recipient"));
255+            return Result::INVALID_PARAMETER;
256+        }
257+
258+        // Add change as recipient with SFFO flag enabled, so fees are deduced from it.
259+        // If the output differs from the original tx output (because the user customized it) a new change output will be created.

ishaanam commented at 9:35 pm on March 31, 2023:

If I’m understanding this correctly, this means that this could result in the creation of a transaction with 2 outputs that our wallet would identify as change. What would happen if we wanted to bump this transaction again, but keep the outputs the same? Though I haven’t tested this, I think that the following will happen:

The first output is added as new_coin_control.destChange
Then we find the second output is also change and over-write the original destChange with the second output
recipients is empty, so we make recipient the current destChange
destChange is now nothing, so we have lost the first output address.

I suppose it doesn’t really matter, because the money is still going to the same wallet, but usually it is expected for the outputs to be preserved unless outputs is used.

furszy commented at 2:39 pm on April 1, 2023:

Yeah, I started working on it here but ended up moving the work to another branch because it’s not a problem introduced in this PR.

This PR enables the bump of single output transactions that were previously not possible. It’s not changing how we decide which output becomes change.

Anyone can craft a transaction with two outputs sending to change addresses already (just need to call getrawchangeaddress twice) and bump it.

That issue is inside the outputs loop that is above this code that doesn’t check destChange existence prior set. Which well, in the future PR, we will need to decide which change address use if there are many of them.

ishaanam commented at 9:36 pm on March 31, 2023: contributor

ACK c260d3fbf4cf8be07176840f39125abcb3d59c1b

DrahtBot added the label Needs rebase on Apr 15, 2023

bumpfee: enable send coins back to yourself

Simple example:

1) User_1 sends 0.1 btc to user_2 on a low fee transaction.
2) After few hours, the tx is still in the mempool, user_2
   is not interested anymore, so user_1 decides to cancel
   it by sending coins back to himself.
3) User_1 has the bright idea of opening the explorer and
   copy the change output address of the transaction. Then
   call bumpfee providing such output (in the "outputs" arg).

Currently, this is not possible. The wallet fails with
"Unable to create transaction. Transaction must have at least
one recipient" error.
The error reason is that we discard the provided output from
the recipients list and set it inside the coin control
so the process adds it later (when the change is calculated).
But.. there is no later if the tx has no outputs.

7bffec6715

test: bumpfee, add coverage for "send coins back to yourself" be72663a15

furszy force-pushed on Apr 15, 2023

furszy commented at 1:48 pm on April 15, 2023: member

rebased, conflicts solved. Ready to go.

DrahtBot removed the label Needs rebase on Apr 15, 2023

ishaanam commented at 2:11 pm on April 24, 2023: contributor

reACK be72663a1521bc6cdf16d43a4feae7c5b57735c0

DrahtBot requested review from achow101 on Apr 24, 2023

fanquake assigned achow101 on Apr 27, 2023

achow101 commented at 12:32 pm on May 1, 2023: member

ACK be72663a1521bc6cdf16d43a4feae7c5b57735c0

DrahtBot removed review request from achow101 on May 1, 2023

achow101 merged this on May 1, 2023

achow101 closed this on May 1, 2023

sidhujag referenced this in commit e86c70f453 on May 1, 2023

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

bumpfee: allow send coins back to yourself #27195

Reviews

Conflicts