From c878d38c606e9add77ac1d1a058060accd1a51c8 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 8 Jul 2023 21:48:18 -0400 Subject: [PATCH 01/12] 3.1 --- coins/bitcoin/src/rpc.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/coins/bitcoin/src/rpc.rs b/coins/bitcoin/src/rpc.rs index ba2c8a0f..c03861f7 100644 --- a/coins/bitcoin/src/rpc.rs +++ b/coins/bitcoin/src/rpc.rs @@ -5,6 +5,8 @@ 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 +28,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 { @@ -40,7 +45,7 @@ pub enum RpcError { impl Rpc { 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?; Ok(rpc) @@ -52,9 +57,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 From 7fa5d291b8c60da1c26525b45f02610180bc90c8 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 9 Jul 2023 15:49:35 -0400 Subject: [PATCH 02/12] Implement a more robust validity check on connection creation --- coins/bitcoin/src/crypto.rs | 2 +- coins/bitcoin/src/rpc.rs | 50 ++++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/coins/bitcoin/src/crypto.rs b/coins/bitcoin/src/crypto.rs index 54052a91..eb30cebe 100644 --- a/coins/bitcoin/src/crypto.rs +++ b/coins/bitcoin/src/crypto.rs @@ -52,7 +52,7 @@ pub fn make_even(mut key: ProjectivePoint) -> (ProjectivePoint, u64) { /// /// If passed an odd nonce, it will have the generator added until it is even. #[derive(Clone, Copy, Debug)] -pub struct Hram {} +pub struct Hram; lazy_static! { static ref TAG_HASH: [u8; 32] = Sha256::digest(b"BIP0340/challenge").into(); diff --git a/coins/bitcoin/src/rpc.rs b/coins/bitcoin/src/rpc.rs index c03861f7..098b0231 100644 --- a/coins/bitcoin/src/rpc.rs +++ b/coins/bitcoin/src/rpc.rs @@ -1,4 +1,5 @@ use core::fmt::Debug; +use std::collections::HashSet; use thiserror::Error; @@ -41,13 +42,60 @@ pub enum RpcError { RequestError(Error), #[error("node sent an invalid response")] InvalidResponse, + #[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 { 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) } From 3480fc5e166d5d4c2b7cfcbfe086534997857770 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 13:33:08 -0400 Subject: [PATCH 03/12] 3.4 --- coins/bitcoin/src/rpc.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/coins/bitcoin/src/rpc.rs b/coins/bitcoin/src/rpc.rs index 098b0231..7d415f2a 100644 --- a/coins/bitcoin/src/rpc.rs +++ b/coins/bitcoin/src/rpc.rs @@ -40,8 +40,10 @@ 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>), } @@ -117,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)), @@ -160,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) @@ -176,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) } @@ -184,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) From 3d00d405a31af05f0ce55921fc61945bf9f5cb8a Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 13:35:47 -0400 Subject: [PATCH 04/12] 3.5 (handled with 3.1) From d75115ce1396949e17c87be6ffab886c2ee5a975 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 14:02:55 -0400 Subject: [PATCH 05/12] 3.7 Replace unwraps with expects Doesn't replace unwraps on integer conversions. --- coins/bitcoin/src/crypto.rs | 7 ++++--- coins/bitcoin/src/wallet/send.rs | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/coins/bitcoin/src/crypto.rs b/coins/bitcoin/src/crypto.rs index eb30cebe..bcb73936 100644 --- a/coins/bitcoin/src/crypto.rs +++ b/coins/bitcoin/src/crypto.rs @@ -32,9 +32,9 @@ 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 @@ -145,7 +145,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/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 33c196c6..a176c2fc 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -339,7 +339,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); From fa1b569b78890cc5edb22dce91f08acd64cef854 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 14:34:32 -0400 Subject: [PATCH 06/12] 3.8 Document termination of unbounded loop --- coins/bitcoin/src/wallet/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index 7f295d7f..46ffff96 100644 --- a/coins/bitcoin/src/wallet/mod.rs +++ b/coins/bitcoin/src/wallet/mod.rs @@ -115,6 +115,9 @@ impl Scanner { /// 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. 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)) { Some(address) => { From 677b9b681f5e2819fe34599beb0947adc22afabd Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 14:43:46 -0400 Subject: [PATCH 07/12] 3.9/3.10. 3.9: Remove cast which fails on a several GB malicious TX 3.10 has its impossibility documented. A malicious RPC cananot effect this code. --- coins/bitcoin/src/wallet/mod.rs | 7 ++++++- coins/bitcoin/src/wallet/send.rs | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index 46ffff96..3997730e 100644 --- a/coins/bitcoin/src/wallet/mod.rs +++ b/coins/bitcoin/src/wallet/mod.rs @@ -137,11 +137,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 a176c2fc..7e245407 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -221,6 +221,8 @@ 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]); From df67b7d94c56ef8f75994e02885bb5c78314ef13 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 10 Jul 2023 15:02:34 -0400 Subject: [PATCH 08/12] 3.13 Better document the offset mapping --- coins/bitcoin/src/wallet/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index 3997730e..ea91d2ae 100644 --- a/coins/bitcoin/src/wallet/mod.rs +++ b/coins/bitcoin/src/wallet/mod.rs @@ -114,6 +114,9 @@ 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 From 6f9d02fdf83e426b0a4a0df9c851de5911067a5e Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Wed, 19 Jul 2023 23:51:21 -0400 Subject: [PATCH 09/12] 3.11 Better document API expectations --- coins/bitcoin/src/crypto.rs | 7 +++++-- coins/bitcoin/src/wallet/mod.rs | 5 +++++ coins/bitcoin/src/wallet/send.rs | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/coins/bitcoin/src/crypto.rs b/coins/bitcoin/src/crypto.rs index bcb73936..741a0289 100644 --- a/coins/bitcoin/src/crypto.rs +++ b/coins/bitcoin/src/crypto.rs @@ -37,8 +37,9 @@ pub fn x_only(key: &ProjectivePoint) -> XOnlyPublicKey { 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,6 +52,8 @@ 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; diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index ea91d2ae..d3575df2 100644 --- a/coins/bitcoin/src/wallet/mod.rs +++ b/coins/bitcoin/src/wallet/mod.rs @@ -24,12 +24,17 @@ 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. +/// +/// If the key is odd, this will return None. pub fn address(network: Network, key: ProjectivePoint) -> Option
{ if key.to_encoded_point(true).tag() != Tag::CompressedEvenY { return None; diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 7e245407..3d18c47c 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -245,7 +245,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>>, From f66fe3c1cb56f90f9d1f1ba8c4d2ff4dc6fa9768 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 20 Jul 2023 00:00:20 -0400 Subject: [PATCH 10/12] 3.10 Remove use of Network::Bitcoin All uses were safe due to addresses being converted to script_pubkeys which don't embed their network. The only risk of there being an issue is if a future address spec did embed the net ID into the script_pubkey and that was moved to. This resolves the audit note and does offer that tightening. --- coins/bitcoin/src/wallet/mod.rs | 14 +++++++------- coins/bitcoin/src/wallet/send.rs | 8 +++----- coins/bitcoin/tests/wallet.rs | 21 ++++++++++++++------- processor/src/coins/bitcoin.rs | 7 ++++--- 4 files changed, 28 insertions(+), 22 deletions(-) diff --git a/coins/bitcoin/src/wallet/mod.rs b/coins/bitcoin/src/wallet/mod.rs index d3575df2..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}; @@ -32,15 +33,15 @@ pub fn tweak_keys(keys: &ThresholdKeys) -> ThresholdKeys { keys.offset(Scalar::from(offset)) } -/// Return the Taproot address for a public key. +/// Return the Taproot address payload for a public key. /// /// If the key is odd, this will return None. -pub fn address(network: Network, key: ProjectivePoint) -> Option
{ +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. @@ -109,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 }) } @@ -127,7 +127,7 @@ impl Scanner { // 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) { diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index 3d18c47c..e4669f97 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] @@ -226,9 +226,7 @@ impl SignableTransaction { 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?; } diff --git a/coins/bitcoin/tests/wallet.rs b/coins/bitcoin/tests/wallet.rs index ab614573..be135d4e 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()); @@ -250,13 +256,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 +334,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/coins/bitcoin.rs b/processor/src/coins/bitcoin.rs index 1de34bca..27b3869c 100644 --- a/processor/src/coins/bitcoin.rs +++ b/processor/src/coins/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, }, 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 Coin for Bitcoin { } fn address(key: ProjectivePoint) -> Address { - Address(address(Network::Bitcoin, key).unwrap()) + Address(BAddress::::new(Network::Bitcoin, address_payload(key).unwrap())) } fn branch_address(key: ProjectivePoint) -> Self::Address { From 1eb3b364f46e003b92baed89dcdef3e4878c79aa Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 20 Jul 2023 00:29:31 -0400 Subject: [PATCH 11/12] Correct dust constant --- coins/bitcoin/src/wallet/send.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index e4669f97..a67d3aa6 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -29,8 +29,11 @@ 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; #[derive(Clone, PartialEq, Eq, Debug, Error)] pub enum TransactionError { From 5121ca75199dff7bd34230880a1fdd793012068c Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 20 Jul 2023 01:20:28 -0400 Subject: [PATCH 12/12] Handle the minimum relay fee --- coins/bitcoin/src/wallet/send.rs | 33 ++++++++++++++++++++++++++++++++ coins/bitcoin/tests/wallet.rs | 7 ++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/coins/bitcoin/src/wallet/send.rs b/coins/bitcoin/src/wallet/send.rs index a67d3aa6..43fa54bf 100644 --- a/coins/bitcoin/src/wallet/send.rs +++ b/coins/bitcoin/src/wallet/send.rs @@ -35,6 +35,17 @@ const MAX_STANDARD_TX_WEIGHT: u64 = 400_000; // 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 { #[error("no inputs were specified")] @@ -45,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")] @@ -166,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)?; } diff --git a/coins/bitcoin/tests/wallet.rs b/coins/bitcoin/tests/wallet.rs index be135d4e..ccaa7e0a 100644 --- a/coins/bitcoin/tests/wallet.rs +++ b/coins/bitcoin/tests/wallet.rs @@ -228,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), ); }