gui: enable use of RBF on unconfirmed transactions #852

jp1ac4 · 2023-12-06T20:19:14Z

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.

darosior

Trying to cancel a transaction. Nothing special about this transaction (1 in 1 out), maybe except that its parent was unconfirmed too.

2023-12-07T12:35:56.319256Z ERROR liana:55: panic occurred at line 126 of file src/app/state/transactions.rs: Some("fee_amount must be set for pending transaction")
   0: liana::setup_panic_hook::{{closure}}
   1: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/alloc/src/boxed.rs:2021:9
      std::panicking::rust_panic_with_hook
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:711:13
   2: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:599:13
   3: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/sys_common/backtrace.rs:170:18
   4: rust_begin_unwind
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:595:5
   5: core::panicking::panic_fmt
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:67:14
   6: core::panicking::panic_display
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:150:5
      core::panicking::panic_str
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:134:5
      core::option::expect_failed
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/option.rs:1988:5
   7: <liana_gui::app::state::transactions::TransactionsPanel as liana_gui::app::state::State>::update
   8: liana_gui::app::App::update
   9: <iced::application::Instance<A> as iced_native::program::Program>::update
  10: iced_winit::application::update
  11: iced_glutin::application::run::{{closure}}
  12: winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run_return
  13: iced_glutin::application::run
  14: liana_gui::main
  15: std::sys_common::backtrace::__rust_begin_short_backtrace
  16: std::rt::lang_start::{{closure}}
  17: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/ops/function.rs:284:13
      std::panicking::try::do_call
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:502:40
      std::panicking::try
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:466:19
      std::panic::catch_unwind
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal::{{closure}}
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/rt.rs:148:48
      std::panicking::try::do_call
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:502:40
      std::panicking::try
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panicking.rs:466:19
      std::panic::catch_unwind
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/panic.rs:142:14
      std::rt::lang_start_internal
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/rt.rs:148:20
  18: main
  19: __libc_start_call_main
             at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  20: __libc_start_main_impl
             at ./csu/../csu/libc-start.c:360:3
  21: _start

darosior · 2023-12-07T10:01:34Z

gui/src/daemon/embedded.rs

