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

Make it possible to create a sweep transaction using createspend #821

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

darosior
Copy link
Member

We add a way to specify what change address to use. This allows the caller to set an external address which has for effect to sweep all the inputs' value to this address, after deduction of the fees and the other (optionally) set destination addresses.

For instance, combined with a self-send (no other destination) and by setting all the wallet's unspent coins, this allows one to sweep all the funds of the wallet to an external address.

@darosior
Copy link
Member Author

I should either explicit in the doc that a sweep address will never be treated as internal (no BIP32 derivs set in PSBT output) or detect internal addresses passed.

Copy link
Member

@edouardparis edouardparis left a comment

Choose a reason for hiding this comment

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

Seems nice !

doc/API.md Outdated Show resolved Hide resolved
src/commands/mod.rs Outdated Show resolved Hide resolved
@darosior darosior force-pushed the 2311_sweep_create_spend branch from 6dc8872 to acc0ac9 Compare November 21, 2023 10:22
src/commands/mod.rs Outdated Show resolved Hide resolved
We leverage the change logic for this. By making it possible to set the
change address to an external address, one can send all the value from
the inputs to this address.
@darosior darosior force-pushed the 2311_sweep_create_spend branch from acc0ac9 to 6bd6218 Compare November 24, 2023 12:52
@darosior
Copy link
Member Author

Rebased and implemented a check for an externally provided address: if we detect it as one of ours we'll set the BIP32 derivations in the PSBT output accordingly (so signers consider it as such) and if it's a change address, we'll check if its derivation index isn't higher than our current next change index. If it is we'll update it.

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

ACK 6bd6218.

received_coins = lianad.rpc.listcoins(["confirmed"])["coins"]
spent_coin = next(c for c in received_coins if c["amount"] == 0.5 * COIN)
destinations = {
"bcrt1qmm5t0ch7vh2hryx9ctq3mswexcugqe4atkpkl2tetm8merqkthas3w7q30": int(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Was there a reason to hardcode the address instead of using bitcoind.rpc.getnewaddress()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember. 😅

@darosior darosior merged commit 7bfc538 into wizardsardine:master Nov 24, 2023
18 checks passed
@darosior darosior deleted the 2311_sweep_create_spend branch November 24, 2023 17:25
darosior added a commit that referenced this pull request Dec 8, 2023
71fd9c4 gui: enable use of RBF on pending transactions (jp1ac4)
ce50dd8 gui: optionally filter spend transactions by txids (jp1ac4)
4846a0b gui: update liana dependency (jp1ac4)

Pull request description:

  This is to resolve #43.

  When viewing an unconfirmed transaction, a user can now either bump its fee or cancel it. This will generate a new PSBT that the user can jump to in order to sign and broadcast it.

  I haven't added any comparison between the previous and replacement inputs as suggested in #43 (comment). I think that would be a bigger change and might be better as a follow-up.

  I decided not to add "Unconfirmed" on the transaction screen as suggested in #43 (comment) as I thought it might be better as a separate PR.

  I haven't yet added the RBF buttons to the home screen, but that could also be done as a follow-up :)

  In a separate commit, I pass `None` to `create_spend` following #821.

ACKs for top commit:
  darosior:
    tested-but-not-review ACK 71fd9c4
  edouardparis:
    ACK 71fd9c4

Tree-SHA512: c3ddbb85ad008e9e450b79ba77816ad9065f1eec675913f20463c4271ff017d5cb9ff0a0fca9ed919c97b3f6bb2b806344dc4ff062f5393ad5ac85c8c039ab83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants