bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
855 stars 307 forks source link

allow_shrinking not working as expected #1342

Open BitcoinZavior opened 8 months ago

BitcoinZavior commented 8 months ago

Using build_fee_bump to replace transaction along with allow_shrinking results in the specified address output amount increasing and consuming all of the input utxo amount.

To Reproduce Create a transaction with one output ADDRESS and specify an x amount to be sent. sign and broadcast. Sync wallet. Create a bump fee txbuilder and call allow_shrinking(ADDRESS) and specify a higher feerate fee_rate

Example Code:


fn create_original_tx(){
     let (wallet, blockchain) = init();
     wallet.sync(&blockchain, SyncOptions::default()).unwrap();
     let addr = bdk::bitcoin::Address::from_str("bcrt1q6m7tyf45hd5swf7ug5f3r35y6htga0m2tlxhfh").unwrap();
     println!("Descriptor balance: {} SAT", wallet.get_balance().unwrap());
     let script = addr.script_pubkey().clone();
     let mut tx_builder = wallet.build_tx();
     tx_builder
         .add_recipient(script.clone(), 10000)
         .fee_rate(FeeRate::from_sat_per_vb(1.0))
         .enable_rbf();
     let (mut psbt, _tx_details) = tx_builder.finish().unwrap();
     let  finalized = wallet.sign(&mut psbt, bdk::SignOptions::default()).unwrap();
     assert!(finalized, "we should have signed all the inputs");
     let tx = psbt.clone().extract_tx();
     blockchain.broadcast(&tx).unwrap();
     let txid = psbt.extract_tx().txid();
     println!("Txid:{:?}",txid );
 }

fn build_bump_psbt(){
     let (wallet, blockchain) = init();
     let addr = bdk::bitcoin::Address::from_str("bcrt1q6m7tyf45hd5swf7ug5f3r35y6htga0m2tlxhfh").unwrap();
     wallet.sync(&blockchain, SyncOptions::default()).unwrap();
     let mut tx_builder = wallet.build_fee_bump(Txid::from_str("f8f8d1e566eb2ae963825628e4a3ef249a82aa71b8bfb5389b5d935d8864eebe").unwrap()).unwrap();
     tx_builder
         .allow_shrinking(addr.script_pubkey()).unwrap()
         .fee_rate(FeeRate::from_sat_per_vb(2.5));
     let (mut psbt, _tx_details) = tx_builder.finish().unwrap();
     let  finalized = wallet.sign(&mut psbt, bdk::SignOptions::default()).unwrap();
     assert!(finalized, "we should have signed all the inputs");
     let tx = psbt.clone().extract_tx();
     blockchain.broadcast(&tx).unwrap();
     let txid = psbt.extract_tx().txid();
     println!("Txid2 :{:?}",txid );
 }

Expected behavior
Expect the output ADDRESS to have the same amount or less. Amount will be less if the input transaction cannot cover the fee as well as the originally specified amount for ADDRESS

However output ADDRESS is having a higher amount than the origincal. It is actually consuming all the input amount.

Build environment

Additional context

Original transaction outputs Screenshot 2024-02-09 at 19 31 09

Transaction outputs after fee bump and allow shrinking: Screenshot 2024-02-09 at 19 32 05

LLFourn commented 8 months ago

Another bug is that the fee of the replacement tx doesn't satisfy RBF rule 4 (the abs fee is actually lower than the original).

This looks like a severe problem. The output ending 9whc7h shouldn't have been deleted. Why would the fee bump steal all its value. I wonder which keychains these were part of?

My 2c is that the builder API here is just not right. A fee bump shouldn't try to interpret the existing transaction and replicate it -- it should just create a tx with the appropriate fee that spends one or more of the existing transaction's outputs.

The builder API should just be tx_builder.replace(tx). You could call it several times to replace several transactions.

notmandatory commented 7 months ago

I think the docs on allow_shrinking are misleading/incomplete. What this function does is set the output for the specified ADDRESS to act as the (shrinkable) change address. So if the transaction inputs are MORE than required is to pay the outputs and fee, no change is made and the whole remaining change balance goes to your allow_shrinking ADDRESS output.

I created #1366 to add a warning to the docs and confirm expected behavior works.

See: https://docs.rs/bdk/latest/src/bdk/wallet/tx_builder.rs.html#662-679