Merge branch 'bitcoin-audit' into develop

coins/bitcoin/src/crypto.rs

@@ -32,13 +32,14 @@ pub fn x(key: &ProjectivePoint) -> [u8; 32] {
   (*encoded.x().expect("point at infinity")).into()
 }
 
-/// Convert a non-infinite even point to a XOnlyPublicKey. Panics on invalid input.
+/// Convert a non-infinity even point to a XOnlyPublicKey. Panics on invalid input.
 pub fn x_only(key: &ProjectivePoint) -> XOnlyPublicKey {
-  XOnlyPublicKey::from_slice(&x(key)).unwrap()
+  XOnlyPublicKey::from_slice(&x(key)).expect("x_only was passed a point which was infinity or odd")
 }
 
-/// Make a point even by adding the generator until it is even. Returns the even point and the
-/// amount of additions required.
+/// Make a point even by adding the generator until it is even.
+///
+/// Returns the even point and the amount of additions required.
 pub fn make_even(mut key: ProjectivePoint) -> (ProjectivePoint, u64) {
   let mut c = 0;
   while key.to_encoded_point(true).tag() == Tag::CompressedOddY {
@@ -51,8 +52,10 @@ pub fn make_even(mut key: ProjectivePoint) -> (ProjectivePoint, u64) {
 /// A BIP-340 compatible HRAm for use with the modular-frost Schnorr Algorithm.
 ///
 /// If passed an odd nonce, it will have the generator added until it is even.
+///
+/// If the key is odd, this will panic.
 #[derive(Clone, Copy, Debug)]
-pub struct Hram {}
+pub struct Hram;
 
 lazy_static! {
   static ref TAG_HASH: [u8; 32] = Sha256::digest(b"BIP0340/challenge").into();
@@ -145,7 +148,8 @@ impl<T: Sync + Clone + Debug + Transcript> Algorithm<Secp256k1> for Schnorr<T> {
       // s = r + cx. Since we added to the r, add to s
       sig.s += Scalar::from(offset);
       // Convert to a secp256k1 signature
-      Signature::from_slice(&sig.serialize()[1 ..]).unwrap()
+      Signature::from_slice(&sig.serialize()[1 ..])
+        .expect("couldn't convert SchnorrSignature to Signature")
     })
   }
 

coins/bitcoin/src/rpc.rs

@@ -1,10 +1,13 @@
 use core::fmt::Debug;
+use std::collections::HashSet;
 
 use thiserror::Error;
 
 use serde::{Deserialize, de::DeserializeOwned};
 use serde_json::json;
 
+use reqwest::Client;
+
 use bitcoin::{
   hashes::{Hash, hex::FromHex},
   consensus::encode,
@@ -26,23 +29,75 @@ enum RpcResponse<T> {
 
 /// A minimal asynchronous Bitcoin RPC client.
 #[derive(Clone, Debug)]
-pub struct Rpc(String);
+pub struct Rpc {
+  client: Client,
+  url: String,
+}
 
 #[derive(Clone, PartialEq, Eq, Debug, Error)]
 pub enum RpcError {
   #[error("couldn't connect to node")]
   ConnectionError,
   #[error("request had an error: {0:?}")]
   RequestError(Error),
-  #[error("node sent an invalid response")]
-  InvalidResponse,
+  #[error("node replied with invalid JSON")]
+  InvalidJson(serde_json::error::Category),
+  #[error("node sent an invalid response ({0})")]
+  InvalidResponse(&'static str),
+  #[error("node was missing expected methods")]
+  MissingMethods(HashSet<&'static str>),
 }
 
 impl Rpc {
+  /// Create a new connection to a Bitcoin RPC.
+  ///
+  /// An RPC call is performed to ensure the node is reachable (and that an invalid URL wasn't
+  /// provided).
+  ///
+  /// Additionally, a set of expected methods is checked to be offered by the Bitcoin RPC. If these
+  /// methods aren't provided, an error with the missing methods is returned. This ensures all RPC
+  /// routes explicitly provided by this library are at least possible.
+  ///
+  /// Each individual RPC route may still fail at time-of-call, regardless of the arguments
+  /// provided to this library, if the RPC has an incompatible argument layout. That is not checked
+  /// at time of RPC creation.
   pub async fn new(url: String) -> Result<Rpc, RpcError> {
-    let rpc = Rpc(url);
+    let rpc = Rpc { client: Client::new(), url };
+
     // Make an RPC request to verify the node is reachable and sane
-    rpc.get_latest_block_number().await?;
+    let res: String = rpc.rpc_call("help", json!([])).await?;
+
+    // Verify all methods we expect are present
+    // If we had a more expanded RPC, due to differences in RPC versions, it wouldn't make sense to
+    // error if all methods weren't present
+    // We only provide a very minimal set of methods which have been largely consistent, hence why
+    // this is sane
+    let mut expected_methods = HashSet::from([
+      "help",
+      "getblockcount",
+      "getblockhash",
+      "getblockheader",
+      "getblock",
+      "sendrawtransaction",
+      "getrawtransaction",
+    ]);
+    for line in res.split('\n') {
+      // This doesn't check if the arguments are as expected
+      // This is due to Bitcoin supporting a large amount of optional arguments, which
+      // occassionally change, with their own mechanism of text documentation, making matching off
+      // it a quite involved task
+      // Instead, once we've confirmed the methods are present, we assume our arguments are aligned
+      // Else we'll error at time of call
+      if expected_methods.remove(line.split(' ').next().unwrap_or("")) &&
+        expected_methods.is_empty()
+      {
+        break;
+      }
+    }
+    if !expected_methods.is_empty() {
+      Err(RpcError::MissingMethods(expected_methods))?;
+    };
+
     Ok(rpc)
   }
 
@@ -52,9 +107,9 @@ impl Rpc {
     method: &str,
     params: serde_json::Value,
   ) -> Result<Response, RpcError> {
-    let client = reqwest::Client::new();
-    let res = client
-      .post(&self.0)
+    let res = self
+      .client
+      .post(&self.url)
       .json(&json!({ "jsonrpc": "2.0", "method": method, "params": params }))
       .send()
       .await
@@ -64,7 +119,7 @@ impl Rpc {
       .map_err(|_| RpcError::ConnectionError)?;
 
     let res: RpcResponse<Response> =
-      serde_json::from_str(&res).map_err(|_| RpcError::InvalidResponse)?;
+      serde_json::from_str(&res).map_err(|e| RpcError::InvalidJson(e.classify()))?;
     match res {
       RpcResponse::Ok { result } => Ok(result),
       RpcResponse::Err { error } => Err(RpcError::RequestError(error)),
@@ -107,13 +162,15 @@ impl Rpc {
   /// Get a block by its hash.
   pub async fn get_block(&self, hash: &[u8; 32]) -> Result<Block, RpcError> {
     let hex = self.rpc_call::<String>("getblock", json!([hex::encode(hash), 0])).await?;
-    let bytes: Vec<u8> = FromHex::from_hex(&hex).map_err(|_| RpcError::InvalidResponse)?;
-    let block: Block = encode::deserialize(&bytes).map_err(|_| RpcError::InvalidResponse)?;
+    let bytes: Vec<u8> = FromHex::from_hex(&hex)
+      .map_err(|_| RpcError::InvalidResponse("node didn't use hex to encode the block"))?;
+    let block: Block = encode::deserialize(&bytes)
+      .map_err(|_| RpcError::InvalidResponse("node sent an improperly serialized block"))?;
 
     let mut block_hash = *block.block_hash().as_raw_hash().as_byte_array();
     block_hash.reverse();
     if hash != &block_hash {
-      Err(RpcError::InvalidResponse)?;
+      Err(RpcError::InvalidResponse("node replied with a different block"))?;
     }
 
     Ok(block)
@@ -123,21 +180,23 @@ impl Rpc {
   pub async fn send_raw_transaction(&self, tx: &Transaction) -> Result<Txid, RpcError> {
     let txid = self.rpc_call("sendrawtransaction", json!([encode::serialize_hex(tx)])).await?;
     if txid != tx.txid() {
-      Err(RpcError::InvalidResponse)?;
+      Err(RpcError::InvalidResponse("returned TX ID inequals calculated TX ID"))?;
     }
     Ok(txid)
   }
 
   /// Get a transaction by its hash.
   pub async fn get_transaction(&self, hash: &[u8; 32]) -> Result<Transaction, RpcError> {
     let hex = self.rpc_call::<String>("getrawtransaction", json!([hex::encode(hash)])).await?;
-    let bytes: Vec<u8> = FromHex::from_hex(&hex).map_err(|_| RpcError::InvalidResponse)?;
-    let tx: Transaction = encode::deserialize(&bytes).map_err(|_| RpcError::InvalidResponse)?;
+    let bytes: Vec<u8> = FromHex::from_hex(&hex)
+      .map_err(|_| RpcError::InvalidResponse("node didn't use hex to encode the transaction"))?;
+    let tx: Transaction = encode::deserialize(&bytes)
+      .map_err(|_| RpcError::InvalidResponse("node sent an improperly serialized transaction"))?;
 
     let mut tx_hash = *tx.txid().as_raw_hash().as_byte_array();
     tx_hash.reverse();
     if hash != &tx_hash {
-      Err(RpcError::InvalidResponse)?;
+      Err(RpcError::InvalidResponse("node replied with a different transaction"))?;
     }
 
     Ok(tx)

coins/bitcoin/src/wallet/mod.rs

@@ -15,7 +15,8 @@ use frost::{
 use bitcoin::{
   consensus::encode::{Decodable, serialize},
   key::TweakedPublicKey,
-  OutPoint, ScriptBuf, TxOut, Transaction, Block, Network, Address,
+  address::Payload,
+  OutPoint, ScriptBuf, TxOut, Transaction, Block,
 };
 
 use crate::crypto::{x_only, make_even};
@@ -24,18 +25,23 @@ mod send;
 pub use send::*;
 
 /// Tweak keys to ensure they're usable with Bitcoin.
+///
+/// Taproot keys, which these keys are used as, must be even. This offsets the keys until they're
+/// even.
 pub fn tweak_keys(keys: &ThresholdKeys<Secp256k1>) -> ThresholdKeys<Secp256k1> {
   let (_, offset) = make_even(keys.group_key());
   keys.offset(Scalar::from(offset))
 }
 
-/// Return the Taproot address for a public key.
-pub fn address(network: Network, key: ProjectivePoint) -> Option<Address> {
+/// Return the Taproot address payload for a public key.
+///
+/// If the key is odd, this will return None.
+pub fn address_payload(key: ProjectivePoint) -> Option<Payload> {
   if key.to_encoded_point(true).tag() != Tag::CompressedEvenY {
     return None;
   }
 
-  Some(Address::p2tr_tweaked(TweakedPublicKey::dangerous_assume_tweaked(x_only(&key)), network))
+  Some(Payload::p2tr_tweaked(TweakedPublicKey::dangerous_assume_tweaked(x_only(&key))))
 }
 
 /// A spendable output.
@@ -104,8 +110,7 @@ impl Scanner {
   /// Returns None if this key can't be scanned for.
   pub fn new(key: ProjectivePoint) -> Option<Scanner> {
     let mut scripts = HashMap::new();
-    // Uses Network::Bitcoin since network is irrelevant here
-    scripts.insert(address(Network::Bitcoin, key)?.script_pubkey(), Scalar::ZERO);
+    scripts.insert(address_payload(key)?.script_pubkey(), Scalar::ZERO);
     Some(Scanner { key, scripts })
   }
 
@@ -114,9 +119,15 @@ impl Scanner {
   /// Due to Bitcoin's requirement that points are even, not every offset may be used.
   /// If an offset isn't usable, it will be incremented until it is. If this offset is already
   /// present, None is returned. Else, Some(offset) will be, with the used offset.
+  ///
+  /// This means offsets are surjective, not bijective, and the order offsets are registered in
+  /// may determine the validity of future offsets.
   pub fn register_offset(&mut self, mut offset: Scalar) -> Option<Scalar> {
+    // This loop will terminate as soon as an even point is found, with any point having a ~50%
+    // chance of being even
+    // That means this should terminate within a very small amount of iterations
     loop {
-      match address(Network::Bitcoin, self.key + (ProjectivePoint::GENERATOR * offset)) {
+      match address_payload(self.key + (ProjectivePoint::GENERATOR * offset)) {
         Some(address) => {
           let script = address.script_pubkey();
           if self.scripts.contains_key(&script) {
@@ -134,11 +145,16 @@ impl Scanner {
   pub fn scan_transaction(&self, tx: &Transaction) -> Vec<ReceivedOutput> {
     let mut res = vec![];
     for (vout, output) in tx.output.iter().enumerate() {
+      // If the vout index exceeds 2**32, stop scanning outputs
+      let Ok(vout) = u32::try_from(vout) else {
+        break
+      };
+
       if let Some(offset) = self.scripts.get(&output.script_pubkey) {
         res.push(ReceivedOutput {
           offset: *offset,
           output: output.clone(),
-          outpoint: OutPoint::new(tx.txid(), u32::try_from(vout).unwrap()),
+          outpoint: OutPoint::new(tx.txid(), vout),
         });
       }
     }

coins/bitcoin/src/wallet/send.rs

@@ -16,21 +16,35 @@ use bitcoin::{
   sighash::{TapSighashType, SighashCache, Prevouts},
   absolute::LockTime,
   script::{PushBytesBuf, ScriptBuf},
-  OutPoint, Sequence, Witness, TxIn, TxOut, Transaction, Network, Address,
+  OutPoint, Sequence, Witness, TxIn, TxOut, Transaction, Address,
 };
 
 use crate::{
   crypto::Schnorr,
-  wallet::{address, ReceivedOutput},
+  wallet::{ReceivedOutput, address_payload},
 };
 
 #[rustfmt::skip]
 // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.h#L27
 const MAX_STANDARD_TX_WEIGHT: u64 = 400_000;
 
 #[rustfmt::skip]
-//https://github.com/bitcoin/bitcoin/blob/a245429d680eb95cf4c0c78e58e63e3f0f5d979a/src/test/transaction_tests.cpp#L815-L816
-const DUST: u64 = 674;
+// https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.cpp#L26-L63
+// As the above notes, a lower amount may not be considered dust if contained in a SegWit output
+// This doesn't bother with delineation due to how marginal these values are, and because it isn't
+// worth the complexity to implement differentation
+const DUST: u64 = 546;
+
+#[rustfmt::skip]
+// The constant is from:
+// https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/policy.h#L56-L57
+// It's used here:
+// https://github.com/bitcoin/bitcoin/blob/296735f7638749906243c9e203df7bd024493806/src/net_processing.cpp#L5386-L5390
+// Peers won't relay TXs below the filter's fee rate, yet they calculate the fee not against weight yet vsize
+// https://github.com/bitcoin/bitcoin/blob/296735f7638749906243c9e203df7bd024493806/src/net_processing.cpp#L5721-L5732
+// And then the fee itself is fee per thousand units, not fee per unit
+// https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/src/policy/feerate.cpp#L23-L37
+const MIN_FEE_PER_KILO_VSIZE: u64 = 1000;
 
 #[derive(Clone, PartialEq, Eq, Debug, Error)]
 pub enum TransactionError {
@@ -42,6 +56,8 @@ pub enum TransactionError {
   DustPayment,
   #[error("too much data was specified")]
   TooMuchData,
+  #[error("fee was too low to pass the default minimum fee rate")]
+  TooLowFee,
   #[error("not enough funds for these payments")]
   NotEnoughFunds,
   #[error("transaction was too large")]
@@ -163,6 +179,26 @@ impl SignableTransaction {
 
     let mut weight = Self::calculate_weight(tx_ins.len(), payments, None);
     let mut needed_fee = fee_per_weight * weight;
+
+    // "Virtual transaction size" is weight ceildiv 4 per
+    // https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki
+
+    // https://github.com/bitcoin/bitcoin/blob/306ccd4927a2efe325c8d84be1bdb79edeb29b04/
+    //  src/policy/policy.cpp#L295-L298
+    // implements this as expected
+
+    // Technically, it takes whatever's greater, the weight or the amount of signature operatons
+    // multiplied by DEFAULT_BYTES_PER_SIGOP (20)
+    // We only use 1 signature per input, and our inputs have a weight exceeding 20
+    // Accordingly, our inputs' weight will always be greater than the cost of the signature ops
+    let vsize = (weight + 3) / 4;
+    // Technically, if there isn't change, this TX may still pay enough of a fee to pass the
+    // minimum fee. Such edge cases aren't worth programming when they go against intent, as the
+    // specified fee rate is too low to be valid
+    if needed_fee < ((MIN_FEE_PER_KILO_VSIZE * vsize) / 1000) {
+      Err(TransactionError::TooLowFee)?;
+    }
+
     if input_sat < (payment_sat + needed_fee) {
       Err(TransactionError::NotEnoughFunds)?;
     }
@@ -221,12 +257,12 @@ impl SignableTransaction {
     let mut sigs = vec![];
     for i in 0 .. tx.input.len() {
       let mut transcript = transcript.clone();
+      // This unwrap is safe since any transaction with this many inputs violates the maximum
+      // size allowed under standards, which this lib will error on creation of
       transcript.append_message(b"signing_input", u32::try_from(i).unwrap().to_le_bytes());
 
       let offset = keys.clone().offset(self.offsets[i]);
-      if address(Network::Bitcoin, offset.group_key())?.script_pubkey() !=
-        self.prevouts[i].script_pubkey
-      {
+      if address_payload(offset.group_key())?.script_pubkey() != self.prevouts[i].script_pubkey {
         None?;
       }
 
@@ -243,7 +279,7 @@ impl SignableTransaction {
 /// A FROST signing machine to produce a Bitcoin transaction.
 ///
 /// This does not support caching its preprocess. When sign is called, the message must be empty.
-/// This will panic if it isn't.
+/// This will panic if either `cache` is called or the message isn't empty.
 pub struct TransactionMachine {
   tx: SignableTransaction,
   sigs: Vec<AlgorithmMachine<Secp256k1, Schnorr<RecommendedTranscript>>>,
@@ -339,7 +375,9 @@ impl SignMachine<Transaction> for TransactionSignMachine {
           commitments[i].clone(),
           cache
             .taproot_key_spend_signature_hash(i, &prevouts, TapSighashType::Default)
-            .unwrap()
+            // This should never happen since the inputs align with the TX the cache was
+            // constructed with, and because i is always < prevouts.len()
+            .expect("taproot_key_spend_signature_hash failed to return a hash")
             .as_ref(),
         )?;
         shares.push(share);