search

bitcoindevkit / bdk

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

Redeposit should check address reuse #253

Open RCasatta opened 3 years ago

RCasatta commented 3 years ago

When doing a redeposit (a tx where the recipient address is owned by the wallet making the tx) in a wallet without a change_descriptor it could happen to have a tx with 2 outputs to the same address which should be avoided (in that case, increase the change derivation by one)

kjain1810 commented 2 years ago

Hi I would like to take this issue up I am new to this project but have experience working in Rust. I would like to familiarise myself with the codebase with this issue.

Can I take this up?

notmandatory commented 2 years ago

Hi @kjain1810 thanks for taking a look, I'll put your name on it. If you have any questions leave a note here or we can discuss on Discord.

kjain1810 commented 2 years ago

@notmandatory I started looking into this issue.

I am considering doing this check on the add_recipient method in tx_builder.rs by checking if the scriptpubkey already exists in the list of recipients or not. This would help fix not only for redeposits but also for different wallets as well.

Do you think it's a good idea to take this path?

notmandatory commented 2 years ago

I recommend looking into the wallet::create_tx function. There is a part of this function where the drain_output is prepared, including setting the change address. You can check the tx outputs here and if the one you were going to use for change is already being used you can use the next one instead.

Before any of that I also recommend adding a new test in the wallet::test module to replicate this issue so you can better understand the scenario. For example a change address gets added after you've already called add_recipient one or more times to add recipient addresses to the TxParams.