Allow honoring reserve in `send_all_to_address` #345

tnull · 2024-08-16T14:47:13Z

Previously, OnchainPayment::send_all_to_address could only be used to fully drain the onchain wallet, i.e., would not retain any reserves.

Here, we try to introduce a retain_reserves bool that allows users to send all funds while honoring the configured on-chain reserves. While we're at it, we move the reserve checks for send_to_address also to the internal wallet's method, which makes the checks more accurate as they now are checked against the final transaction value, including transaction fees.

This was requested by a user, but I'm a bit on the fence if we actually should move forward with it: for one, figuring out the spendable amount above the reserve is always gonna be inexact compared to draining the wallet.

~~Moreover, adding this to our API might send the wrong message of the reserve value being an exact value, while it's always on the safer side to maintain a larger reserve.~~

jkczyz · 2024-08-28T00:30:18Z

src/wallet.rs

 				},
 				Err(err) => {
 					log_error!(self.logger, "Failed to create transaction: {}", err);
 					return Err(err.into());
 				},
 			};

+			// Check the reserve requirements (again) and return an error if they aren't met.


Not sure I follow. Why is a second check needed?

The general issue is that we don't have a "send_all_but_X" method available, we can only set X amount or entirely drain the wallet (the latter of course resulting in not adding a change output). We also don't have any good tools to pre-compute the fee it gonna takes to construct a particular transaction without completely replicating BDK internals here (and even then you wouldn't be able to invert the fee estimation algorithm).

So the approach we took here is: construct a temporary draining transaction to estimate how much fees it would take, check that our available balance (i.e., the entire balance minus the reserve) is sufficient to cover the fee, then use this estimate to calculate how much above the reserve we're able to spend, and then construct the actual spending transaction with the estimated fee and the estimated spendable balance.

Once we did all that we now build the PSBT and check again that we're really able to spend what we just constructed without infringing on the reserve, and go ahead and sign it.

So TLDR: first round of checks are on the temporary transaction we use to estimate fees (and hence the spendable amount), second round of checks on the actual final transaction we try to spend.

jkczyz · 2024-08-28T00:32:34Z

tests/integration_tests_rust.rs

@@ -279,7 +279,7 @@ fn onchain_spend_receive() {
 	assert!(node_b.list_balances().spendable_onchain_balance_sats < 100000);

 	let addr_b = node_b.onchain_payment().new_address().unwrap();
-	let txid = node_a.onchain_payment().send_all_to_address(&addr_b).unwrap();
+	let txid = node_a.onchain_payment().send_all_to_address(&addr_b, false).unwrap();


Could we additional test coverage?

Now added test coverage and discovered small issues around relayability (on regtest, at least). Therefore now finally got around to do a refactor of FeeEstimator that should allow us to configure these targets a little more fine-grained rather than misusing LDK's API (which we more ore less did before). Now based this PR on top of #352.

tnull · 2024-08-29T11:03:58Z

Now based on top of #352.

jkczyz · 2024-08-29T20:33:05Z

src/wallet.rs

+					tmp_tx_builder
+						.add_recipient(address.script_pubkey(), spendable_amount_sats)
+						.fee_rate(fee_rate)
+						.enable_rbf();


Couldn't this result in a transaction where the entire cur_anchor_reserve_sats goes to fees? And thus there could be one less output, which would cause the fee estimation to be off?

Would the following solve this?

let change_address = locked_wallet.get_internal_address(AddressIndex::Peek(0)); tmp_tx_builder .drain_wallet() .drain_to(address.script_pubkey()) .add_recipient(change_address, cur_anchor_reserve_sats) .fee_rate(fee_rate) .enable_rbf();

Though if cur_anchor_reserve_sats was exactly covered by one utxo, then an extra input would be used in the estimation. 🤔

Couldn't this result in a transaction where the entire cur_anchor_reserve_sats goes to fees? And thus there could be one less output, which would cause the fee estimation to be off?

Yes, this could be the case, I think.

Would the following solve this?

let change_address = locked_wallet.get_internal_address(AddressIndex::Peek(0)); tmp_tx_builder .drain_wallet() .drain_to(address.script_pubkey()) .add_recipient(change_address, cur_anchor_reserve_sats) .fee_rate(fee_rate) .enable_rbf();

Mhh, seems reasonable to assume that this would make the estimation a tad more precise, I'll add a fixup for this.

Though if cur_anchor_reserve_sats was exactly covered by one utxo, then an extra input would be used in the estimation. 🤔

Yes, this would be an edge case. Similarly, the coin selection algorithm might decide to omit outputs (if they'd end up to be dust, for example), which also would throw the calculation off.

Btw, curiously, at least in local small-scale testing, both approaches seem to result in virtually the same weight discrepancies between temporary and final transaction, while other factors (randomness in coin selection?) seem to have a bigger impact.

jkczyz · 2024-08-29T20:33:32Z

src/wallet.rs

+					};
+
+					let estimated_tx_fee_sats =
+						tmp_tx_details.fee.unwrap_or(0).max(FEERATE_FLOOR_SATS_PER_KW as u64);


Shouldn't FEERATE_FLOOR_SATS_PER_KW be accounted for by the fee estimator when computing the fee_rate passed to the builder?

Yes, it's mostly a failsafe as we use the absolute estimated fee, not the fee rate. But you're right, assuming the estimation is reasonably close, we can probably omit this.

.. previously we used LDK's `FeeEstimator` and `ConfirmationTarget` and ~misused some of the latter's variants for our non-Lightning operations. Here, we introduce our own `FeeEstimator` and `ConfirmationTarget` allowing to add specific variants for `ChannelFunding` and `OnchainPayment`s, for example.

.. while it's most often bitcoind already knowing about a transaction already, the error sometimes holds additional information (e.g., not meeting the mempool min).

Previously, `OnchainPayment::send_all_to_address` could only be used to fully drain the onchain wallet, i.e., would not retain any reserves. Here, we try to introduce a `retain_reserves` bool that allows users to send all funds while honoring the configured on-chain reserves. While we're at it, we move the reserve checks for `send_to_address` also to the internal wallet's method, which makes the checks more accurate as they now are checked against the final transaction value, including transaction fees.

.. rather than undercutting the fee, we now choose a method that might slightly overestimate fees, ensuring its relayability. We also make sure the net fee is always larger than the fee rate floor.

tnull · 2024-09-02T13:26:48Z

src/wallet.rs

 			}

-			let mut psbt = match tx_builder.finish() {
-				Ok((psbt, _)) => {
+			let (mut psbt, tx_details) = match tx_builder.finish() {


@jkczyz FYI: It seems BDK 1.0 removed the returned tx_details here, so we won't be able to use them for the checks below going forward (see bitcoindevkit/bdk#1401).

I think I'll deprioritize this PR for now and try to rebase/pick it up again after the BDK upgrade is through and we can evaluate what APIs are available really (e.g., might need to reimplement sent/received logic manually based on the inputs/outputs and the Wallet::is_mine functionality).

tnull marked this pull request as draft August 16, 2024 14:47

tnull force-pushed the 2024-08-regard-reserve-spending-all branch from a020d7c to b62484a Compare August 16, 2024 15:20

tnull changed the title ~~WIP: Allow honoring reserve in send_all_to_address~~ Allow honoring reserve in send_all_to_address Aug 27, 2024

tnull marked this pull request as ready for review August 27, 2024 10:18

jkczyz self-requested a review August 27, 2024 14:07

jkczyz reviewed Aug 28, 2024

View reviewed changes

tnull force-pushed the 2024-08-regard-reserve-spending-all branch from b62484a to 2266163 Compare August 29, 2024 11:00

tnull force-pushed the 2024-08-regard-reserve-spending-all branch 3 times, most recently from 385fbe5 to 28fed4c Compare August 29, 2024 11:12

jkczyz reviewed Aug 29, 2024

View reviewed changes

tnull added 9 commits August 30, 2024 08:34

Make Wallet and WalletKeysManager pub(crate)

7202c83

f s/which we use/used/

9b1ea5a

Log HTTP 400 at TRACE instead of ignoring it

01c499e

.. while it's most often bitcoind already knowing about a transaction already, the error sometimes holds additional information (e.g., not meeting the mempool min).

f Fix logged argument order

e86d843

f Make sure transaction is relayable

326cc38

.. rather than undercutting the fee, we now choose a method that might slightly overestimate fees, ensuring its relayability. We also make sure the net fee is always larger than the fee rate floor.

f Make temporary estimation a bit more precise

156a970

Add test for send_all_to_address behavior

b0f8d6c

tnull force-pushed the 2024-08-regard-reserve-spending-all branch from 28fed4c to b0f8d6c Compare August 30, 2024 07:40

tnull commented Sep 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow honoring reserve in `send_all_to_address` #345

Allow honoring reserve in `send_all_to_address` #345

tnull commented Aug 16, 2024 •

edited

Loading

jkczyz Aug 28, 2024

tnull Aug 28, 2024 •

edited

Loading

jkczyz Aug 28, 2024

tnull Aug 29, 2024

tnull commented Aug 29, 2024

jkczyz Aug 29, 2024

tnull Aug 30, 2024

tnull Aug 30, 2024

jkczyz Aug 29, 2024

tnull Aug 30, 2024

tnull Sep 2, 2024 •

edited

Loading

Allow honoring reserve in send_all_to_address #345

Are you sure you want to change the base?

Allow honoring reserve in send_all_to_address #345

Conversation

tnull commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

tnull Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Allow honoring reserve in `send_all_to_address` #345

Allow honoring reserve in `send_all_to_address` #345

tnull commented Aug 16, 2024 •

edited

Loading

tnull Aug 28, 2024 •

edited

Loading

tnull Sep 2, 2024 •

edited

Loading