From 349c0da3a2d3c52f8f0114aa2d165f39b0c77d2c Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:21:57 -0400 Subject: [PATCH] fix(key-wallet): reject multi-OP_RETURN drains and memo-on-asset-lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found auditing merged #928 (OP_RETURN memo outputs). 1. A multi-OP_RETURN drain built an UNRELAYABLE transaction that stranded the entire balance. #928's drain guard requires exactly one VALUE carrier but put no ceiling on data outputs, so `dest + 2 memos` built cleanly. Dash Core's IsStandardTx tallies data carriers into nDataOut and rejects nDataOut > 1 as `multi-op-return`, so no node ever accepts it. A drain spends everything and reserves its inputs, and the sweep path only reclaims inputs once a competing transaction confirms — for a transaction that was never relayed no competitor can exist, so the balance sits behind something that can neither confirm nor be replaced until an explicit abandon. Enforce nDataOut <= 1 with a typed BuilderError::TooManyOpReturnOutputs naming the standardness rule. The guard counts the FINAL output set, after the asset-lock burn substitution and the change push, so it measures the shape that would go on the wire exactly as a validating node counts it. That also makes it general rather than drain-only: an ordinary two-memo send is just as unrelayable and is now refused too. 2. An asset-lock build silently DISCARDED an OP_RETURN memo. The build replaces tx_outputs with vec![burn], dropping self.outputs, and #928 exempted asset locks from both drain guards — so `.add_op_return(..)` on an asset lock succeeded with the memo gone and the caller believing their data was on-chain. Memo-on-asset-lock is unsupported (the one standard data slot is taken by the credit-mirroring burn); the silent drop was the defect. Refuse it with a typed BuilderError::OpReturnOnAssetLock, before coin selection so nothing is selected or reserved. Both new variants are additive and every consumer renders BuilderError via Display, so no match arms change. Tests: two-memo drain and two-memo send both yield the typed error with nothing built and no reservation left behind; a single carrier still builds; asset lock refuses a memo in both drain and non-drain modes. Also pins the DEFAULT output order. All four #928 tests called preserve_output_order(), leaving the default shape untested. BIP-69 sorts by value ascending, so a zero-value memo lands at vout 0 and the destination follows. That is left as-is and documented: output position carries no consensus or relay meaning, and no consumer reads the memo positionally — add_op_return has no call site outside this module and the FFI does not expose it. Callers needing a fixed layout opt into preserve_output_order(), as the MAYA-style test does. Co-Authored-By: Claude Opus 4.8 --- .../transaction_builder.rs | 241 ++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 1ca3132f9..83a8f71f2 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -31,6 +31,11 @@ const MAX_STANDARD_TX_INPUTS: usize = 500; /// Public so callers can reject an over-long payload before handing over a builder /// `add_op_return` would consume. pub const MAX_STANDARD_OP_RETURN_BYTES: usize = 80; +/// Relay-policy limit on the NUMBER of OP_RETURN outputs in one transaction. Dash Core's +/// `IsStandardTx` tallies data carriers into `nDataOut` and rejects `nDataOut > 1` with the +/// `multi-op-return` reason, so a second data output makes the transaction unrelayable: no +/// node accepts it and nothing ever mines it. +pub const MAX_STANDARD_OP_RETURN_OUTPUTS: usize = 1; /// Calculate varint size for a given number fn varint_size(n: usize) -> usize { @@ -446,6 +451,16 @@ impl TransactionBuilder { if p.credit_outputs.is_empty() { return Err(BuilderError::NoOutputs); } + // An asset-lock build replaces the caller's outputs wholesale with the single burn + // that mirrors the payload credits (see the `tx_outputs` match below), and its data + // slot is spoken for by that burn. A memo handed to `add_op_return` would therefore + // be dropped without a word — the caller believes their data is on-chain when the + // broadcast transaction never carried it. Refuse the combination instead: an + // unsupported request must fail loudly, not silently succeed as something else. + // Checked before coin selection so nothing is selected or reserved. + if self.outputs.iter().any(|out| out.script_pubkey.is_op_return()) { + return Err(BuilderError::OpReturnOnAssetLock); + } } else if self.outputs.is_empty() && self.special_payload.is_none() { return Err(BuilderError::NoOutputs); } @@ -620,6 +635,29 @@ impl TransactionBuilder { }); } + // Standardness: Dash Core's `IsStandardTx` tallies every OP_RETURN output into + // `nDataOut` and rejects the transaction as `multi-op-return` once that count passes + // one. Such a transaction is not merely suboptimal, it is unrelayable — every node + // drops it on receipt, so it never reaches a mempool and can never be mined. + // + // A drain makes that outcome expensive rather than merely annoying. It spends the + // ENTIRE balance and reserves its inputs, so the wallet is left pointing at a + // transaction that cannot confirm and cannot be replaced: the sweep path only reclaims + // inputs once a competing transaction confirms, and for something no node ever relayed + // no competitor can exist. Recovery takes an explicit abandon. Rejecting the build is + // the only outcome that leaves the balance spendable. + // + // Counted on the FINAL output set — after the asset-lock burn substitution and the + // change push — so this measures the shape that would actually go on the wire, exactly + // as a validating node counts it, and cannot drift if a later path adds a data output. + let data_outputs = tx_outputs.iter().filter(|out| out.script_pubkey.is_op_return()).count(); + if data_outputs > MAX_STANDARD_OP_RETURN_OUTPUTS { + return Err(BuilderError::TooManyOpReturnOutputs { + count: data_outputs, + max: MAX_STANDARD_OP_RETURN_OUTPUTS, + }); + } + if !self.preserve_output_order && !matches!(self.special_payload, Some(TransactionPayload::AssetLockPayloadType(_))) { @@ -929,6 +967,16 @@ pub enum BuilderError { len: usize, max: usize, }, + /// More OP_RETURN outputs than relay policy accepts. Dash Core's `IsStandardTx` rejects + /// `nDataOut > 1` as `multi-op-return`, so the transaction would never relay. + TooManyOpReturnOutputs { + count: usize, + max: usize, + }, + /// An OP_RETURN output was supplied alongside an asset-lock payload. The asset-lock build + /// replaces the caller's outputs with the credit-mirroring burn, which already occupies the + /// transaction's one standard data slot, so the memo cannot be carried. + OpReturnOnAssetLock, } impl fmt::Display for BuilderError { @@ -961,6 +1009,25 @@ impl fmt::Display for BuilderError { } => { write!(f, "OP_RETURN payload too large: {len} bytes (max {max})") } + Self::TooManyOpReturnOutputs { + count, + max, + } => { + write!( + f, + "Too many OP_RETURN data outputs: {count} (max {max}); Dash Core's \ + IsStandardTx rejects nDataOut > {max} as multi-op-return, so the \ + transaction would not relay" + ) + } + Self::OpReturnOnAssetLock => { + write!( + f, + "OP_RETURN data outputs are not supported on an asset-lock transaction: its \ + one data output is the burn mirroring the payload credits, so a memo would \ + be silently dropped" + ) + } } } } @@ -2127,4 +2194,178 @@ mod tests { other => panic!("expected the asset-lock payload, got {other:?}"), } } + + /// Relay policy allows ONE OP_RETURN per transaction: Dash Core's `IsStandardTx` rejects + /// `nDataOut > 1` as `multi-op-return`. The #928 drain guard counted only VALUE carriers, + /// so a second memo sailed past it and produced a transaction no node will accept — and a + /// drain spends the whole balance with its inputs reserved, leaving funds stranded behind a + /// transaction that can never confirm and has no competitor to displace it. + #[test] + fn test_drain_rejects_a_second_data_carrier() { + let ctx = TestWalletContext::new_random(); + let account = + ctx.wallet.accounts.standard_bip44_accounts.get(&0).expect("BIP44 account").clone(); + let mut funds = ManagedCoreFundsAccount::dummy_bip44(); + let utxo = Utxo::dummy(0x01, 1_000_000, 100, false, true); + funds.utxos.insert(utxo.outpoint, utxo.clone()); + + let result = TransactionBuilder::new() + .set_current_height(200) + .add_funding(&mut funds, &account) + .add_output(&Address::dummy(Network::Testnet, 0), 1) + .add_op_return(b"=:ETH.ETH:0x1c7b17362c84287bd1184447e6dfeaf920c31bbe") + .expect("first memo within the ceiling") + .add_op_return(b"a second memo") + .expect("second memo within the ceiling") + .preserve_output_order() + .set_selection_strategy(SelectionStrategy::All) + .build_unsigned_reserved(); + + match result { + Err(BuilderError::TooManyOpReturnOutputs { + count, + max, + }) => { + assert_eq!(count, 2); + assert_eq!(max, MAX_STANDARD_OP_RETURN_OUTPUTS); + // The message must name the rule that rejects it, so an operator reading the + // error can match it against Dash Core's reject reason. + let rendered = BuilderError::TooManyOpReturnOutputs { + count, + max, + } + .to_string(); + assert!(rendered.contains("multi-op-return"), "got: {rendered}"); + } + other => panic!("expected a second data carrier to be rejected, got {other:?}"), + } + + // Nothing was built, so nothing may be held: a rejected build that still reserved its + // inputs would strand the balance just as effectively as the unrelayable transaction. + assert!(funds.reservations().reserved(200).is_empty()); + } + + /// The standardness rule is not drain-specific — an ordinary send with two memos is just as + /// unrelayable, so the guard sits on the final output set rather than inside the drain arm. + #[test] + fn test_ordinary_send_rejects_a_second_data_carrier() { + let result = TransactionBuilder::new() + .set_current_height(200) + .add_inputs([Utxo::dummy(0, 1_000_000, 100, false, true)]) + .set_change_address(Address::dummy(Network::Testnet, 1)) + .add_output(&Address::dummy(Network::Testnet, 0), 500_000) + .add_op_return(b"memo one") + .expect("first memo within the ceiling") + .add_op_return(b"memo two") + .expect("second memo within the ceiling") + .build_unsigned_reserved(); + + assert!( + matches!( + result, + Err(BuilderError::TooManyOpReturnOutputs { + count: 2, + .. + }) + ), + "expected a two-memo send to be rejected, got {result:?}" + ); + } + + /// A single data carrier is still fine on an ordinary send: the guard draws the line at the + /// relay limit, it does not ban memos outright. + #[test] + fn test_ordinary_send_still_allows_one_data_carrier() { + let memo = b"=:ETH.ETH:0xabc"; + let (tx, _fee, _reservation) = TransactionBuilder::new() + .set_current_height(200) + .add_inputs([Utxo::dummy(0, 1_000_000, 100, false, true)]) + .set_change_address(Address::dummy(Network::Testnet, 1)) + .add_output(&Address::dummy(Network::Testnet, 0), 500_000) + .add_op_return(memo) + .expect("memo within the ceiling") + .build_unsigned_reserved() + .expect("one data carrier builds"); + + assert_eq!( + tx.output.iter().filter(|o| o.script_pubkey.is_op_return()).count(), + 1, + "the single memo survives" + ); + assert!(tx.output.iter().any(|o| o.script_pubkey == op_return_script(memo))); + } + + /// An asset-lock build swaps the caller's outputs for the credit-mirroring burn, so a memo + /// handed to `add_op_return` used to vanish without a word: the build succeeded and the + /// caller believed their data was on-chain. Memo-on-asset-lock is unsupported; the silent + /// drop was the defect, so it must surface as a typed error. + #[test] + fn test_asset_lock_rejects_an_op_return_memo() { + let credit_script = Address::dummy(Network::Testnet, 14).script_pubkey(); + let build = |strategy: SelectionStrategy| { + TransactionBuilder::new() + .set_current_height(200) + .add_inputs([Utxo::dummy(0, 1_000_000, 100, false, true)]) + .set_change_address(Address::dummy(Network::Testnet, 1)) + .add_op_return(b"maya-memo") + .expect("memo within the ceiling") + .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload { + version: 1, + credit_outputs: vec![TxOut { + value: 100_000, + script_pubkey: credit_script.clone(), + }], + })) + .set_selection_strategy(strategy) + .build_unsigned_reserved() + }; + + // Both the drain and the ordinary asset lock discard `self.outputs`, so both must refuse. + for strategy in [SelectionStrategy::All, SelectionStrategy::BranchAndBound] { + let result = build(strategy); + assert!( + matches!(result, Err(BuilderError::OpReturnOnAssetLock)), + "expected {strategy:?} asset lock to reject the memo, got {result:?}" + ); + } + + let rendered = BuilderError::OpReturnOnAssetLock.to_string(); + assert!(rendered.contains("asset-lock"), "got: {rendered}"); + } + + /// Pins the DEFAULT (BIP-69) output order for a drain carrying a memo. All four #928 tests + /// called `preserve_output_order`, leaving the shape callers get by default untested. + /// + /// BIP-69 sorts by value ascending, so the zero-value data carrier lands at VOUT0 and the + /// destination follows. That is intended and is left as-is: output order carries no + /// consensus or relay meaning — `IsStandardTx` counts data outputs but never inspects their + /// position — and no consumer reads the memo positionally (`add_op_return` has no call site + /// outside this module, and the FFI does not expose it). A caller that needs a specific + /// layout, e.g. a vault expecting VOUT0, opts into `preserve_output_order` as the MAYA-style + /// test above does. This test exists so that shape changes deliberately rather than silently. + #[test] + fn test_drain_default_output_order_places_the_zero_value_memo_first() { + let vault = Address::dummy(Network::Testnet, 0); + let memo = b"=:ETH.ETH:0x1c7b17362c84287bd1184447e6dfeaf920c31bbe"; + + let (tx, fee, _reservation) = TransactionBuilder::new() + .set_current_height(200) + .add_inputs([Utxo::dummy(0, 100_000, 100, false, true)]) + .add_output(&vault, 1) // ignored by a drain + .add_op_return(memo) + .expect("memo within the OP_RETURN ceiling") + .set_selection_strategy(SelectionStrategy::All) + .build_unsigned_reserved() + .expect("a default-ordered drain with a data carrier builds"); + + assert_eq!(tx.output.len(), 2, "vault + memo, no change"); + // VOUT0 is the zero-value memo: BIP-69 orders by value, and 0 sorts first. + assert_eq!(tx.output[0].value, 0); + assert_eq!(tx.output[0].script_pubkey, op_return_script(memo)); + // VOUT1 is the destination, holding the whole drained balance. + assert_eq!(tx.output[1].value, 100_000 - fee); + assert_eq!(tx.output[1].script_pubkey, vault.script_pubkey()); + // The drain arithmetic is order-independent: nothing is left behind either way. + assert_eq!(tx.output.iter().map(|o| o.value).sum::() + fee, 100_000); + } }