@@ -89,7 +89,7 @@ impl Daemon for EmbeddedDaemon {
        feerate_vb: u64,
    ) -> Result<CreateSpendResult, DaemonError> {
        self.control()?
-            .create_spend(destinations, coins_outpoints, feerate_vb)
+            .create_spend(destinations, coins_outpoints, feerate_vb, None)


Looks like this should be in the previous commit otherwise it wouldn't compile?

Ah yes, I'll squash it into the first commit.

This has now been done.

darosior · 2023-12-07T13:04:09Z

There is a small lag after clicking on "go to replacement" and being shown the tx details screen. What is it due to? Can't we have a load spinner of some sort if it's unavoidable? Since currently it's showing the list of transactions it's a bit confusing.

We can click on "Bump fees" -- "Cancel tx" even for transactions made from another wallet. (Not sure it's worth fixing here if not trivial, see #854.)

Besides, i could successfully cancel an outgoing transaction (as long as the parent is confirmed), cancel the cancel transaction, and bump the cancel of the cancel to a very high feerate such as to need a new coin to pay for the fees. I was correctly shown warning on my Ledger about my insane fees compared to the sent amount.

jp1ac4 · 2023-12-07T13:43:29Z

There is a small lag after clicking on "go to replacement" and being shown the tx details screen. What is it due to?

When the PSBTs screen first loads, it uses spend transactions from the cache, which doesn't yet contain the replacement PSBT. After loading once, it then refreshes the list of spend transactions and finds the replacement to open. I could try forcing it to load new spend transactions on the first load, which will hopefully avoid the list of PSBTs being shown.

This is needed to use the `rbfpsbt` command. Now, `create_spend` has an additional parameter for change address, which we leave as `None`.

jp1ac4 · 2023-12-07T19:30:52Z

Trying to cancel a transaction. Nothing special about this transaction (1 in 1 out), maybe except that its parent was unconfirmed too.

I didn't manage to reproduce this error, but I was wrongly assuming before that fee_amount would be Some, which I now check explicitly. In case it's None, which implies missing coins, the bump fee / cancel buttons won't be shown.

There is a small lag after clicking on "go to replacement" and being shown the tx details screen. What is it due to? Can't we have a load spinner of some sort if it's unavoidable? Since currently it's showing the list of transactions it's a bit confusing.

I've changed it to load the replacement PSBT directly without the intermediate list view. There's still a small lag while it loads all other spends, which I think could be reduced by modifying the list_spend_transactions function in the GUI to accept a &[Txid} parameter. Even though it would still load all spends from the DB, it would only process the required ones so should be a bit faster.

We can click on "Bump fees" -- "Cancel tx" even for transactions made from another wallet. (Not sure it's worth fixing here if not trivial, see #854.)

I don't think this is trivial to fix as currently only confirmed transactions are loaded from the DB. We would need to load also unconfirmed transactions from the DB in order to cross-check our list of pending transactions. It's probably not too difficult either, but would be a larger change and so it may be worth tackling #854 directly instead.

jp1ac4 · 2023-12-07T19:31:54Z

I rebased on master in order to get the changes from #845 when updating the liana dependency.

jp1ac4 · 2023-12-08T00:12:32Z

There's still a small lag while it loads all other spends, which I think could be reduced by modifying the list_spend_transactions function in the GUI to accept a &[Txid} parameter. Even though it would still load all spends from the DB, it would only process the required ones so should be a bit faster.

I've added an optional filter to list_spend_transactions and the replacement PSBT loads much faster now.

jp1ac4 · 2023-12-08T00:43:17Z

gui/src/app/view/transactions.rs

+                        .push(Container::new(p1_bold("Feerate")).padding(10))
+                        .spacing(10)
+                        .push(
+                            form::Form::new_trimmed("", feerate, move |msg| {


It might be better not to trim the feerate value as it's difficult to edit a single-digit value. It's not possible to delete the existing value and enter a new one and we have to instead add a second digit and delete the first.

I agree but since we are close to feature freeze and it's not a deal breaker i'm going to leave this for a follow-up.

darosior · 2023-12-08T10:07:56Z

tested-but-not-review ACK 71fd9c4

edouardparis · 2023-12-08T10:12:34Z

gui/src/daemon/mod.rs

+    // Set `txids` to `None` for no filter (passing an empty slice returns no transactions).
+    fn list_spend_transactions(
+        &self,
+        txids: Option<&[Txid]>,


I am ok with the change, it would be nice to upstream it later in lianad so the command takes a filter on txids.

Most definitely, it was raised here already: #43 (comment). Opened #862 to track this.

edouardparis

ACK 71fd9c4
It is smooth

edouardparis · 2023-12-08T10:39:41Z

gui/src/app/menu.rs

@@ -10,4 +10,5 @@ pub enum Menu {
    CreateSpendTx,
    Recovery,
    RefreshCoins(Vec<OutPoint>),
+    PsbtPreSelected(Txid),


Maybe one day, we would separate list panels from list item modal, like a Menu::PSBTs and Menu::PSBT(txid) and keep all the panels states in memory instead of having one app current state.

darosior reviewed Dec 7, 2023

View reviewed changes

gui: update liana dependency

4846a0b

This is needed to use the `rbfpsbt` command. Now, `create_spend` has an additional parameter for change address, which we leave as `None`.

jp1ac4 force-pushed the gui-add-rbf branch from 2f9568c to 5a698c8 Compare December 7, 2023 19:01

gui: optionally filter spend transactions by txids

ce50dd8

jp1ac4 force-pushed the gui-add-rbf branch from 5a698c8 to d237049 Compare December 8, 2023 00:02

gui: enable use of RBF on pending transactions

71fd9c4

jp1ac4 force-pushed the gui-add-rbf branch from d237049 to 71fd9c4 Compare December 8, 2023 00:39

jp1ac4 commented Dec 8, 2023

View reviewed changes

darosior requested a review from edouardparis December 8, 2023 08:31

darosior mentioned this pull request Dec 8, 2023

GUI: RBF: improve feerate text input #860

Closed

edouardparis reviewed Dec 8, 2023

View reviewed changes

edouardparis approved these changes Dec 8, 2023

View reviewed changes

edouardparis reviewed Dec 8, 2023

View reviewed changes

darosior merged commit 3211045 into wizardsardine:master Dec 8, 2023
18 checks passed

jp1ac4 deleted the gui-add-rbf branch December 8, 2023 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gui: enable use of RBF on unconfirmed transactions #852

gui: enable use of RBF on unconfirmed transactions #852

jp1ac4 commented Dec 6, 2023

darosior left a comment •

edited

Loading

darosior Dec 7, 2023

jp1ac4 Dec 7, 2023

jp1ac4 Dec 7, 2023

darosior commented Dec 7, 2023 •

edited

Loading

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 8, 2023

jp1ac4 Dec 8, 2023

darosior Dec 8, 2023

darosior commented Dec 8, 2023 •

edited

Loading

edouardparis Dec 8, 2023

darosior Dec 8, 2023

edouardparis left a comment

edouardparis Dec 8, 2023

gui: enable use of RBF on unconfirmed transactions #852

gui: enable use of RBF on unconfirmed transactions #852

Conversation

jp1ac4 commented Dec 6, 2023

darosior left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Dec 7, 2023 • edited Loading

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 7, 2023

jp1ac4 commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edouardparis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior left a comment •

edited

Loading

darosior commented Dec 7, 2023 •

edited

Loading

darosior commented Dec 8, 2023 •

edited

Loading