commands: include unconfirmed change as coin selection candidates #873

jp1ac4 · 2023-12-13T00:22:14Z

This is a first PR towards resolving #826 that adds unconfirmed change as coin selection candidates when creating a spend.

As per #826 (comment), I haven't made any changes to the rbfpsbt command.

We will also need to apply the same change in the GUI when selecting candidates for coin selection there (see #863 (comment)).

darosior

Unfortunately it's more complicated than just including unconfirmed coins in the selection. Each unconfirmed parent transaction at a lower feerate than the transaction we are creating would decrease the effective feerate (ie the ancestor feerate) of the transaction we are creating.

I am not sure how to approach this. One option is to:

For each unconfirmed coin, fetch the parent transaction's ancestor feerate and vsize using a getmempoolentry. (if no result, ignore the coin)
If it's lower than the feerate we are targeting, compensate by increasing the feerate of the transaction we are creating (effectively perform a CPFP).

This would be an issue because on the GUI we display the standalone feerate of the transaction, whereas what matters to a miner is its ancestor feerate. If we go this route we'll certainly confuse the user as we may exceed the requested feerate by a huge margin. We could still do this but display a warning to the user "transaction was created with a feerate of 300sat/vb to pay for its unconfirmed parents, to make the whole package pay 100sat/vb as requested". This brings up #811 once again.

I guess this also changes the effective value of a coin (some will have larger, or lower feerate, parents and will need more fees to be spent).

Another approach would be to only consider coins whose unconfirmed transaction has an ancestor feerate higher or equal to the transaction we are presently creating. I'm afraid this would greatly decrease the utility of being able to spend unconfirmed coins, though.

In any case we should also prefer spending confirmed coins when possible.

Ideally we would also check the mempool limits when creating the transaction. At least for the most basic sanity checks of checking the number of descendants of the transaction we'd be spending.

darosior · 2023-12-15T17:05:35Z

src/commands/mod.rs

+            // Including unconfirmed external deposits would introduce a whole bunch
+            // of additional complexity.


It's more that it would be "unsafe" without being explicitly acknowledged by the user, as these coins may disappear at any time.

darosior · 2023-12-15T17:41:47Z

We will also need to apply the same change in the GUI when selecting candidates for coin selection there (see #863 (comment)).

I think this should go in this PR as well, since we don't need to update the liana version in the GUI before making the change.

jp1ac4 · 2023-12-15T19:45:40Z

Thanks for explaining these additional considerations.

I think your first approach could work. If we display extra information in the GUI for transactions spending unconfirmed coins, I think it would help avoid some of the potential confusion for the user.

We may be able to reflect the effect of ancestors in an unconfirmed candidate by increasing its weight attribute in order to increase the fee of the new transaction in such a way that the effective feerate for the new transaction plus its ancestors would be the same as the target feerate of the new transaction. Any unconfirmed candidates with low-feerate ancestors would be less likely to be selected in that case.

pythcoiner · 2023-12-20T10:52:08Z

We could still do this but display a warning to the user "transaction was created with a feerate of 300sat/vb to pay for its unconfirmed parents, to make the whole package pay 100sat/vb as requested". This brings up #811 once again.

yes I think we should display 2 fees: one related to the fee debt of previous transaction and a second one related to the current tx

jp1ac4 · 2024-01-04T01:32:30Z

I guess this also changes the effective value of a coin (some will have larger, or lower feerate, parents and will need more fees to be spent).

I think the additional cost of spending an unconfirmed candidate in a way that achieves target_feerate should be something like

(target_feerate * ancestor_size) - ancestor_fee

So we could reduce the candidate's value by this amount. If the ancestor feerate is higher than target_feerate, this delta would be negative so we would increase the candidate's effective value.

darosior · 2024-01-04T17:25:20Z

(target_feerate * ancestor_size) - ancestor_fee

Yes that makes sense to me. However two coins may (partially) share the same ancestry.. We can just explicitly not take this into account and sometimes count an ancestor twice, that'd be unlikely but quite ugly when hit.

So we could reduce the candidate's value by this amount.

Hmm it sounds like it could have unintended consequences. Instead we could increase the weight to achieve the same?

If the ancestor feerate is higher than target_feerate, this delta would be negative so we would increase the candidate's effective value.

But this would make the selection think it's contributing more value than it really does wouldn't it? If so we certainly shouldn't do that, as parent don't pay for child. :)

I think what you want here is to bias the selection toward coins which aren't holding a large backlog right?

jp1ac4 · 2024-01-04T18:53:47Z

Yes that makes sense to me. However two coins may (partially) share the same ancestry.. We can just explicitly not take this into account and sometimes count an ancestor twice, that'd be unlikely but quite ugly when hit.

That's true. Ideally, we'll be able to reflect any such overpayment in a warning.

But this would make the selection think it's contributing more value than it really does wouldn't it? If so we certainly shouldn't do that, as parent don't pay for child. :)

