Return UTXO struct from coin selection by LLFourn · Pull Request #159 · bitcoindevkit/bdk

LLFourn · 2020-11-11T02:58:20Z

The benefits of this PR are the following:

Removes this responsibility from the coin selection algorithms it was a bit awkward since the coin selection can't fill in any of the details in the TxIn anyway.
Preserves the metadata about the UTXOs that were selected so that they can passed down to complete_transaction. In most cases complete_transaction does not need to do another database lookup now because it has all the metadata already.
This makes it easier to introduce foreign UTXOs in the future.

LLFourn · 2020-11-11T03:02:42Z

 pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection;

+/// The weight of a transaction input with an empty witness and scriptSig
+pub const TXIN_BASE_WEIGHT: usize = 164;


It looks like @danielabrozzoni 's PR has done something similar here (and chosen the exact same varaible name 😎).

I think decomposing the value as there is a good idea.

I've added the description here (without the comment about electrs because I'm not sure what it is getting at).

In that other PR the "BASE_WEIGHT" had been originally set to prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) without counting the "+1" for the script len, because @danielabrozzoni assumed that it was included in the max_satisfaction_weight by rust-miniscript. This was making the end to end tests with the full node and electrs fail because the fees were just below min-relay-fee, because the weight was being underestimated by exactly 1 vbyte.

I'm still kinda thinking this could be a bug in rust-miniscript, because potentially longer scripts can require more than 1 byte for the length, since it should be a VarInt. So miniscript which is aware of the length should also add the correct extra weight for it. But in the meantime I guess we just have to workaround it and add our own "+1".

Maybe instead of the comment we should open an issue, just to make sure we don't forget about it

I'll remove the FIXME: in my PR and open an issue then :)

LLFourn · 2020-11-11T03:05:00Z

+            .into_iter()
+            .map(|utxo| (utxo.outpoint, utxo))
+            .collect();
+        let psbt = self.complete_transaction(tx, lookup_output, builder)?;


❓ @afilini what do you think about just building this hashmap inside complete transaction.

If you can find a nice way to do it, absolutely: IIRC I tried doing that while I was refactoring something but I was having some troubles in bump_fee because the selected vec only contained the extra added inputs, not all the inputs of the transaction, and that was breaking something in complete_transaction.

I did this. I don't see what you mean because the hashmap originally only had the "extra added inputs".

Ah then I guess I don't remember where I got stuck the last time 😂

Well, glad you managed to make it work though, that's the important thing!

afilini · 2020-11-12T13:51:08Z

                None => continue,
            };

+            let is_segwit = utxo.txout.script_pubkey.is_witness_program();


I'm not entirely sure about this, I think nested witness scripts here would be false, because the script_pubkey looks like a normal P2SH

Very good catch. I agree this is wrong. I am not sure if current behavior is right (see comment below). If we can figure it out I'll write tests to check for this.

I've just removed this bit for now and added a test to make sure we don't accidentally introduce the problem in the future.

afilini · 2020-11-12T13:57:38Z

+            .into_iter()
+            .map(|utxo| (utxo.outpoint, utxo))
+            .collect();
+        let psbt = self.complete_transaction(tx, lookup_output, builder)?;


If you can find a nice way to do it, absolutely: IIRC I tried doing that while I was refactoring something but I was having some troubles in bump_fee because the selected vec only contained the extra added inputs, not all the inputs of the transaction, and that was breaking something in complete_transaction.

LLFourn · 2020-11-12T23:07:16Z

-                        Some(prev_tx.output[prev_output.vout as usize].clone());
-                }
-                if !derived_descriptor.is_witness() || builder.force_non_witness_utxo {
-                    psbt_input.non_witness_utxo = Some(prev_tx);


My reading of the spec is that this should be here for wrapped p2sh segwit (it currently isn't):

The transaction in network serialization format the current input spends from. This should be present for inputs that spend non-segwit outputs and can be present for inputs that spend segwit outputs.

so is wrapped p2sh a "segwit output" well I'm not sure but the definition of PSBT_IN_WITNESS_UTXO makes sure to make it explicit that wrapped are in included:

The entire transaction output in network serialization which the current input spends from. This should only be present for inputs which spend segwit outputs, including P2SH embedded ones.

which would indicate that wrapped p2sh is not included in the first one and so non-segwit output should include wrapped p2sh....myabe?

I agree that the spec isn't super clear, but it sounds reasonable to me to consider wrapped outputs "segwit outputs", because their signatures will commit to the previous output's value just like they would do with native segwit outputs (BIP143).

That's basically the reason why you can get away with only including the previous utxo, and not the full transaction. Some wallets (I think Trezor and maybe others) will still require the full transaction even for native segwit outputs to mitigate some types of attacks (that's why there's one flag to always explicitly include it), but I think in general it shouldn't be required.

I agree that the spec isn't super clear, but it sounds reasonable to me to consider wrapped outputs "segwit outputs", because their signatures will commit to the previous output's value just like they would do with native segwit outputs (BIP143).

So I guess the reasoning is that since the witness in the PSBT is a wrapped segwit witness it must be a segwit output if it gets spent and therefore you don't need the previous transaction. Ok.

- We want to keep the metadata in the UTXO around for things later - It is easier to turn a UTXO into a TxIn outside

To avoid the caller having to do it.

LLFourn · 2020-11-17T04:21:12Z

Ready for final review I think. I've removed the modifications to complete_transaction (apart from building the output lookup inside the function).

LLFourn force-pushed the wallet/coin_select_return_utxo branch from 15d8704 to 24495ac Compare November 11, 2020 03:08

LLFourn commented Nov 11, 2020

View reviewed changes

LLFourn force-pushed the wallet/coin_select_return_utxo branch from 24495ac to 065acd3 Compare November 11, 2020 03:13

LLFourn mentioned this pull request Nov 11, 2020

[wallet] Add branch and bound coin selection #148

Merged

afilini reviewed Nov 12, 2020

View reviewed changes

danielabrozzoni mentioned this pull request Nov 12, 2020

How to calculate TXIN_BASE_WEIGHT? #160

Closed

LLFourn commented Nov 12, 2020

View reviewed changes

LLFourn force-pushed the wallet/coin_select_return_utxo branch 2 times, most recently from b98f923 to 4028fa3 Compare November 17, 2020 01:43

LLFourn added 3 commits November 17, 2020 15:11

[wallet] Make coin_select return UTXOs instead of TxIns

004f81b

- We want to keep the metadata in the UTXO around for things later - It is easier to turn a UTXO into a TxIn outside

[wallet] Add test that shwpkh populates witness_utxo

fcc408f

[wallet] Build output lookup inside complete transaction

35579cb

To avoid the caller having to do it.

LLFourn force-pushed the wallet/coin_select_return_utxo branch from 4028fa3 to 35579cb Compare November 17, 2020 04:12

afilini merged commit 35579cb into bitcoindevkit:master Nov 17, 2020

Conversation

LLFourn commented Nov 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afilini Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

afilini Nov 12, 2020 •

edited

Loading