Apply coin selection for spend #560

jp1ac4 · 2023-06-20T11:16:20Z

These are some initial changes towards #51.

I've added a selectcoinsforspend command that applies BnB coin selection using a waste metric.

This coin selection is then used in createspend if no coins are specified.

@darosior The changes are still in their early stages so I'm creating this draft PR to facilitate discussion about the approach in general and specific details.

darosior

I need to look more into the tradeoffs of various coin selection algorithms to review the strategy used here, but the approach of selecting coins if none are passed in createspend looks good to me.

darosior · 2023-06-20T12:05:54Z

src/commands/mod.rs

+    /// Select coins for spend.
+    pub fn select_coins_for_spend(


I know you've made it a command to test, but in the end i think it should be a helper. Probably in src/commands/utils.

darosior · 2023-06-20T12:52:30Z

src/commands/mod.rs

+        &self,
+        spend_amount: bitcoin::Amount,
+        spend_feerate_vb: u64,
+        long_term_feerate_vb: u64,


We should probably make this a constant.

That sounds good. If I understand well, bitcoin core uses a value based on the config option -consolidatefeerate, which has a default value of 10 sat/vbyte:

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.h#L87

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L842

We could follow a similar approach, giving the user the option to set their own value if they wish. I think using "consolidation" instead of "long-term" in the name would make it easier to reason about for the user when choosing a value.

Another option is to use a rolling average of the feerate of the user's previous transactions. I think this will lead to reasonable estimates about whether the current feerate is higher and lower than what they usually experience. Of course you need to have a reasonable sample size over enough time to have good results.

Good point. @jp1ac4 let's stick with a constant for now (we'll need one as a fallback anyways), we can implement what Lloyd suggests in a follow-up.

darosior · 2023-06-20T12:55:41Z

src/commands/mod.rs