Ah yes, of course 😄

I think what you want here is to bias the selection toward coins which aren't holding a large backlog right?

That's right. I think changing the weight would be better then. We could increase/decrease an unconfirmed candidate's effective weight by

ancestor_size - (ancestor_fee / target_feerate)

which would then change the cost of spending this candidate at target_feerate as required. The effective weight could go down to 0 if the ancestor feerate is much higher.

jp1ac4 · 2024-01-04T19:43:35Z

We could increase/decrease an unconfirmed candidate's effective weight by ...

Perhaps we shouldn't reduce the weight in the case the ancestor feerate is higher as we might end up with a feerate below target_feerate if two unconfirmed candidates share the same ancestors.

darosior · 2024-01-05T08:32:57Z

Yes, it's also a case of parent-pays-for-child.

jp1ac4 · 2024-01-08T19:13:27Z

I've updated it to get the ancestor size and fees of any unconfirmed coins, and to modify the corresponding candidate's weight accordingly. This is done for both automated coin selection and manual coin selection. In the latter case, I'm adding ancestor info for all unconfirmed coins that have been specified, whether they are change or not.

In order to return a warning to the user, one option would be to check if any selected candidates have had their weights modified and to return the additional fee paid.

darosior · 2024-01-09T11:34:29Z

So we don't have to modify anything in the GUI for the manual selection? The "amount left to select" in the coin control is now using this right?

jp1ac4 · 2024-01-09T13:39:30Z

For manual selection, the GUI would need to be updated here to apply the same change when creating the CandidateCoin in order to calculate the "amount left to select" in the same way.

jp1ac4 · 2024-01-10T18:58:49Z

For manual selection, the GUI would need to be updated here to apply the same change when creating the CandidateCoin in order to calculate the "amount left to select" in the same way.

Would it also be possible to allow spend::create_spend() to access the mempool_entry function? I guess we would need to add this to the TxGetter trait. Then we could add the ancestor info within that function instead of in the GUI.

darosior · 2024-01-11T09:33:16Z

That's unfortunate that we should duplicate this in the GUI.. I don't any non-ugly way of doing this. Should we go back to calling the RPC command instead of trying to use the library's create_spend?

jp1ac4 · 2024-01-11T13:47:30Z

Should we go back to calling the RPC command instead of trying to use the library's create_spend?

If it would still be possible to parse any error (to get amount left to select), then that seems the best way to avoid duplication of both the ancestor info and also criteria for selecting candidates (confirmed + unconfirmed change).

Another idea would be some kind of listcandidates command that would include ancestor info and whether the coin can be used for manual/auto selection, e.g. unconfirmed external deposits can only be used for manual selection.

jp1ac4 · 2024-01-18T17:20:44Z

Building on the changes from #905, I've now added a warning in case an additional fee is included to pay for ancestors.

With the changes from #927, the GUI will be able to call the createspend command directly instead of the library's create_spend and so the GUI will not need to duplicate any logic from this PR.

If this approach looks fine, I'll continue adding tests.

darosior · 2024-01-23T14:44:58Z

This approach looks good to me.

@darosior

1339898 commands: include missing amount in response (jp1ac4) Pull request description: This PR follows a discussion around #873 (comment). The GUI uses the `InsufficientFunds` error to get the missing amount when the user is creating a new spend, but it is not straightforward to extract this information in a general way from the RPC error (see #822 (comment)) and instead the spend module's `create_spend` is currently used (see #863). With this PR, the missing amount will be included in the `createspend` response rather than as an error. These changes are based on suggestions from @darosior and @edouardparis. In a follow-up PR, the GUI should revert to using the `createspend` command to calculate the amount left to select. ACKs for top commit: darosior: re-ACK 1339898 Tree-SHA512: bf702d6b355339e96e719c1d95824e7941ac4fbaece4ec4cccd00b56ea4683ce7fb0cefc43faa5731b57e7935ef99da3a2c73b84aaeb9fa5f67703c799be2196

jp1ac4 · 2024-01-26T07:58:13Z

I've now updated the functional tests to check for additional fees due to ancestors.

