From 6362b8cb26f25d4db679e0f56630d24c536f3ebe Mon Sep 17 00:00:00 2001 From: Daniel Jordon Date: Tue, 26 Nov 2024 13:41:11 -0500 Subject: [PATCH] fix: deposit max-fees must not exceed the deposit amount (#933) * Take the minimum of the max fee and the amount for deposits when filtering deposits * Use the actual max solo deposit size * Fix up withdrawal minimums too --- signer/src/bitcoin/utxo.rs | 91 ++++++++++++++++++++++++++++++-------- 1 file changed, 72 insertions(+), 19 deletions(-) diff --git a/signer/src/bitcoin/utxo.rs b/signer/src/bitcoin/utxo.rs index 58467774a..b6dc10ecc 100644 --- a/signer/src/bitcoin/utxo.rs +++ b/signer/src/bitcoin/utxo.rs @@ -57,17 +57,18 @@ const DEFAULT_INCREMENTAL_RELAY_FEE_RATE: f64 = bitcoin::policy::DEFAULT_INCREMENTAL_RELAY_FEE as f64 / 1000.0; /// This constant represents the virtual size (in vBytes) of a BTC -/// transaction that includes two inputs and one output. The inputs -/// consist of the signers' input UTXO and a UTXO for a deposit request. -/// The output is the signers' new UTXO. -const SOLO_DEPOSIT_TX_VSIZE: f64 = 234.0; +/// transaction that includes two inputs and one output. The inputs consist +/// of the signers' input UTXO and a UTXO for a deposit request. The output +/// is the signers' new UTXO. The deposit request is such that the sweep +/// transaction has the largest size of solo deposit sweep transactions. +const SOLO_DEPOSIT_TX_VSIZE: f64 = 267.0; /// This constant represents the virtual size (in vBytes) of a BTC -/// transaction with only one input and two outputs. The input is the -/// signers' input UTXO. The outputs include the withdrawal UTXO for a -/// withdrawal request and the signers' new UTXO. This size assumes -/// the script in the withdrawal UTXO is empty. -const BASE_WITHDRAWAL_TX_VSIZE: f64 = 172.0; +/// transaction servicing only one withdrawal request, except the +/// withdrawal output is not in the transaction. This way the sweep +/// transaction's OP_RETURN output is the right size and we can handle the +/// variability of output sizes. +const BASE_WITHDRAWAL_TX_VSIZE: f64 = 163.0; /// It appears that bitcoin-core tracks fee rates in sats per kilo-vbyte /// (or BTC per kilo-vbyte). Since we work in sats per vbyte, this constant @@ -155,19 +156,22 @@ impl SbtcRequests { .filter(|req| { // This is the size for a BTC transaction servicing // a single withdrawal. - let tx_vsize = BASE_WITHDRAWAL_TX_VSIZE + req.script_pubkey.len() as f64; + let withdrawal_output = req.as_tx_output(); + let tx_vsize = BASE_WITHDRAWAL_TX_VSIZE + withdrawal_output.size() as f64; req.max_fee >= self.compute_minimum_fee(tx_vsize) }) .map(RequestRef::Withdrawal); - // Now we filter deposit requests where the user's max fee could - // be less than the fee we may charge. This is simpler because - // deposit UTXOs have a known fixed size. + // Now we filter deposit requests where the user's max fee could be + // less than the fee we may charge. This is simpler because deposit + // UTXOs have a known fixed size. Also, the "actual" max fee must + // always be less than or equal to the deposit amount, so take the + // minimum of the max fee and the amount. let minimum_deposit_fee = self.compute_minimum_fee(SOLO_DEPOSIT_TX_VSIZE); let deposits = self .deposits .iter() - .filter(|req| req.max_fee >= minimum_deposit_fee) + .filter(|req| req.max_fee.min(req.amount) >= minimum_deposit_fee) .map(RequestRef::Deposit); // Create a list of requests where each request can be approved on its own. @@ -704,7 +708,7 @@ impl<'a> UnsignedTransaction<'a> { pub fn new(requests: Requests<'a>, state: &SignerBtcState) -> Result { // Construct a transaction. This transaction's inputs have witness // data with dummy signatures so that our virtual size estimates - // are accurate. Afterwards we remove the witness data. + // are accurate. Afterward we remove the witness data. let mut unsigned = Self::new_stub(requests, state)?; // Now we can reset the witness data, since this is an unsigned // transaction. @@ -724,8 +728,8 @@ impl<'a> UnsignedTransaction<'a> { /// 3. The signer output UTXO is the first output. The second output /// is the OP_RETURN data output. /// 4. Each input has a fake signature in the witness data. - /// 5. With the exception of the fake signatures from (4), all - /// witness data is correctly set. + /// 5. All witness data is correctly set, except for the fake + /// signatures from (4). pub fn new_stub(requests: Requests<'a>, state: &SignerBtcState) -> Result { // Construct a transaction base. This transaction's inputs have // witness data with dummy signatures so that our virtual size @@ -1414,10 +1418,13 @@ mod tests { let mut signer_bitmap: BitArray<[u8; 16]> = BitArray::ZERO; signer_bitmap[..votes_against].fill(true); + let contract_name = std::iter::repeat('a').take(128).collect::(); + let principal_str = format!("{}.{contract_name}", StacksAddress::burn_address(false)); + let deposit_inputs = DepositScriptInputs { signers_public_key, max_fee: 10000, - recipient: PrincipalData::from(StacksAddress::burn_address(false)), + recipient: PrincipalData::parse(&principal_str).unwrap(), }; DepositRequest { @@ -1570,7 +1577,7 @@ mod tests { // We need to zero out the withdrawal script since this value // changes depending on the user. - unsigned.tx.output[2].script_pubkey = ScriptBuf::new(); + unsigned.tx.output.pop(); testing::set_witness_data(&mut unsigned, keypair); println!("Solo withdrawal vsize: {}", unsigned.tx.vsize()); @@ -1902,6 +1909,52 @@ mod tests { } } + /// Deposit requests add to the signers' UTXO. + #[test] + fn deposits_with_low_amount_and_high_max_fee() { + // The bad deposit + let deposit_amount = 100; + let max_fee = 123456; + + let public_key = XOnlyPublicKey::from_str(X_ONLY_PUBLIC_KEY1).unwrap(); + let requests = SbtcRequests { + deposits: vec![ + create_deposit(deposit_amount, max_fee, 0), + create_deposit(345678, 345678, 0), + ], + withdrawals: Vec::new(), + signer_state: SignerBtcState { + utxo: SignerUtxo { + outpoint: OutPoint::null(), + amount: 55, + public_key, + }, + fee_rate: 1.0, + public_key, + last_fees: None, + magic_bytes: [0; 2], + }, + num_signers: 10, + accept_threshold: 0, + }; + + // This should all be in one transaction since there are no votes + // against any of the requests. + let mut transactions = requests.construct_transactions().unwrap(); + assert_eq!(transactions.len(), 1); + + // There should be two outputs, one for the signer and another for + // the one of the deposits. + let unsigned_tx = transactions.pop().unwrap(); + assert_eq!(unsigned_tx.tx.output.len(), 2); + + // The input amounts should be the sum of the signer amount and the + // one deposit amount. + let signer_amount = requests.signer_state.utxo.amount; + let input_amount = unsigned_tx.input_amounts(); + assert_eq!(input_amount, signer_amount + 345678) + } + /// Deposit requests add to the signers' UTXO. #[test] fn deposits_increase_signers_utxo_amount() {