diff --git a/coins/bitcoin/src/crypto.rs b/coins/bitcoin/src/crypto.rs index 54052a91..741a0289 100644 --- a/coins/bitcoin/src/crypto.rs +++ b/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 Algorithm for Schnorr { // 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") }) } diff --git a/coins/bitcoin/src/rpc.rs b/coins/bitcoin/src/rpc.rs index ba2c8a0f..7d415f2a 100644 --- a/coins/bitcoin/src/rpc.rs +++ b/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,7 +29,10 @@ enum RpcResponse { /// 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 { @@ -34,15 +40,64 @@ pub enum RpcError { 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 { - 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 { - 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 = - 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 { let hex = self.rpc_call::("getblock", json!([hex::encode(hash), 0])).await?; - let bytes: Vec = FromHex::from_hex(&hex).map_err(|_| RpcError::InvalidResponse)?; - let block: Block = encode::deserialize(&bytes).map_err(|_| RpcError::InvalidResponse)?; + let bytes: Vec = 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,7 +180,7 @@ impl Rpc { pub async fn send_raw_transaction(&self, tx: &Transaction) -> Result { 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) } @@ -131,13 +188,15 @@ impl Rpc { /// Get a transaction by its hash. pub async fn get_transaction(&self, hash: &[u8; 32]) -> Result { let hex = self.rpc_call::("getrawtransaction", json!([hex::encode(hash)])).await?; - let bytes: Vec = FromHex::from_hex(&hex).map_err(|_| RpcError::InvalidResponse)?; - let tx: Transaction = encode::deserialize(&bytes).map_err(|_| RpcError::InvalidResponse)?; + let bytes: Vec = 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) diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index 7f295d7f..bfe5494c 100644 --- a/coins/bitcoin/src/wallet/mod.rs +++ b/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) -> ThresholdKeys { 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
{ +/// 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 { 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 { 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 { + // 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 { 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), }); } } diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 33c196c6..43fa54bf 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -16,12 +16,12 @@ 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] @@ -29,8 +29,22 @@ use crate::{ 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>>, @@ -339,7 +375,9 @@ impl SignMachine 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); diff --git a/coins/bitcoin/tests/wallet.rs b/coins/bitcoin/tests/wallet.rs index ab614573..ccaa7e0a 100644 --- a/coins/bitcoin/tests/wallet.rs +++ b/coins/bitcoin/tests/wallet.rs @@ -22,9 +22,12 @@ use bitcoin_serai::{ hashes::Hash as HashTrait, blockdata::opcodes::all::OP_RETURN, script::{PushBytesBuf, Instruction, Instructions, Script}, + address::NetworkChecked, OutPoint, TxOut, Transaction, Network, Address, }, - wallet::{tweak_keys, address, ReceivedOutput, Scanner, TransactionError, SignableTransaction}, + wallet::{ + tweak_keys, address_payload, ReceivedOutput, Scanner, TransactionError, SignableTransaction, + }, rpc::Rpc, }; @@ -43,7 +46,10 @@ async fn send_and_get_output(rpc: &Rpc, scanner: &Scanner, key: ProjectivePoint) rpc .rpc_call::>( "generatetoaddress", - serde_json::json!([1, address(Network::Regtest, key).unwrap()]), + serde_json::json!([ + 1, + Address::::new(Network::Regtest, address_payload(key).unwrap()) + ]), ) .await .unwrap(); @@ -187,7 +193,7 @@ async_sequential! { assert_eq!(output.offset(), Scalar::ZERO); let inputs = vec![output]; - let addr = || address(Network::Regtest, key).unwrap(); + let addr = || Address::::new(Network::Regtest, address_payload(key).unwrap()); let payments = vec![(addr(), 1000)]; assert!(SignableTransaction::new(inputs.clone(), &payments, None, None, FEE).is_ok()); @@ -222,13 +228,18 @@ async_sequential! { Err(TransactionError::TooMuchData), ); + assert_eq!( + SignableTransaction::new(inputs.clone(), &[], Some(addr()), None, 0), + Err(TransactionError::TooLowFee), + ); + assert_eq!( SignableTransaction::new(inputs.clone(), &[(addr(), inputs[0].value() * 2)], None, None, FEE), Err(TransactionError::NotEnoughFunds), ); assert_eq!( - SignableTransaction::new(inputs, &vec![(addr(), 1000); 10000], None, None, 0), + SignableTransaction::new(inputs, &vec![(addr(), 1000); 10000], None, None, FEE), Err(TransactionError::TooLargeTransaction), ); } @@ -250,13 +261,14 @@ async_sequential! { // Declare payments, change, fee let payments = [ - (address(Network::Regtest, key).unwrap(), 1005), - (address(Network::Regtest, offset_key).unwrap(), 1007) + (Address::::new(Network::Regtest, address_payload(key).unwrap()), 1005), + (Address::::new(Network::Regtest, address_payload(offset_key).unwrap()), 1007) ]; let change_offset = scanner.register_offset(Scalar::random(&mut OsRng)).unwrap(); let change_key = key + (ProjectivePoint::GENERATOR * change_offset); - let change_addr = address(Network::Regtest, change_key).unwrap(); + let change_addr = + Address::::new(Network::Regtest, address_payload(change_key).unwrap()); // Create and sign the TX let tx = SignableTransaction::new( @@ -327,7 +339,7 @@ async_sequential! { SignableTransaction::new( vec![output], &[], - address(Network::Regtest, key), + Some(Address::::new(Network::Regtest, address_payload(key).unwrap())), Some(data.clone()), FEE ).unwrap() diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index cfd0338c..1b208173 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -17,10 +17,11 @@ use bitcoin_serai::{ hashes::Hash as HashTrait, consensus::{Encodable, Decodable}, script::Instruction, + address::{NetworkChecked, Address as BAddress}, OutPoint, Transaction, Block, Network as BitcoinNetwork, }, wallet::{ - tweak_keys, address, ReceivedOutput, Scanner, TransactionError, + tweak_keys, address_payload, ReceivedOutput, Scanner, TransactionError, SignableTransaction as BSignableTransaction, TransactionMachine, }, rpc::{RpcError, Rpc}, @@ -33,7 +34,7 @@ use bitcoin_serai::bitcoin::{ sighash::{EcdsaSighashType, SighashCache}, script::{PushBytesBuf, Builder}, absolute::LockTime, - Sequence, Script, Witness, TxIn, TxOut, Address as BAddress, + Sequence, Script, Witness, TxIn, TxOut, }; use serai_client::{ @@ -326,7 +327,7 @@ impl Network for Bitcoin { } fn address(key: ProjectivePoint) -> Address { - Address(address(BitcoinNetwork::Bitcoin, key).unwrap()) + Address(BAddress::::new(BitcoinNetwork::Bitcoin, address_payload(key).unwrap())) } fn branch_address(key: ProjectivePoint) -> Self::Address { @@ -482,7 +483,12 @@ impl Network for Bitcoin { } // No outputs left and the change isn't worth enough Err(TransactionError::NoOutputs) => None, + // amortize_fee removes payments which fall below the dust threshold + Err(TransactionError::DustPayment) => panic!("dust payment despite removing dust"), Err(TransactionError::TooMuchData) => panic!("too much data despite not specifying data"), + Err(TransactionError::TooLowFee) => { + panic!("created a transaction whose fee is below the minimum") + } Err(TransactionError::NotEnoughFunds) => { if tx_fee.is_none() { // Mot even enough funds to pay the fee @@ -491,8 +497,6 @@ impl Network for Bitcoin { panic!("not enough funds for bitcoin TX despite amortizing the fee") } } - // amortize_fee removes payments which fall below the dust threshold - Err(TransactionError::DustPayment) => panic!("dust payment despite removing dust"), Err(TransactionError::TooLargeTransaction) => { panic!("created a too large transaction despite limiting inputs/outputs") }