…sing amount cb5073c gui(spend): use create_spend_tx for missing amount (jp1ac4) fae32df gui: pass change_address param to createspend (jp1ac4) Pull request description: This PR is a follow-up to #927 and uses changes made in #938. It reverts to using the `createspend` command in the GUI for automated coin selection and to determine the amount left to select when creating a new spend (which was previously changed in #863). With this PR, the changes from #873 will become effective in the GUI so that (some) unconfirmed coins are used as candidates and any additional fee to pay for ancestors is included when calculating the amount left to select. ACKs for top commit: edouardparis: ACK cb5073c Tree-SHA512: d780b8a0238b595301701d889c45b263682867cdff1ec054872f717de7ae3d325fc5010c8df29333ae2a44ae2e92a86689d332a26ac7334c7e92fd9ffc7a6397

85f5a68 gui(spend): display warnings for draft PSBTs (jp1ac4) Pull request description: This is to complete #811. It displays any warnings generated by `createspend` on the draft PSBT view, which could currently be those from #905 and #873. Once the PSBT has been saved, these warnings will no longer be shown. Below are some screenshots. With a warning: ![image](https://github.com/wizardsardine/liana/assets/121959000/fb2167d9-4800-4849-8622-ff20ac55623e) ![image](https://github.com/wizardsardine/liana/assets/121959000/e1ab9ec2-00ae-457f-81e1-9b3a2863af31) Without a warning: ![image](https://github.com/wizardsardine/liana/assets/121959000/73b5b4bd-55b5-40ee-b587-4f57bfd57b4c) ![image](https://github.com/wizardsardine/liana/assets/121959000/e9b23d4e-bb13-4ae7-944e-258f9c9a0abd) ACKs for top commit: edouardparis: ACK 85f5a68 Tree-SHA512: b9875801f7607f6f824ad9ab5182e93aa533ba2bbb8f9a4168c4a2a779a8f39cf77dc0b24eac7c99a0fce36c486c126144d23b0cb7ecec27ebc145ea2ffdb212

darosior

ACK c0d4320

Great work. Good test coverage. I've got a few comments but nothing worth blocking what's been waiting for my review for over a month.

Stress tested the coin selection functional tests by running them hundreds of time in parallel and did some lightweight testing of this PR rebased on master.

darosior · 2024-03-09T15:27:17Z

src/spend.rs

+    // Get feerate as u32 for calculation relating to ancestor below.
+    // We expect `feerate_vb` to be a positive integer, but take ceil()
+    // just in case to be sure we pay enough for ancestors.
+    let feerate_vb_u32 = feerate_vb.ceil() as u32;


It's preferable to explicitly panic if the feerate is outside the range of u32 than to potentially let an(other) insane value in.

That's true, I could have manually checked the value, perhaps with feerate_vb.ceil() > u32::MAX as f32.
I was initially looking for something like try_into, but couldn't find anything for float to int.

Oh that's unfortunate that there is no try_into and we should implement those check manually.

darosior · 2024-03-09T15:31:51Z

