GUI: setting a too high feerate for a Spend crashes the software

[darosior]

We should really avoid careless conversions. Or idk sanitize inputs.

  2023-11-17T13:19:00.521450Z ERROR liana:55: panic occurred at line 278 of file src/app/state/spend/step.rs: Some("attempt to multiply with overflow")                                                            
   0: liana::setup_panic_hook::{{closure}}                                                                                                                                                                         
             at /home/darosior/.cargo/git/checkouts/liana-efb3a908181ef2cf/2d303b1/src/lib.rs:49:18                                                                                                                
   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:597: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                                                                                                                                                                                       
             at /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/core/src/panicking.rs:117:5                                                                                                                
   7: liana_gui::app::state::spend::step::DefineSpend::amount_left_to_select                                                                                                                                       
             at src/app/state/spend/step.rs:278:34                                                                                                                                                                 
   8: <liana_gui::app::state::spend::step::DefineSpend as liana_gui::app::state::spend::step::Step>::update                                                                                                        
             at src/app/state/spend/step.rs:320:29                                                                                                                                                                 
   9: <liana_gui::app::state::spend::CreateSpendPanel as liana_gui::app::state::State>::update                                                                                                                     
             at src/app/state/spend/mod.rs:105:20                                                                                                                                                                  
  10: liana_gui::app::App::update                                                                                                                                                                                  
             at src/app/mod.rs:174:18                                                                                                                                                                              
  11: <liana_gui::GUI as iced::application::Application>::update                                                                                                                                                   
             at src/main.rs:248:17                                                                                                                                                                                 
  12: <iced::application::Instance<A> as iced_native::program::Program>::update                                                                                                                                    
             at /home/darosior/.cargo/registry/src/index.crates.io-6f17d22bba15001f/iced-0.9.0/src/application.rs:227:9                                                                                            
  13: iced_winit::application::update::{{closure}}
             at /home/darosior/.cargo/registry/src/index.crates.io-6f17d22bba15001f/iced_winit-0.9.1/src/application.rs:664:40
  14: iced_futures::backend::native::tokio::<impl iced_futures::executor::Executor for tokio::runtime::runtime::Runtime>::enter                                                                                     
             at /home/darosior/.cargo/git/checkouts/iced-01eedf742052ef90/2d8318b/futures/src/backend/native/tokio.rs:19:9     
  15: iced_futures::runtime::Runtime<Hasher,Event,Executor,Sender,Message>::enter                         
             at /home/darosior/.cargo/git/checkouts/iced-01eedf742052ef90/2d8318b/futures/src/runtime.rs:53:9                                                                                                       
  16: iced_winit::application::update                                                                     
             at /home/darosior/.cargo/registry/src/index.crates.io-6f17d22bba15001f/iced_winit-0.9.1/src/application.rs:664:23                                                                                      

liana/gui/src/app/state/spend/step.rs

