Allow honoring reserve in `send_all_to_address` by tnull · Pull Request #345 · lightningdevkit/ldk-node

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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

 },
 };

+ // 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


 	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 bitcoindevkit/bdk#352.

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

+ 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

+ };
+
+ 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.

tnull · 2024-11-08T13:10:06Z

Now rebased and adjusted to accommodate the BDK 1.0 API changes.

Should be ready for another round of re-review, @jkczyz.

jkczyz

LGTM. Please squash.

.. 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.

tnull · 2024-11-11T17:54:26Z

LGTM. Please squash.

Squashed fixups without further changes.

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 b62484aCompare August 16, 2024 15:20

tnull mentioned this pull request Aug 23, 2024
Add Initial Protobuf generation setup and initial protos for v1 api's. lightningdevkit/ldk-server#2
Merged

tnull changed the title ~~WIP: Allow honoring reserve in send_all_to_address~~Allow honoring reserve in send_all_to_addressAug 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 2266163Compare August 29, 2024 11:00

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

jkczyz reviewed Aug 29, 2024
View reviewed changes

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

tnull commented Sep 2, 2024
View reviewed changes

Comment threadsrc/wallet.rs Outdated

tnull mentioned this pull request Oct 8, 2024
LDK Node v0.5 Tracking Issue #360
Closed
10 tasks

tnull added this to the 0.5 milestone Oct 8, 2024

tnull force-pushed the 2024-08-regard-reserve-spending-all branch from b0f8d6c to 2bd4c68Compare November 8, 2024 13:08

tnull requested a review from jkczyz November 8, 2024 13:09

jkczyz reviewed Nov 8, 2024
View reviewed changes

Comment threadsrc/chain/mod.rs Outdated
Comment threadsrc/wallet/mod.rs Outdated
Comment threadsrc/wallet/mod.rs
Comment threadsrc/wallet/mod.rs Outdated
Comment threadtests/integration_tests_rust.rs Outdated
Comment threadtests/integration_tests_rust.rs Outdated

tnull force-pushed the 2024-08-regard-reserve-spending-all branch 2 times, most recently from 91207fb to 1eb0c0dCompare November 11, 2024 09:52

jkczyz reviewed Nov 11, 2024
View reviewed changes

tnull added 3 commits November 11, 2024 18:53

Log HTTP 400 at TRACE instead of ignoring it
51c9b4d
.. 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).

Add test for send_all_to_address behavior
91da460

tnull force-pushed the 2024-08-regard-reserve-spending-all branch from 1eb0c0d to 91da460Compare November 11, 2024 17:54

jkczyz approved these changes Nov 11, 2024
View reviewed changes

tnull merged commit c08c3d5 into lightningdevkit:mainNov 11, 2024