src/spend.rs

    let candidates: Vec<Candidate> = candidate_coins
        .iter()
        .map(|cand| Candidate {
            input_count: 1,
            value: cand.amount.to_sat(),
-            weight: max_input_weight,
+            weight: {


nit: i'd rather this calculation was done outside of the initialization of the Candidate struct. I find the code easier to follow if we compute everything we need and only then proceed to initialize the struct.

darosior · 2024-03-09T15:51:29Z

src/spend.rs

+    let witness_factor: u32 = WITNESS_SCALE_FACTOR
+        .try_into()
+        .expect("scale factor must fit in u32");


nit: On the other hand, since that's a constant an as cast here is fine i think.

darosior · 2024-03-09T15:53:43Z

src/spend.rs

+                let extra = cand
+                    .ancestor_info
+                    .map(|info| {
+                        // The implied ancestor vsize if the fee had been paid at our target feerate.
+                        let ancestor_vsize_at_feerate: u32 = info
+                            .fee
+                            .checked_div(feerate_vb_u32)
+                            .expect("feerate is greater than zero");
+                        // If the actual ancestor vsize is bigger than the implied vsize, we will need to
+                        // pay the difference in order for the combined feerate to be at the target value.
+                        // We multiply the vsize by 4 to get the ancestor weight, which is an upper bound
+                        // on its true weight (vsize*4 - 3 <= weight <= vsize*4), to ensure we pay enough.
+                        // Note that if candidates share ancestors, we may add this difference more than
+                        // once in the resulting transaction.
+                        info.vsize
+                            .saturating_sub(ancestor_vsize_at_feerate)
+                            .checked_mul(witness_factor)
+                            .expect("weight difference must fit in u32")
+                    })
+                    .unwrap_or(0);


I stared at this for a while. Maybe we are thinking about this differently (which is interesting), but don't you think it's more obvious to just calculate the delta between the ancestor and target feerates instead?

diff --git a/src/spend.rs b/src/spend.rs index d545375..72f3aeb 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -341,21 +341,22 @@ fn select_coins_for_spend( let extra = cand .ancestor_info .map(|info| { - // The implied ancestor vsize if the fee had been paid at our target feerate. - let ancestor_vsize_at_feerate: u32 = info + // The additional weight carried by this candidate is the size of its + // unconfirmed ancestors minus the fees paid by this ancestor. + // Calculate the delta between the ancestor's feerate and our target + // feerate to compute the extra weight, if any. + let ancestor_feerate = info .fee - .checked_div(feerate_vb_u32) - .expect("feerate is greater than zero"); - // If the actual ancestor vsize is bigger than the implied vsize, we will need to - // pay the difference in order for the combined feerate to be at the target value. - // We multiply the vsize by 4 to get the ancestor weight, which is an upper bound - // on its true weight (vsize*4 - 3 <= weight <= vsize*4), to ensure we pay enough. - // Note that if candidates share ancestors, we may add this difference more than - // once in the resulting transaction. - info.vsize - .saturating_sub(ancestor_vsize_at_feerate) + .checked_div(info.vsize) + .expect("Can't have a null vsize"); + let extra_feerate = feerate_vb_u32.saturating_sub(ancestor_feerate); + let ancestor_weight = info + .vsize .checked_mul(witness_factor) - .expect("weight difference must fit in u32") + .expect("Weight must fit in a u32"); + extra_feerate + .checked_mul(ancestor_weight) + .expect("Must stay within the u32 bounds") }) .unwrap_or(0); // Store the extra weight for this candidate for use later on.

That's true, I think it can also be thought of as the extra feerate for the ancestor component, which could be more intuitive.

Note that in your suggested change, extra_feerate.checked_mul(ancestor_weight) gives the extra fee that needs to be paid for the ancestor, and dividing by feerate_vb to get the extra weight still gives the same value of ancestor_size - (ancestor_fee / feerate).

Yes i didn't mean to change the semantic, just to make it clearer.

darosior · 2024-03-09T16:13:37Z

src/commands/mod.rs

+                // In case any previous coins are unconfirmed, we don't include their ancestor info
+                // in the candidate as the replacement fee and feerate will be higher and any
+                // additional fee to pay for ancestors should already have been taken into account
+                // when including these coins in the previous transaction.


Couldn't this lead to a miscalculation when selecting from the set of previous coins though? For instance in case of a cancel of a transaction for which one of the inputs has a large backlog, when canceling it without considering ancestors we could mistakenly consider the input with the large backlog as high value and sufficient to take alone to RBF the original transaction.

My thinking was that the min_fee should help here, as the previous fee would have taken into account paying extra for ancestors, and so the replacement would pay at least this amount. So even if we think a coin has high value, the min_fee would already have taken into account its ancestors and so we'd need to choose more coins to meet this min value.

On the other hand, I think I was taking shortcuts and it's probably safer just to add the ancestor info here too :) I'll add it to the RBF follow-ups issue #853 as at least something to consider.

darosior · 2024-03-09T16:15:48Z

src/commands/mod.rs

+                .map(|(op, c)| {
+                    let ancestor_info = if c.block_info.is_none() {
+                        // We include any non-change coins here as they have been selected by the caller.
+                        // If the unconfirmed coin's transaction is no longer in the mempool, keep the


nit: probably not worth addressing here but if any ancestor of the unconfirmed coin is no longer in the mempool then this coin doesn't exist anymore and we should probably bail out early.

I suppose it could either be that it doesn't exist or that it's been confirmed but not yet in our DB? I was initially going to leave out the coin to be on the safe side, but then decided to keep it in.

darosior · 2024-03-09T17:33:47Z

tests/test_spend.py

+    assert bitcoind.rpc.getmempoolentry(deposit_d)["ancestorsize"] == 165
+    assert bitcoind.rpc.getmempoolentry(deposit_d)["fees"]["ancestor"] * COIN == 165
+    # ancestor vsize at feerate 2 sat/vb = ancestor_fee / 2 = 165 / 2 = 82
+    # extra_weight <= (extra vsize * witness factor) = (165 - 82) * 4 = 332
+    # additional fee at 2 sat/vb (0.5 sat/wu) = 332 * 0.5 = 166


Wouldn't it be even more obvious (and robust) to perform this calculation directly in Python?

Oh yeah, I didn't think of that :) Will bear that in mind if coming back to this code.

Adapting this to Taproot i basically translated this to Python.

f9bae9c database: add migration from db version 3 to 4 (jp1ac4) 2c96ef5 commands: exclude immature coins from coin selection (jp1ac4) 3a7c151 database: allow for coinbase transactions to change addresses (jp1ac4) Pull request description: As a follow-up to #873, this ~~adds a comment to make clear that immature coins are not included as candidates for auto-selection~~ excludes immature coins from auto-selection. An unconfirmed coin could be both immature and marked as change, as the latter only depends on whether the address is derived from the wallet's change descriptor. I've also removed a corresponding assertion that may not always hold. ACKs for top commit: darosior: ACK f9bae9c -- this is very nice. Again, great catch. And thanks for adding a more extensive unit test. Tree-SHA512: 12abe3dd18723db58ff701f664c6085e7fd29d39fefa7206e3e00fa5fb3e3b4320720c183a9719a3a0f3124896347ac74571b45a54bd504d674f779913466c16

darosior reviewed Dec 15, 2023

View reviewed changes

jp1ac4 marked this pull request as draft December 21, 2023 12:51

jp1ac4 force-pushed the include-unconfirmed-change-for-selection branch from 55f2115 to 5225ca8 Compare January 8, 2024 18:51

wizardsardine deleted a comment from jp1ac4 Jan 9, 2024

jp1ac4 mentioned this pull request Jan 17, 2024

commands: include missing amount in spend response #927

Merged

jp1ac4 force-pushed the include-unconfirmed-change-for-selection branch from 5225ca8 to db52c83 Compare January 18, 2024 16:27

jp1ac4 added 7 commits January 25, 2024 14:55

func test: use utils function

0450322

bitcoin: add ancestor size and fees to mempool entry

edbf00f

bitcoin: add mempool_entry to interface

5f00220

spend: increase candidate weight to pay for ancestor

94ef66c

commands: add ancestor info for user-selected unconfirmed coins

b05b0f1

commands: include unconfirmed change as candidates

62bb4ad

spend: return additional fee paid for ancestors

a38c173

spend: add warning about fee for ancestor

c0d4320

jp1ac4 force-pushed the include-unconfirmed-change-for-selection branch from db52c83 to c0d4320 Compare January 26, 2024 07:48

jp1ac4 marked this pull request as ready for review January 26, 2024 07:56

This was referenced Jan 26, 2024

Warn when adding the change output's value to fees. #811

Closed

gui(spend): display warnings for draft PSBTs #940

Merged

gui: use createspend command for coin selection and missing amount #947

Merged

darosior self-assigned this Feb 6, 2024

darosior reviewed Mar 9, 2024

View reviewed changes

darosior merged commit 1fe5acb into wizardsardine:master Mar 9, 2024
18 checks passed

jp1ac4 mentioned this pull request Mar 11, 2024

RBF follow-ups #853

Open

5 tasks

This was referenced Mar 11, 2024

Taproot support #985

Merged

Flaky test_coin_selection #1000

Closed

jp1ac4 mentioned this pull request Mar 11, 2024

commands: exclude immature coins from auto-selection #1001

Merged

jp1ac4 deleted the include-unconfirmed-change-for-selection branch March 11, 2024 17:07

jp1ac4 mentioned this pull request Nov 22, 2024

spend: include only those unconfirmed coins from self for coin selection #1484

Closed

		// Including unconfirmed external deposits would introduce a whole bunch
		// of additional complexity.

commands: include unconfirmed change as coin selection candidates #873

commands: include unconfirmed change as coin selection candidates #873

Conversation

jp1ac4 commented Dec 13, 2023

darosior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Dec 15, 2023

jp1ac4 commented Dec 15, 2023

pythcoiner commented Dec 20, 2023

jp1ac4 commented Jan 4, 2024

darosior commented Jan 4, 2024

jp1ac4 commented Jan 4, 2024

jp1ac4 commented Jan 4, 2024

darosior commented Jan 5, 2024

jp1ac4 commented Jan 8, 2024

darosior commented Jan 9, 2024 • edited Loading

jp1ac4 commented Jan 9, 2024

jp1ac4 commented Jan 10, 2024

darosior commented Jan 11, 2024

jp1ac4 commented Jan 11, 2024

jp1ac4 commented Jan 18, 2024

darosior commented Jan 23, 2024

jp1ac4 commented Jan 26, 2024

darosior left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darosior commented Jan 9, 2024 •

edited

Loading