Replace wallet bump_fee command --send_all with new --shrink option

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.

[notmandatory]

Description

Replace wallet bump_fee command --send_all with new --shrink ADDRESS option to reduce the output amount for the specified address to increase RBF transaction fee.

Notes to the reviewers

This new option is primarily needed when bumping the fee of a send_all transaction or any other transaction that doesn't have a change output that can be reduced to bump the fee on the new RBF transaction.

Steps I used to test

1. create wallet with two utxos
2. create and broadcast send_all tx with rbf enabled and 1 sat/vbyte fee back to testnet faucet
3. create and broadcast bump_fee rbf for above tx with 7 sat/vbyte fee and shrinking the testnet faucet address

https://mempool.space/testnet/tx/e35892843875e084f39c5285ee6f522764210351f78cd89305a49efbf4bfea48

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

[rajarshimaitra]

Ah right. We could get the scriptpubkey from specifying the address. And it seems bdk internal doesn't care if the address belongs to us or not, while shrinking.

But I am thinking, doesn't this opens an attack vector in bdk wallet code?

For example I can construct a transaction to a merchant, with very low fee. He will see money coming into his wallet (as many doesn't wait for confirmations). Then half an hour later after leaving the shop, I can then bumpfee to reduce the merchant's output to 0.

The cost of this attack is basically zero, because the attacker will not loose any more money, than he already paid for. He has already spent his input and will get the change back.

Note: This behavior of not to decrease external recipient's amount is not enforced by the bitcoin protocol (it can't be). It is an wallet level feature. And I think wallets implementing bumpfee will need to account for that in their impls.

For reference, heres the doc for bumpfee behaviour of the Bitcoin Core wallet. It doesn't allow reducing external outputs, and if there isn't any change ouput in a tx, the wallet will proactively add one.

https://developer.bitcoin.org/reference/rpc/bumpfee.html

This where the change selection logic happens in bitcoin core wallet.
https://github.com/bitcoin/bitcoin/blob/927586990eb9bc8403a3831247847bdd3bf60423/src/wallet/feebumper.cpp#L176-L187

[notmandatory]

There is logic in the existing bdkwallet/mod.rs line 561-570 that will throw an Error::FeeTooLow if the new fee amount is less than the previous fee, but it sounds like there could still be a different issue with this logic, see: bitcoindevkit/bdk_wallet#192

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

[rajarshimaitra]

For the purposed of this PR I think it's OK to allow bumping fees to external receivers even if there could be a missing case in the minimum fee logic, at least bdk requires the bump fee be greater than the current tx fee.

Sorry, I am not seeing how the minimum fee issue is relevant here. The issue is reducing external recipient's amount in RBF, which can be a potential bug. Min fee checking will always pass, if someone exploits this (the fee given will then be very high).

bitcoindevkit/bdk_wallet#192 are also fee amount related issues which is orthogonal to this.

[notmandatory]

Ok I thought your concern was that bdk isn't enforcing that a bump_fee transaction has a higher fee than the original tx, which I believe is being enforced. With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner. Also keep in mind that allow_shrinking is only allowed for a TxBuilder created with the BumpFee context.

Even so I think it's a valuable feature to be able to bump fee by reducing an external single recipients output for the case where you're sending to a different external wallet that you control.

[rajarshimaitra]

With the current bump fee API I don't see how you could reduce the external recipients amount to zero except by setting such a high tx fee that it eats up all the sent amount. In this case the attacker is cheating the external recipient, but isn't gaining much since the output amount now goes to the miner.

Yes that is correct. With this attack the attacker won't gain anything, although its a "zero cost micheif" opportunity, which I in my opinion should not be allowed. It's not a critical vulnerability, but an UI/UX gap. There's nothing in the bitcoin protocol itself to stop this from happening. So wallets need to care for it. That's my whole point.

I agree with this current change for fixing the bump_fee operation in bdk-cli, and its also simpler than bitcoindevkit/bdk#42. But I still think bdk should check for this behavior, and should not allow shrinking non wallet addresses. This can be done in a separate PR in bdk and not in bdk-cli, subjected to opinions of others on the same.

This PR is good to go and I think its a good substitution for bitcoindevkit/bdk#42.

[notmandatory]

Although I see bdk-cli mostly as a test tool that should be able to create any valid bump fee transaction BDK and the bitcoin network will accept, I also see @rajarshimaitra's point that it should demonstrate wallet and BDK best practices.

We can discuss at our team chat tomorrow and if no strong opinions against will merge. :-)
Since bitcoindevkit/bdk#45 is closed I'll go ahead and merge this one.

[notmandatory]

Rebased to lastest master.