+
+        let cs = CoinSelector::new(&candidates, base_weight);
+        // TODO: use our own custom metric function.
+        let solutions = cs.branch_and_bound(Waste {


Using the waste metric for coin selection could lead to huge consolidations. See for instance this comment from upstream (and the linked stackexchange discussion of the algorithm): https://github.com/bitcoindevkit/bdk/pull/924/files#diff-f1378f655c539699f8c398a3b3a11d02c939875b3e2d9fdb2a9685b9202fd70fR4-R18.

We should probably do some consolidation for Liana but i'm afraid using only the waste metric could lead to unreasonable selections.

That's true, especially when the long-term feerate is higher than the spend feerate. I'll give it further thought.

As an additional safeguard, for now at least, we could let the user in the GUI edit the list of automatically selected coins when creating a new spend transaction.

Yes I still think that waste by itself is a pretty bad default. To get this effect of "some consolidation" I like the idea of combining waste with a "minimize fee" metric that takes into account the fee paid when spending a change output as part of the cost. I'll try and add this weighted combination feature. I expect that finding via branch and bound a weighted combination of two metrics will be less efficient in practice because the lower bound estimation will be too loose. In other words the combined estimator may predict there will be a low waste and a low fee on some child branch but these local minimums will occur at different selections and so there really is no selection that has a low waste and low fee. This will cause the BnB algorithm to end up exploring a lot of child branches unnecessarily. At worst this degenerates to exhaustive search but hopefully it won't be too bad.

The other step is of course to add the minimize fee metric which is by itself a good default.

LLFourn

Thanks for trying out my bdk coin selection work. It was really helpful to see what was unclear for improving and documenting the API. Cheers! I left some comments.

LLFourn · 2023-06-21T03:17:27Z

src/commands/mod.rs

+            .enumerate()
+            .filter_map(|(i, sol)| Some((i, sol?)))
+            .last()
+            .expect("it should have found solution");


I don't think you necessarily know that it would find a solution here? What if you don't have enough coins.

Also for a large enough number of inputs this could take forever (literally) to return (especially if there is no solution) so you should always .take(100_000) or something on this iterator so that it terminates after 100_000 iterations or so.

I think I should add an explicit API for doing:

Run this X amount of times and give me the best solution

If no best solution according to metric just select coins until satisfied

otherwise return error with the amount of sats we were missing.

LLFourn · 2023-06-21T03:26:39Z

src/commands/mod.rs

+
+        let cs = CoinSelector::new(&candidates, base_weight);
+        // TODO: use our own custom metric function.
+        let solutions = cs.branch_and_bound(Waste {


Yes I still think that waste by itself is a pretty bad default. To get this effect of "some consolidation" I like the idea of combining waste with a "minimize fee" metric that takes into account the fee paid when spending a change output as part of the cost. I'll try and add this weighted combination feature. I expect that finding via branch and bound a weighted combination of two metrics will be less efficient in practice because the lower bound estimation will be too loose. In other words the combined estimator may predict there will be a low waste and a low fee on some child branch but these local minimums will occur at different selections and so there really is no selection that has a low waste and low fee. This will cause the BnB algorithm to end up exploring a lot of child branches unnecessarily. At worst this degenerates to exhaustive search but hopefully it won't be too bad.

The other step is of course to add the minimize fee metric which is by itself a good default.

LLFourn · 2023-06-21T03:29:56Z

src/commands/mod.rs

+
+            // TODO: check what weights we need to add to `TXIN_BASE_WEIGHT` to give this candidate's weight
+            let added_weight =
+                coin_desc.witness_script().len() + (coin_desc.script_pubkey().len() * 4);


This should be the satisfaction weight i.e. witness weight. Miniscript can calculate the max_weight_to_satisfy which should work well. We're working on a more fine grained way of estimating satisfaction weight in rust-bitcoin/rust-miniscript#481 rather than just using the maximum (e.g. tr key path spends will be much cheaper than particular leaves so using the max can overshoot badly in some cases).

LLFourn · 2023-06-21T03:37:33Z

src/commands/mod.rs

+        &self,
+        spend_amount: bitcoin::Amount,
+        spend_feerate_vb: u64,
+        long_term_feerate_vb: u64,


Another option is to use a rolling average of the feerate of the user's previous transactions. I think this will lead to reasonable estimates about whether the current feerate is higher and lower than what they usually experience. Of course you need to have a reasonable sample size over enough time to have good results.

LLFourn · 2023-06-21T03:40:37Z

src/commands/mod.rs

+        let drain = Drain {
+            weight: TXOUT_BASE_WEIGHT,
+            spend_weight: TXIN_BASE_WEIGHT,
+            value: 0, // value is ignored when using `change_policy::min_value` below


We probably should have a version of Drain like DrainWeight to avoid having to make comments like these.

LLFourn · 2023-06-21T03:53:12Z

src/commands/mod.rs

+        };
+        // Ensure any change output is not too small.
+        let change_policy = change_policy::min_value(drain, DUST_OUTPUT_SATS);
+        // TODO: is this the correct `base_weight` for `CoinSelector`?


You need to add the outputs.

We should probably add a fund_outputs(&[(weight, value)) constructor which should be used by default where you pass in the weights of the outputs you are trying to fund and their values so you don't have to construct an actual bitcoin::Transaction.

Ah yes thanks, I was thinking only about change outputs and forgot about including the non-change outputs :)

We should probably add a fund_outputs(&[(weight, value)) constructor which should be used by default where you pass in the weights of the outputs you are trying to fund and their values so you don't have to construct an actual bitcoin::Transaction.

That sounds good. I'll look out for this in your PR if you have a chance to include it.

LLFourn · 2023-06-21T03:55:36Z

src/commands/mod.rs

+        let target = Target {
+            value: spend_amount.to_sat(),
+            feerate,
+            min_fee: 0, // TODO: Perhaps for RBF `min_fee` will need to be passed as argument to ensure absolute fee of replacement is high enough.


Indeed. This is one of the main reasons this exists on Target.

jp1ac4 · 2023-06-21T10:31:17Z

Thanks for trying out my bdk coin selection work. It was really helpful to see what was unclear for improving and documenting the API. Cheers! I left some comments.

Hi @LLFourn, thanks very much for looking through the changes and adding comments. I really appreciate it. I'll go through those and reply as required.

darosior · 2023-06-27T10:03:10Z

src/commands/mod.rs

+        let final_coins_outpoints: &[bitcoin::OutPoint] = if coins_outpoints.is_empty() {
+            log::info!("No outpoints specified. Selecting coins...");
+            let candidate_coins: Vec<Vec<Coin>> = db_conn
+                // TODO: Will replacement transactions select coins here or elsewhere?


A possibility for replacement transactions is to select all confirmed unspent coins and then to add the coins spent by the to-be-replaced transaction to the vector.

I've removed this comment for now as I will leave RBF considerations for a separate PR.

darosior · 2023-06-27T10:28:36Z

src/commands/utils.rs

+    candidate_coins: Vec<Vec<Coin>>, // Each inner vector of coins should be spent together
+    destinations: &HashMap<bitcoin::Address, u64>,
+    feerate_vb: u64,
+    min_fee: u64,


Since it's optional (only used for RBF AFAICT) it should be an Option instead of setting it to 0.

For now I've removed this parameter completely as I'm still not sure how the RBF interface will work and so will focus on non-replacement transactions here.

Oops, I just added min_fee again and this comment re-appeared 😄

Would you prefer this to be Option still?

darosior · 2023-06-27T10:34:09Z

src/commands/utils.rs

+    }
+    let max_input_weight =
+        TXIN_BASE_WEIGHT + control.config.main_descriptor.max_sat_weight() as u32;
+    // TODO: more accurate input weight estimate using https://github.com/rust-bitcoin/rust-miniscript/pull/481


I don't think it should be a TODO, as it's pretty far down the road and quite unclear how we'd use it.

That's true, I've removed it now.

darosior · 2023-06-27T10:34:26Z

src/commands/utils.rs

+            input_count: coins.len(),
+            value: coins.iter().map(|c| c.amount.to_sat()).sum(),
+            weight: max_input_weight * coins.len() as u32,
+            is_segwit: true, // TODO: can we assume this or need to check for each coin?


Yes, Liana only ever supports receiving on Segwit scripts.

Perfect thanks, I've updated the comment.

darosior · 2023-06-27T11:51:18Z

src/commands/utils.rs

+    // Transaction base weight calculated from transaction with no inputs
+    let base_weight = bitcoin::Transaction {
+        input: Vec::new(),
+        output: destinations
+            .iter()
+            .map(|(address, value)| bitcoin::TxOut {
+                value: *value,
+                script_pubkey: address.script_pubkey(),
+            })
+            .collect(),
+        lock_time: bitcoin::PackedLockTime(0),
+        version: 2,
+    }
+    .weight() as u32;


This would not account for the Segwit marker flag nor the size of the witnesses. But the CoinSelector is taking care of this. May be worth a comment as i had to double check it does take care of it.

Ah thanks, I've added a comment.

darosior · 2023-06-27T12:16:35Z

src/commands/utils.rs

+        .take(100_000); // Ensure iterator does not take too long or run forever, e.g. if there are no solutions.
+
+    // TODO: use explicit API to handle different cases of solution being found (https://github.com/wizardsardine/liana/pull/560#discussion_r1236232805)
+    if let Some(Some(best)) = solutions.filter(|sol| sol.is_some()).last() {


Any reason for taking the last? I guess it doesn't matter if the API gets reworked anyways.

I think this should correspond to the solution with best metric score, as the iterator only returns solutions that give a lower score than the best score so far and otherwise returns Some(None). But we can check again once the reworked API is ready.

darosior · 2023-06-27T12:23:00Z

src/commands/utils.rs

+
+/// Select coins for spend.
+pub fn select_coins_for_spend(
+    control: &DaemonControl,


This is a bit ugly to pass a reference to the caller here, i'd say just pass in the maximum satisfaction weight which is the only thing you use it for anyways.

I agree and have now changed this.

jp1ac4 · 2023-06-28T07:34:32Z

Hi @darosior, regarding GUI integration, I think there are two options:

We could add an "auto-select" button to the coin selection area of the Send page, which would then show the auto-selected coins (as if the user had ticked them manually) in the list of coins and allow further editing by the user. This would provide a safeguard against over-consolidation, for example, and give the user transparency about which coins are being used in the transaction before creating it. For this option, we might need to add a standalone command for selecting coins.
Alternatively, when using auto-select, a new transaction would be created without the user first seeing which coins have been selected, so we would skip the part of the Send page that lists (selected) coins.

Did you have in mind one of these options?

darosior · 2023-06-28T08:10:03Z

Hey, yes. Thanks for bringing that up.

I think we should always auto-select coins and the user can manually opt to select different ones. I don't think this needs another command, we might as well reuse createspend since we need all the same arguments (destinations, feerate). I'm not too familiar with the GUI logic but i had in mind something like: "once they've filled in the destination(s) and the feerate, run a createspend and tick the coins in the list that were chosen". If they update the feerate or the destination, we need to update the selected coins too. If they start ticking coins before they fill-in the destinations and feerate i don't know, probably opt-out of automated coin selection. Does that make sense?

Also, you probably want to work on the GUI integration in another PR. So you can update the liana dependency to this branch and later point it back to master.

jp1ac4 · 2023-06-28T08:30:53Z

Yep that makes sense thanks. I agree that it's better to give the user the option to edit the selection. And it's true that a separate command shouldn't be required as we can determine the selected coins from the return value of createspend, so I'll start with this approach and see how it goes.

Also, you probably want to work on the GUI integration in another PR. So you can update the liana dependency to this branch and later point it back to master.

Oh yes good point, I'll add GUI changes in a separate branch and PR.

jp1ac4 · 2023-06-28T10:22:51Z

I've added a basic proof of concept for the GUI in https://github.com/jp1ac4/liana/tree/add-auto-coin-selection-to-gui.

As the approach discussed above seems like it should work, I'll leave the GUI changes for now and will focus on adding tests to this current PR.

darosior · 2023-06-28T13:42:30Z

Regarding the failure in the Rust v1.48 CI i've just merged #561 which bumps our MSRV from 1.48 to 1.54. I guess it could help.

jp1ac4 · 2023-06-28T14:35:34Z

Thanks for that. I've rebased on master (and took the chance to squash my commits as I was getting a conflict otherwise from an intermediate change) and all checks are now passing 💯

darosior · 2023-07-13T09:41:46Z

I created a couple conflicts by upgrading rust-miniscript and rust-jsonrpc, but they should be trivial.

jp1ac4 · 2023-07-13T11:57:40Z

I've now rebased on master and resolved the conflicts.

darosior · 2023-07-13T12:03:46Z

Thanks.

For anyone following along: i discussed it with @jp1ac4 elsewhere and we decided to put this on hold until upstream is merged. Efforts will be moved elsewhere for now but we'll get back to this eventually.

LLFourn · 2023-07-14T02:44:33Z

Thanks the effort here was really useful for me to learn some strengths/weaknesses of the API.

jp1ac4 · 2023-10-18T09:02:37Z

I've updated my previous changes based on the changes in bitcoindevkit/bdk#1072.

Now, the select_coins_for_spend function uses the LowestFee metric and min_value_and_waste change policy. If no BnB solution is found, it will attempt to select coins by descending value. It also returns the change amount for use when creating the spend transaction in create_spend.

As well as any general refactoring and improvements, I still need to add tests.

176859e commands: add weights before converting to vbytes (jp1ac4) Pull request description: The `sanity_check_psbt()` function checks, among other things, that the feerate in sats/vbytes is in the required range. When estimating the transaction size, converting each part to vbytes first can lead to a larger result due to rounding multiple times and therefore a lower feerate, which could fall below 1 and fail the sanity check. This issue has arisen in #560 when creating a transaction using coin selection. ACKs for top commit: darosior: utACK 176859e -- great catch Tree-SHA512: 5e4fc795c5977c71a0f02b21c09cd71d59c9412f94bcafd5a4adb429c4ae3a568a25c4174bd305e0be927f06ae4e9be9aef791ffbbee2a7ac1c40740f639ab10

jp1ac4 · 2023-10-23T12:33:12Z

This is now ready for an initial review.

I've moved some existing code in create_spend() to the top in order to reuse variables when calling select_coins_for_spend().

jp1ac4 · 2023-10-25T10:19:11Z

Rebased on master.

jp1ac4 · 2023-10-30T09:36:58Z

src/commands/mod.rs

+        let final_coins_outpoints: &[bitcoin::OutPoint] = if use_coin_selection {
+            log::info!("No outpoints specified. Selecting coins...");
+            let candidate_coins: Vec<Coin> = db_conn
+                .coins(&[CoinStatus::Confirmed], &[])


I think I should also include CoinStatus::Unconfirmed here to be consistent with the coins available for selection in the GUI.

darosior · 2023-10-30T09:50:04Z

Regarding considering unconfirmed coins.

Considering unconfirmed coins in the selection makes things more difficult. Now we need to pay fees for the unconfirmed parent as well, which also affects the cost of including an input. And we need to be careful not to create a too large unconfirmed chain of transactions. Also unconfirmed coins shouldn't be introduced in a replacement transaction once we do RBF.

I don't think we should introduce the complexity of correctly dealing with unconfirmed transactions just yet. This leaves us with two imperfect possibilities:

Include unconfirmed coins, but treat it as though they were confirmed. This may result in undershooting the fees, potentially creating a too long transaction chain as the wallet gets used but nothing confirms.
Don't include unconfirmed coins and fallback to manual coin selection if we can't find a solution. In this case we should disclaim it to the user somehow.

I'm in favour of the latter, but i'm happy to reconsider.

jp1ac4 · 2023-10-30T10:07:38Z

Ah I see, I'm also happy with the latter approach in that case. We can consider any extra information to give the user as part of #563.

jp1ac4 · 2023-10-30T10:20:47Z

This may not be a problem, but I've found that when a transaction will give a change amount close to the dust threshold of 5000 sats, there can be a difference between the transaction created with auto vs manual selection, even if the same coins are used as inputs.

I created two PSBTs with a target feerate of 10 sats/vb.

This one used auto selection and gave change amount of 5000 sats:

Here, I selected the same coins manually and there was no change output, leading to a higher estimated feerate:

I signed and broadcast the first PSBT to make sure its feerate was not lower than the 10 sats/vb:

The feerate was 10.7 sats/vb according to https://mempool.space/signet/tx/6f3df02ced55effd873df27ed7a266bda729d41d905bbfbf49d313672b162b4f.

jp1ac4 · 2023-10-30T15:15:28Z

Based on further discussion, I'll change create_spend() so that we determine the change amount using bdk_coin_select::change_policy for both manual and auto selection, which should ensure consistency when transaction inputs are the same.

darosior · 2023-10-31T10:22:28Z

Rebased it for you here during my review, if that helps: https://github.com/darosior/liana/tree/rebased_pr_560.

jp1ac4 · 2023-10-31T11:02:58Z

I've just pushed my latest changes, which include some failing tests that I'm still investigating.

The main difference is that now the select_coins_for_spend() function is used in both manual and auto cases to ensure consistency in change amount. For manual coin selection, the specified coins are pre-selected and not chosen by BnB so that only the change amount is left to determine.

I've removed some code from create_spend() that was used to determine change amount.

darosior

Approach ACK. Consolidating the change output calculation is also a great improvement.

I've got a number of review comments but i didn't find any bug, it's overall pretty well though-out. I've started cleaning up and adding comments to a bunch of things as i was playing with the code in this PR. Pushed those here if you'd like to include some of the changes: https://github.com/darosior/liana/tree/pr_560_modified. It also addresses most of my review comments.

Haven't looked at the tests yet, will continue playing with this a bit more.

src/commands/mod.rs

src/commands/utils.rs

src/commands/mod.rs

src/commands/utils.rs

src/commands/mod.rs

jp1ac4 · 2023-11-03T11:12:45Z

I've mostly addressed your comments now in 2a1fcd9, including with changes from your branch (https://github.com/darosior/liana/tree/pr_560_modified - thanks 🙏).

I still need to fix the failing functional test. Will squash commits again once no more changes are planned.

jp1ac4 · 2023-11-09T09:50:07Z

It can happen that attempting a self-send of a coin near the dust limit will result in a transaction with no outputs, which then returns a SanityCheckFailure with a message saying it's a bug that should be reported. Previously, this would return an InsufficientFunds error. Should we check for a self-send with no outputs before calling the sanity check and return InsufficientFunds?

I still need to fix the failing functional test.

This has now been fixed. It was related to the order of transaction inputs no longer being guaranteed to be the same as the order in the coins_outpoints parameter.

darosior · 2023-11-09T10:35:26Z

Should we check for a self-send with no outputs before calling the sanity check and return InsufficientFunds?

Yes. Good catch.

jp1ac4 · 2023-11-09T12:19:44Z

I've now added a check for a self-send with no change and updated the test. This check was there before and I had accidentally removed it as part of the other updates to create_spend.

I also updated a comment and removed an unused variable.

When creating a new spend, if coin outpoints are not provided, then coins will be selected automatically. This automatic selection is such that the transaction fee is minimized, taking into account the cost of creating any change output now and the cost of spending it in the future. If change is added, it must reduce the transaction waste and be above the dust threshold. This same policy is applied also in the case of manual coin selection, replacing the previous logic for determining the change amount. This ensures that creating a spend with auto-selection and another with manual selection using the same auto-selected coins will give the same change amount.

jp1ac4 · 2023-11-14T13:44:15Z

The first force-push to 4bca5c8 included the following changes:

update bdk_coin_select dependency commit hash
remove unused out_value variable and edit comment
move conversion of feerate_vb to f32 to create_spend and return error if it fails

The second force-push to cfa0f91 was to rebase on master as there was a conflict.

darosior

ACK cfa0f91

One thing i wanted to convince myself of is that we would never create a too-low-fee transaction which would never broadcast. I tested creating randomly sized transactions spending random coin values at 1sat/vb of feerate. Never failed so far. My hacks:

diff --git a/tests/test_spend.py b/tests/test_spend.py
index 8ad1849..e91ad41 100644
--- a/tests/test_spend.py
+++ b/tests/test_spend.py
@@ -322,3 +322,55 @@ def test_coin_selection(lianad, bitcoind):
     assert auto_psbt.tx.vout[0].scriptPubKey == manual_psbt.tx.vout[0].scriptPubKey
     # Change amount is the same (change address will be different).
     assert auto_psbt.tx.vout[1].nValue == manual_psbt.tx.vout[1].nValue
+
+    # Get rid of the leftover coins.
+    res = lianad.rpc.createspend({bitcoind.rpc.getnewaddress(): 69_450}, [], 1)
+    psbt = PSBT.from_base64(res["psbt"])
+    assert len(psbt.o) == 1
+    signed_psbt = lianad.signer.sign_psbt(psbt)
+    lianad.rpc.updatespend(signed_psbt.to_base64())
+    spend_txid = signed_psbt.tx.txid().hex()
+    lianad.rpc.broadcastspend(spend_txid)
+    bitcoind.generate_block(1, wait_for_mempool=spend_txid)
+    wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 0)
+
+    import random, logging
+    descs = bitcoind.rpc.listdescriptors()["descriptors"]
+    bitcoind_desc = next(d["desc"] for d in descs if d["desc"].startswith("wpkh"))
+    bitcoind_addrs = bitcoind.rpc.deriveaddresses(bitcoind_desc, 10_000)
+    bitcoind.generate_block(5)
+    for _ in range(100):
+        # Receive a random value coin.
+        value = random.randrange(int(COIN / 10_000), 100 * COIN)
+        addr = lianad.rpc.getnewaddress()["address"]
+        deposit = bitcoind.rpc.sendtoaddress(addr, value / COIN)
+        bitcoind.generate_block(1, wait_for_mempool=deposit)
+        wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 1)
+
+        # Create a randomly sized set of destinations to make the size of the transaction vary.
+        # A single input single output tx is 127 vbytes. Compute the fee accounting for 31 vb per
+        # added output. This is the minimum possible fee attached before createspend starts failing.
+        dest_count = random.randrange(1, 3_000 if value >= COIN else 100)
+        rest = value % dest_count
+        out_value = int(value // dest_count)
+        fee = 127 + 31 * (dest_count - 1)
+        if dest_count >= 0xfd:
+            fee += 2  # out count compactsize len
+        dest = {bitcoind_addrs[0]: out_value + rest - fee}
+        dest.update({bitcoind_addrs[i + 1]: out_value for i in range(dest_count - 1)})
+
+        logging.info(f"Value: {value / COIN} BTC. Dest count: {dest_count}. Fee: {fee}.")
+
+        # Create a 1sat/vbyte PSBT. It never gets created with a feerate lower than 1sat/vb.
+        # We can always sign and broadcast it.
+        res = lianad.rpc.createspend(dest, [], 1)
+        logging.debug(f"PSBT: {res['psbt']}")
+        psbt = PSBT.from_base64(res["psbt"])
+        assert len(psbt.o) == dest_count
+        signed_psbt = lianad.signer.sign_psbt(psbt)
+        lianad.rpc.updatespend(signed_psbt.to_base64())
+        spend_txid = signed_psbt.tx.txid().hex()
+        lianad.rpc.broadcastspend(spend_txid)
+        logging.debug(f"Mem entry: {bitcoind.rpc.getmempoolentry(spend_txid)}")
+        bitcoind.generate_block(1, wait_for_mempool=spend_txid)
+        wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 0)

darosior · 2023-11-15T15:47:49Z

tests/test_rpc.py

+    # The transaction must contain the spent transaction for each input.
+    # We don't make assumptions about the ordering of PSBT inputs.
+    assert sorted(
+        [psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in spend_psbt.i]
+    ) == sorted(
+        [bytes.fromhex(bitcoind.rpc.gettransaction(op[:64])["hex"]) for op in outpoints]
+    )


nit, for next time: no need to create a temporary list in this case.

diff --git a/tests/test_rpc.py b/tests/test_rpc.py index e492da6..5fff40c 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -402,9 +402,9 @@ def test_create_spend(lianad, bitcoind): # The transaction must contain the spent transaction for each input. # We don't make assumptions about the ordering of PSBT inputs. assert sorted( - [psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in spend_psbt.i] + psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in spend_psbt.i ) == sorted( - [bytes.fromhex(bitcoind.rpc.gettransaction(op[:64])["hex"]) for op in outpoints] + bytes.fromhex(bitcoind.rpc.gettransaction(op[:64])["hex"]) for op in outpoints ) # We can sign it and broadcast it.

darosior · 2023-11-15T15:55:22Z

tests/test_spend.py

+
+    # Now create a transaction with manual coin selection using the same outpoints.
+    outpoints = [
+        f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin


nit:

Suggested change

f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

f"{txin.prevout.hash:64x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

I don't think the 0 is needed?

Actually i tested and neither is the 64:

Suggested change

f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

f"{txin.prevout.hash:x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

darosior · 2023-11-15T15:56:43Z

tests/test_spend.py

+def test_coin_selection(lianad, bitcoind):
+    """We can create a spend using coin selection."""


Could use some insane value sanity checks. But i'll do some of those when tackling #800.

darosior · 2023-11-15T17:22:11Z

I'm going to merge this. I think it's in good shape, and we are going to continue testing it anyways (with the GUI integration and the RBF work). We can also update to the new upstream API in a followup.

Congrats on getting this almost-5-months old PR to the finish line @jp1ac4!

9e2407e gui: auto-select coins for spend (jp1ac4) Pull request description: This adds automated coin selection from #560 to the GUI. The new "Auto-select" button uses automated coin selection to select coins for a spend, which the user can then modify as required. The button can only be clicked once the recipients and feerate have been validated, and it does not appear when creating a self-send. I haven't added any validation on the amount before making the button clickable, but it may be hard to anticipate all coin selection errors once fees are taken into consideration. ACKs for top commit: darosior: light ACK 9e2407e Tree-SHA512: 6ce3389d849470b3beb6ac8df75d2c3b7b6c04ee881dd0e9116c4d87f54376a8ed6666cbfd0ff0152a3eb839c8f7f17a175fe078ef030f03451430b84ab40cb6

darosior reviewed Jun 20, 2023

View reviewed changes

darosior mentioned this pull request Jun 20, 2023

Introduce new bdk_coin_select implementation bitcoindevkit/bdk#924

Closed

LLFourn reviewed Jun 21, 2023

View reviewed changes

darosior reviewed Jun 27, 2023

View reviewed changes

jp1ac4 force-pushed the auto-coin-selection branch from d714cea to b561dd6 Compare June 28, 2023 14:07

This was referenced Jun 30, 2023

doc(api): fix order of params for createspend #556

Merged

gui: Add automated coin selection for spend #563

Merged

jp1ac4 force-pushed the auto-coin-selection branch from 337515e to 7a78f00 Compare July 13, 2023 11:43

evanlinjin mentioned this pull request Aug 9, 2023

Introduce new bdk_coin_select implementation bitcoindevkit/bdk#1072

Closed

14 tasks

jp1ac4 force-pushed the auto-coin-selection branch from 7a78f00 to c3b85b1 Compare October 18, 2023 08:48

jp1ac4 force-pushed the auto-coin-selection branch from c3b85b1 to 92cef7b Compare October 18, 2023 15:48

jp1ac4 mentioned this pull request Oct 20, 2023

commands: add weights before converting to vbytes #734

Merged

jp1ac4 force-pushed the auto-coin-selection branch 2 times, most recently from 704d824 to 243e044 Compare October 23, 2023 12:16

jp1ac4 marked this pull request as ready for review October 23, 2023 12:30

jp1ac4 changed the title ~~[WIP] - Apply coin selection for spend~~ Apply coin selection for spend Oct 23, 2023

jp1ac4 force-pushed the auto-coin-selection branch from 243e044 to 1fcb332 Compare October 25, 2023 10:17

jp1ac4 commented Oct 30, 2023

View reviewed changes

jp1ac4 force-pushed the auto-coin-selection branch from 1fcb332 to e6f08e7 Compare October 31, 2023 10:51

darosior reviewed Oct 31, 2023

View reviewed changes

darosior mentioned this pull request Nov 3, 2023

RPC commands: sanity check given numbers values #800

Open

jp1ac4 force-pushed the auto-coin-selection branch 2 times, most recently from 5aab76f to b090618 Compare November 6, 2023 14:50

jp1ac4 mentioned this pull request Nov 8, 2023

RBF support for Spend transactions #43

Closed

jp1ac4 force-pushed the auto-coin-selection branch from ae5c1c3 to 4bca5c8 Compare November 14, 2023 13:26

jp1ac4 force-pushed the auto-coin-selection branch from 4bca5c8 to cfa0f91 Compare November 14, 2023 13:34

darosior approved these changes Nov 15, 2023

View reviewed changes

darosior merged commit 2d303b1 into wizardsardine:master Nov 15, 2023
18 checks passed

jp1ac4 deleted the auto-coin-selection branch November 15, 2023 19:51

darosior mentioned this pull request Nov 20, 2023

Let coin selection use (some?) unconfirmed coins #826

Closed

	f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin
	f"{txin.prevout.hash:64x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

	f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin
	f"{txin.prevout.hash:x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin

		def test_coin_selection(lianad, bitcoind):
		"""We can create a spend using coin selection."""

Apply coin selection for spend #560

Apply coin selection for spend #560

Conversation

jp1ac4 commented Jun 20, 2023

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

LLFourn Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

darosior Jun 27, 2023 • edited Loading

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

LLFourn 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

LLFourn Jun 21, 2023 • edited Loading

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

jp1ac4 commented Jun 21, 2023

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

jp1ac4 commented Jun 28, 2023

darosior commented Jun 28, 2023

jp1ac4 commented Jun 28, 2023

jp1ac4 commented Jun 28, 2023

darosior commented Jun 28, 2023

jp1ac4 commented Jun 28, 2023

darosior commented Jul 13, 2023

jp1ac4 commented Jul 13, 2023

darosior commented Jul 13, 2023

LLFourn commented Jul 14, 2023

jp1ac4 commented Oct 18, 2023

jp1ac4 commented Oct 23, 2023

jp1ac4 commented Oct 25, 2023

Choose a reason for hiding this comment

darosior commented Oct 30, 2023

jp1ac4 commented Oct 30, 2023

jp1ac4 commented Oct 30, 2023

jp1ac4 commented Oct 30, 2023

darosior commented Oct 31, 2023

jp1ac4 commented Oct 31, 2023

darosior left a comment

Choose a reason for hiding this comment

jp1ac4 commented Nov 3, 2023

jp1ac4 commented Nov 9, 2023

darosior commented Nov 9, 2023

jp1ac4 commented Nov 9, 2023

jp1ac4 commented Nov 14, 2023

darosior left a comment • edited Loading

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 Nov 15, 2023

LLFourn Jun 21, 2023 •

edited

Loading

darosior Jun 27, 2023 •

edited

Loading

LLFourn Jun 21, 2023 •

edited

Loading

darosior left a comment •

edited

Loading