Lines 173 to 236 in 514535d

	fn amount_left_to_select(&mut self) {
	// We need the feerate in order to compute the required amount of BTC to
	// select. Return early if we don't to not do unnecessary computation.
	let feerate = match self.feerate.value.parse::<u64>() {
	Ok(f) => f,
	Err(_) => {
	self.amount_left_to_select = None;
	return;
	}
	};
	// The coins to be included in this transaction.
	let selected_coins: Vec<_> = self
	.coins
	.iter()
	.filter_map(|(c, selected)| if *selected { Some(c) } else { None })
	.collect();
	// A dummy representation of the transaction that will be computed, for
	// the purpose of computing its size in order to anticipate the fees needed.
	// NOTE: we make the conservative estimation a change output will always be
	// needed.
	let tx_template = bitcoin::Transaction {
	version: 2,
	lock_time: bitcoin::blockdata::locktime::absolute::LockTime::ZERO,
	input: selected_coins
	.iter()
	.map(|_| bitcoin::TxIn::default())
	.collect(),
	output: self
	.recipients
	.iter()
	.filter_map(|recipient| {
	if recipient.valid() {
	Some(bitcoin::TxOut {
	script_pubkey: Address::from_str(&recipient.address.value)
	.unwrap()
	.payload
	.script_pubkey(),
	value: recipient.amount().unwrap(),
	})
	} else {
	None
	}
	})
	.collect(),
	};
	// nValue size + scriptPubKey CompactSize + OP_0 + PUSH32 + <wit program>
	const CHANGE_TXO_SIZE: usize = 8 + 1 + 1 + 1 + 32;
	let satisfaction_vsize = self.descriptor.max_sat_weight() / 4;
	let transaction_size =
	tx_template.vsize() + satisfaction_vsize * tx_template.input.len() + CHANGE_TXO_SIZE;
	// Now the calculation of the amount left to be selected by the user is a simple
	// substraction between the value needed by the transaction to be created and the
	// value that was selected already.
	let selected_amount = selected_coins.iter().map(|c| c.amount.to_sat()).sum();
	let output_sum: u64 = tx_template.output.iter().map(|o| o.value).sum();
	let needed_amount: u64 = transaction_size as u64 * feerate + output_sum;
	self.amount_left_to_select = Some(Amount::from_sat(
	needed_amount.saturating_sub(selected_amount),
	));
	}
	}

[jp1ac4]

In relation to the amount_left_to_select function, we could perhaps use the error from create_spend to determine the amount left to select. The InsufficientFunds error includes a (private) missing field with the missing amount in sats. This would require more calls to create_spend, but would ensure consistency in the amount shown with how the transaction is being created in the backend. Once #821 has been done, we could avoid incrementing the change index by passing a fixed change address for these intermediate calls.

[edouardparis]

Can we just add a max feerate value for now, something like 1000 ?

[darosior]

What's wrong with @jp1ac4's suggestion? Do you think it would necessitate a lot more work?

[edouardparis]

I do not understand how it prevent the user to put a too big feerate and stop the multiplication, it is not clear for me.

[darosior]

Well i think in this case you don't have the multiplication anymore since you use the result from create_spend to compute the amount left to select?

[edouardparis]

OK understood now.

[edouardparis]

We did forget that we have our RPC interface in the way for separate GUI and daemon.
Currently GUI and Daemon do not transfer information else than error message through the rpc interface. I think in order to tackle correctly this issue it is better to wait your refactoring work on the spend module, so GUI could call the create_spend method without needed to communicate through rpc to the distant daemon for each psbt generation.

[darosior]

It's already doing that for the coin selection though?

[edouardparis]

mmmh we may have a bug then, I am afraid that CoinSelectionError is not serialized correctly from the gui when receiving a RPC error from the daemon. I am trying right now

[darosior]

What do you mean?

[edouardparis]

Error while doing a spend with a detached daemon and gui:
ERROR liana_gui::daemon::client:48: method createspend failed: Rpc(RpcError { code: -32603, message: "Coin selection error: 'Insufficient funds. Missing 670000 sats.'", data: None })

When the daemon is embedded (not distant) the CoinSelectionError is correctly passed as DaemonError::CoinSelection
https://github.com/wizardsardine/liana/blob/master/gui/src/daemon/embedded.rs#L94

When the daemon is distant and we have rpc communication, the error is received as DaemonError::RpcError
https://github.com/wizardsardine/liana/blob/master/gui/src/daemon/client/mod.rs#L89
https://github.com/wizardsardine/liana/blob/master/gui/src/daemon/mod.rs#L20

[darosior]

Indeed, overlooked the detached daemon mode. We could use the JSONRPC error data field, but i'm afraid that's more work than we can afford right now.

[darosior]

So i think based on our discussions #842 makes this (as well as #840) more doable?

[edouardparis]

Yes, GUI would directly call the spend::create_spend module function without even using the command function and handle directly the error.