Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fee-bump to example-cli example crate #1111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sjeohp
Copy link
Contributor

@sjeohp sjeohp commented Sep 5, 2023

Description

Opening for feedback.

This adds a run_bump_cmd to example_cli and tweaks nursery/coin_select/src/coin_selector.rs to allow required utxos.

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • [ *] I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sjeohp. Left some comments. It will take some time to get this feature in since there's a lot in the pipline atm.

cs_algorithm,
Some(new_feerate),
tx.output,
original_outpoints,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need to spend one of the original outpoints to RBF the transaction here.

if !tx
.input
.iter()
.any(|txin| txin.sequence.to_consensus_u32() <= 0xFFFFFFFD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the sequence type might have a helper method for this check. I would just emit a warning here rather than error out.

.map(|txin| txin.previous_output)
.collect::<Vec<_>>();

if tx.output.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid this "figure out which one is change" algorithm. Instead the API should be that we want to double spend a transaction (or even set of transactions) with a new spend. We don't need to enforce that the new spend has the same txouts as the old one. If you want to do ordinary RBF you just put the same destination address in that you used to create the original transaction. This means the application is responsible for recalling the intent of any particular transaction when they want to RBF it so that the new transaction's effect matches the old one.

So to be clear I think the CLI interface should just be to add a --replace <txid> flag to the send command which would just make sure that it spends at least one input of any --replace transaction and has a higher feerate and higher absolute fee than that transaction.

let selected_txos = selection.apply_selection(&candidates).collect::<Vec<_>>();
let selected_txos = selection
.apply_selection(&candidates)
.chain(required.iter())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So required utxos should just be selected in the coin_selector (then apply_selection is bound to include it).

@notmandatory notmandatory added this to the 1.0.0-beta.0 milestone Nov 13, 2023
@notmandatory
Copy link
Member

I put this in the beta.0 milestone since it's only changing example code.

@notmandatory notmandatory added documentation Improvements or additions to documentation module-wallet labels Mar 18, 2024
@notmandatory notmandatory removed this from the 1.0.0-beta milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation module-wallet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants