[PoC/Bitcoin/Utxo] Correctly Consider Fees for UTXO Selection (#3667)

* change utxo selection logic for tw_utxo::Compiler

* small cleanup

* update selected utxos, init input_selection test module

* expand tests for change output generation

* handle change output correctly in fee estimation, expand tests

* calculate the effective fee rate, not the theoretical

* add extra sanity checks

* add input_selection_select_ascending test

* test for empty inputs and outputs, extend error variant

* additional comments and sanity checks

* simplify transaction builder for BRC20, update tests

* update reveal transaction signing in BRC20 builder

* remove weight and fee_estimate checks in tw_utxo, plan for later

* extra check for error

* add input_selection_insufficient_inputs test

* scrap the BitcoinPlanBuilder

* remove utils module

* cargo fmt

* calculate the effective fee after setting the change output, if enabled

* add UseAll selection tests, check for 'effective' fee

* remove fee_estimate.rs, covered in input_selection.rs

* use consts for alice and bob info

* do extra checks in compile_impl

* update change amount correctly in the native Transaction type before calculating sighash

* build index of private keys AFTER selecting UTXOs

* cargo fmt

* [BRC20]: Take Inscription amount as a string

* [BRC20]: Take Inscription amount as a string in C++

* [BRC20]: Add a test for BitcoinV2 bridge through the legacy `SigningInput`

---------

Co-authored-by: Satoshi Otomakan <[email protected]>

rust/tw_bitcoin/src/entry.rs

@@ -1,4 +1,3 @@
-use crate::modules::plan_builder::BitcoinPlanBuilder;
 use crate::modules::signer::Signer;
 use crate::{Error, Result};
 use bitcoin::address::NetworkChecked;
@@ -11,6 +10,7 @@ use tw_coin_entry::derivation::Derivation;
 use tw_coin_entry::error::{AddressError, AddressResult};
 use tw_coin_entry::modules::json_signer::NoJsonSigner;
 use tw_coin_entry::modules::message_signer::NoMessageSigner;
+use tw_coin_entry::modules::plan_builder::NoPlanBuilder;
 use tw_coin_entry::modules::wallet_connector::NoWalletConnector;
 use tw_coin_entry::prefix::NoPrefix;
 use tw_coin_entry::signing_output_error;
@@ -44,7 +44,7 @@ impl CoinEntry for BitcoinEntry {
 
     // Optional modules:
     type JsonSigner = NoJsonSigner;
-    type PlanBuilder = BitcoinPlanBuilder;
+    type PlanBuilder = NoPlanBuilder;
     type MessageSigner = NoMessageSigner;
     type WalletConnector = NoWalletConnector;
 
@@ -128,7 +128,7 @@ impl CoinEntry for BitcoinEntry {
 
     #[inline]
     fn plan_builder(&self) -> Option<Self::PlanBuilder> {
-        Some(BitcoinPlanBuilder)
+        None
     }
 }
 
@@ -147,14 +147,15 @@ impl BitcoinEntry {
             .map(crate::modules::transactions::InputBuilder::utxo_from_proto)
             .collect::<Result<Vec<_>>>()?;
 
-        // Convert output builders into Utxo outputs.
+        // Convert output builders into Utxo outputs (does not contain the change output).
         let mut utxo_outputs = proto
             .outputs
             .iter()
             .map(crate::modules::transactions::OutputBuilder::utxo_from_proto)
             .collect::<Result<Vec<_>>>()?;
 
-        // If automatic change output is enabled, a change script must be provided.
+        // If automatic change output creation is enabled (by default), a change
+        // script must be provided.
         let change_script_pubkey = if proto.disable_change_output {
             Cow::default()
         } else {
@@ -186,17 +187,23 @@ impl BitcoinEntry {
             disable_change_output: proto.disable_change_output,
         };
 
-        // Generate the sighashes to be signed.
+        // Generate the sighashes to be signed. This also selects the inputs
+        // according to the input selecter and appends a change output, if
+        // enabled.
         let utxo_presigning = tw_utxo::compiler::Compiler::preimage_hashes(utxo_signing);
         handle_utxo_error(&utxo_presigning.error)?;
 
-        // If a change output was created by the Utxo compiler, we return it here too.
-        if utxo_presigning.outputs.len() == utxo_outputs.len() + 1 {
+        // Check whether the change output is present.
+        if !proto.disable_change_output {
+            // Change output has been added.
+            debug_assert_eq!(utxo_presigning.outputs.len(), utxo_outputs.len() + 1);
+
             let change_output = utxo_presigning
                 .outputs
                 .last()
                 .expect("expected change output");
 
+            // Push it to the list of outputs.
             utxo_outputs.push(Proto::mod_PreSigningOutput::TxOut {
                 value: change_output.value,
                 script_pubkey: change_output.script_pubkey.to_vec().into(),
@@ -205,6 +212,10 @@ impl BitcoinEntry {
             })
         }
 
+        // Sanity check.
+        debug_assert!(utxo_presigning.inputs.len() <= proto.inputs.len());
+        debug_assert_eq!(utxo_presigning.outputs.len(), utxo_outputs.len());
+
         Ok(Proto::PreSigningOutput {
             error: Proto::Error::OK,
             error_message: Default::default(),
@@ -241,13 +252,14 @@ impl BitcoinEntry {
                 crate::modules::transactions::InputClaimBuilder::utxo_claim_from_proto(
                     input, signature,
                 )?;
+
             utxo_input_claims.push(utxo_claim);
         }
 
-        // Process all the outputs.
+        // Prepare all the outputs.
         let mut utxo_outputs = vec![];
-        for output in proto.outputs {
-            let utxo = crate::modules::transactions::OutputBuilder::utxo_from_proto(&output)?;
+        for output in &proto.outputs {
+            let utxo = crate::modules::transactions::OutputBuilder::utxo_from_proto(output)?;
 
             utxo_outputs.push(utxo);
         }
@@ -272,9 +284,13 @@ impl BitcoinEntry {
         let utxo_serialized = tw_utxo::compiler::Compiler::compile(utxo_preserializtion);
         handle_utxo_error(&utxo_serialized.error)?;
 
-        // Prepare `Proto::TransactionInput` protobufs for signing output.
+        let mut total_input_amount = 0;
+
+        // Prepare `Proto::TransactionInput` for end result.
         let mut proto_inputs = vec![];
         for input in utxo_input_claims {
+            total_input_amount += input.value;
+
             proto_inputs.push(Proto::TransactionInput {
                 txid: Cow::Owned(input.txid.to_vec()),
                 vout: input.vout,
@@ -288,7 +304,7 @@ impl BitcoinEntry {
             });
         }
 
-        // Prepare `Proto::TransactionOutput` protobufs for output.
+        // Prepare `Proto::TransactionOutput` for end result.
         let mut proto_outputs = vec![];
         for output in utxo_outputs {
             proto_outputs.push(Proto::TransactionOutput {
@@ -299,14 +315,29 @@ impl BitcoinEntry {
             });
         }
 
-        // Prepare `Proto::Transaction` protobuf for output.
+        // Prepare `Proto::Transaction` for end result.
         let transaction = Proto::Transaction {
             version: proto.version,
             lock_time: proto.lock_time,
             inputs: proto_inputs,
             outputs: proto_outputs,
         };
 
+        let total_output_amount = transaction
+            .outputs
+            .iter()
+            .map(|output| output.value)
+            .sum::<u64>();
+
+        // Sanity check.
+        debug_assert_eq!(transaction.inputs.len(), proto.inputs.len());
+        debug_assert_eq!(transaction.outputs.len(), proto.outputs.len());
+        // Every output is accounted for, including the fee.
+        debug_assert_eq!(
+            total_input_amount,
+            total_output_amount + utxo_serialized.fee
+        );
+
         // Return the full protobuf output.
         Ok(Proto::SigningOutput {
             error: Proto::Error::OK,
@@ -353,6 +384,7 @@ fn handle_utxo_error(utxo_err: &UtxoProto::Error) -> Result<()> {
         UtxoProto::Error::Error_missing_sighash_method => Proto::Error::Error_utxo_missing_sighash_method,
         UtxoProto::Error::Error_failed_encoding => Proto::Error::Error_utxo_failed_encoding,
         UtxoProto::Error::Error_insufficient_inputs => Proto::Error::Error_utxo_insufficient_inputs,
+        UtxoProto::Error::Error_no_outputs_specified => Proto::Error::Error_utxo_no_outputs_specified,
         UtxoProto::Error::Error_missing_change_script_pubkey => Proto::Error::Error_utxo_missing_change_script_pubkey,
     };
 

rust/tw_bitcoin/src/lib.rs

@@ -16,7 +16,6 @@ pub type Result<T> = std::result::Result<T, Error>;
 #[derive(Debug)]
 pub struct Error(Proto::Error);
 
-// TODO: We can improve this.
 impl Display for Error {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{:?}", self.0)

rust/tw_bitcoin/src/modules/mod.rs

@@ -1,5 +1,3 @@
 pub mod legacy;
-pub mod plan_builder;
 pub mod signer;
 pub mod transactions;
-mod utils;