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

allow_shrinking not working as expected #1342

Open
BitcoinZavior opened this issue Feb 13, 2024 · 2 comments
Open

allow_shrinking not working as expected #1342

BitcoinZavior opened this issue Feb 13, 2024 · 2 comments
Assignees
Labels
api A breaking API change bug Something isn't working documentation Improvements or additions to documentation
Milestone

Comments

@BitcoinZavior
Copy link

BitcoinZavior commented Feb 13, 2024

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

  • BDK v0.28.2
  • macOS 13.2
  • Rust/Cargo version: 1.76.0

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
Copy link
Contributor

LLFourn commented Feb 13, 2024

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
Copy link
Member

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

@notmandatory notmandatory added documentation Improvements or additions to documentation api A breaking API change labels Mar 18, 2024
@notmandatory notmandatory self-assigned this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change bug Something isn't working documentation Improvements or additions to documentation
Projects
Status: Ready to Review
Development

No branches or pull requests

3 participants