diff --git a/processor/src/multisigs/mod.rs b/processor/src/multisigs/mod.rs index 20d2bce1..f9cd4feb 100644 --- a/processor/src/multisigs/mod.rs +++ b/processor/src/multisigs/mod.rs @@ -381,19 +381,15 @@ impl MultisigManager { costs of embedding arbitrary data. Since we can't rely on the Eventuality system to detect if it's a forwarded transaction, - due to the asynchonicity of the Eventuality system, we instead interpret an External - output with no InInstruction, which has an amount associated with an InInstruction - being forwarded, as having been forwarded. This does create a specific edge case where - a user who doesn't include an InInstruction may not be refunded however, if they share - an exact amount with an expected-to-be-forwarded transaction. This is deemed acceptable. - - TODO: Add a fourth address, forwarded_address, to prevent this. + due to the asynchonicity of the Eventuality system, we instead interpret an Forwarded + output which has an amount associated with an InInstruction which was forwarded as having + been forwarded. */ Plan { key: self.existing.as_ref().unwrap().key, payments: vec![Payment { - address: N::address(self.new.as_ref().unwrap().key), + address: N::forward_address(self.new.as_ref().unwrap().key), data: None, balance: output.balance(), }], @@ -582,6 +578,7 @@ impl MultisigManager { } false } + OutputType::Forwarded => false, } }); plans @@ -728,18 +725,15 @@ impl MultisigManager { running_operating_costs, ); - // This will false positive on a transaction being refunded which meets these conditions - // That doesn't impact any of the following code at this time yet shows consideration - // is needed here in the future - let to_be_forwarded = { - let output = &plan.inputs[0]; - (step == RotationStep::ForwardFromExisting) && - (output.kind() == OutputType::External) && - (output.key() == self.existing.as_ref().unwrap().key) - }; - if to_be_forwarded { - assert_eq!(plan.inputs.len(), 1); - } + // TODO: Don't do an error-prone post-detection for if this is being forwarded + // Be told this is being forwarded + let to_be_forwarded = (step == RotationStep::ForwardFromExisting) && + plan + .payments + .first() + .map(|payment| payment.address == N::forward_address(self.new.as_ref().unwrap().key)) + .unwrap_or(false) && + plan.change.is_none(); // If we're forwarding this output, don't take the opportunity to amortze operating costs // The scanner handler below, in order to properly save forwarded outputs' instructions, @@ -823,26 +817,36 @@ impl MultisigManager { .expect("didn't have the block number for a block we just scanned"); let step = self.current_rotation_step(block_number); - // If these aren't externally received funds, don't handle it as an instruction + // Instructions created from this block + let mut instructions = vec![]; + + // If any of these outputs were forwarded, create their instruction now + for output in &outputs { + if output.kind() != OutputType::Forwarded { + continue; + } + + if let Some(instruction) = + MultisigsDb::::take_forwarded_output(txn, output.balance()) + { + instructions.push(instruction); + } + } + + // If the remaining outputs aren't externally received funds, don't handle them as + // instructions outputs.retain(|output| output.kind() == OutputType::External); // These plans are of limited context. They're only allowed the outputs newly received // within this block and are intended to handle forwarding transactions/refunds - // Those all end up as plans with a single input, leading to a later check: - /* - if to_be_forwarded { - assert_eq!(plan.inputs.len(), 1); - } - Hence the name, which places that assumed precondition into verbiage here. - */ - let mut single_input_plans = vec![]; + let mut plans = vec![]; // If the old multisig is explicitly only supposed to forward, create all such plans now if step == RotationStep::ForwardFromExisting { let mut i = 0; while i < outputs.len() { let output = &outputs[i]; - let single_input_plans = &mut single_input_plans; + let plans = &mut plans; let txn = &mut *txn; #[allow(clippy::redundant_closure_call)] @@ -852,12 +856,12 @@ impl MultisigManager { return true; } - let plans_at_start = single_input_plans.len(); + let plans_at_start = plans.len(); let (refund_to, instruction) = instruction_from_output::(output); if let Some(mut instruction) = instruction { // Build a dedicated Plan forwarding this let forward_plan = self.forward_plan(output.clone()); - single_input_plans.push(forward_plan.clone()); + plans.push(forward_plan.clone()); // Set the instruction for this output to be returned // We need to set it under the amount it's forwarded with, so prepare its forwarding @@ -879,12 +883,12 @@ impl MultisigManager { } else if let Some(refund_to) = refund_to { if let Ok(refund_to) = refund_to.consume().try_into() { // Build a dedicated Plan refunding this - single_input_plans.push(Self::refund_plan(output.clone(), refund_to)); + plans.push(Self::refund_plan(output.clone(), refund_to)); } } // Only keep if we didn't make a Plan consuming it - plans_at_start == single_input_plans.len() + plans_at_start == plans.len() })() .await; if should_retain { @@ -895,7 +899,6 @@ impl MultisigManager { } } - let mut instructions = vec![]; for output in outputs { // If this is an External transaction to the existing multisig, and we're either solely // forwarding or closing the existing multisig, drop it @@ -909,33 +912,15 @@ impl MultisigManager { } let (refund_to, instruction) = instruction_from_output::(&output); - let refund = || { - if let Some(refund_to) = refund_to { - if let Ok(refund_to) = refund_to.consume().try_into() { - single_input_plans.push(Self::refund_plan(output.clone(), refund_to)) - } - } - }; let instruction = if let Some(instruction) = instruction { instruction } else { - // If the output data is empty, this may be a forwarded transaction from a prior - // multisig - // If it's not empty, it's corrupt in some way and should be refunded - if !output.data().is_empty() { - refund(); - continue; - } - - if let Some(instruction) = - MultisigsDb::::take_forwarded_output(txn, output.balance()) - { - instruction - } else { - // If it's not a forwarded output, refund - refund(); - continue; + if let Some(refund_to) = refund_to { + if let Ok(refund_to) = refund_to.consume().try_into() { + plans.push(Self::refund_plan(output.clone(), refund_to)); + } } + continue; }; // Delay External outputs received to new multisig earlier than expected @@ -956,7 +941,7 @@ impl MultisigManager { // Save the plans created while scanning // TODO: Should we combine all of these plans? - MultisigsDb::::set_plans_from_scanning(txn, block_number, single_input_plans); + MultisigsDb::::set_plans_from_scanning(txn, block_number, plans); // If any outputs were delayed, append them into this block match step { diff --git a/processor/src/networks/bitcoin.rs b/processor/src/networks/bitcoin.rs index 688823ad..e28f6ba3 100644 --- a/processor/src/networks/bitcoin.rs +++ b/processor/src/networks/bitcoin.rs @@ -302,6 +302,7 @@ impl BlockTrait for Block { const KEY_DST: &[u8] = b"Serai Bitcoin Output Offset"; static BRANCH_OFFSET: Lazy = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"branch")); static CHANGE_OFFSET: Lazy = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"change")); +static FORWARD_OFFSET: Lazy = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"forward")); // Always construct the full scanner in order to ensure there's no collisions fn scanner( @@ -325,6 +326,7 @@ fn scanner( register(OutputType::Branch, *BRANCH_OFFSET); register(OutputType::Change, *CHANGE_OFFSET); + register(OutputType::Forwarded, *FORWARD_OFFSET); (scanner, offsets, kinds) } @@ -550,6 +552,11 @@ impl Network for Bitcoin { Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Change])) } + fn forward_address(key: ProjectivePoint) -> Address { + let (_, offsets, _) = scanner(key); + Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Forwarded])) + } + async fn get_latest_block_number(&self) -> Result { self.rpc.get_latest_block_number().await.map_err(|_| NetworkError::ConnectionError) } diff --git a/processor/src/networks/mod.rs b/processor/src/networks/mod.rs index 162203b3..86d19637 100644 --- a/processor/src/networks/mod.rs +++ b/processor/src/networks/mod.rs @@ -71,6 +71,9 @@ pub enum OutputType { // Should be added to the available UTXO pool with no further action Change, + + // Forwarded output from the prior multisig + Forwarded, } impl OutputType { @@ -79,6 +82,7 @@ impl OutputType { OutputType::External => 0, OutputType::Branch => 1, OutputType::Change => 2, + OutputType::Forwarded => 3, }]) } @@ -89,6 +93,7 @@ impl OutputType { 0 => OutputType::External, 1 => OutputType::Branch, 2 => OutputType::Change, + 3 => OutputType::Forwarded, _ => Err(io::Error::new(io::ErrorKind::Other, "invalid OutputType"))?, }) } @@ -293,6 +298,8 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug { fn branch_address(key: ::G) -> Self::Address; /// Address for the given group key to use for change. fn change_address(key: ::G) -> Self::Address; + /// Address for forwarded outputs from prior multisigs. + fn forward_address(key: ::G) -> Self::Address; /// Get the latest block's number. async fn get_latest_block_number(&self) -> Result; diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index 30dc392f..06aa5a1d 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -49,6 +49,7 @@ pub struct Output(SpendableOutput, Vec); const EXTERNAL_SUBADDRESS: Option = SubaddressIndex::new(0, 0); const BRANCH_SUBADDRESS: Option = SubaddressIndex::new(1, 0); const CHANGE_SUBADDRESS: Option = SubaddressIndex::new(2, 0); +const FORWARD_SUBADDRESS: Option = SubaddressIndex::new(3, 0); impl OutputTrait for Output { // While we could use (tx, o), using the key ensures we won't be susceptible to the burning bug. @@ -61,6 +62,7 @@ impl OutputTrait for Output { EXTERNAL_SUBADDRESS => OutputType::External, BRANCH_SUBADDRESS => OutputType::Branch, CHANGE_SUBADDRESS => OutputType::Change, + FORWARD_SUBADDRESS => OutputType::Forwarded, _ => panic!("unrecognized address was scanned for"), } } @@ -262,6 +264,7 @@ impl Monero { debug_assert!(EXTERNAL_SUBADDRESS.is_none()); scanner.register_subaddress(BRANCH_SUBADDRESS.unwrap()); scanner.register_subaddress(CHANGE_SUBADDRESS.unwrap()); + scanner.register_subaddress(FORWARD_SUBADDRESS.unwrap()); scanner } @@ -484,6 +487,10 @@ impl Network for Monero { Self::address_internal(key, CHANGE_SUBADDRESS) } + fn forward_address(key: EdwardsPoint) -> Address { + Self::address_internal(key, FORWARD_SUBADDRESS) + } + async fn get_latest_block_number(&self) -> Result { // Monero defines height as chain length, so subtract 1 for block number Ok(self.rpc.get_height().await.map_err(map_rpc_err)? - 1) @@ -520,7 +527,7 @@ impl Network for Monero { // This just ensures nothing invalid makes it through for tx_outputs in &txs { for output in tx_outputs { - assert!([EXTERNAL_SUBADDRESS, BRANCH_SUBADDRESS, CHANGE_SUBADDRESS] + assert!([EXTERNAL_SUBADDRESS, BRANCH_SUBADDRESS, CHANGE_SUBADDRESS, FORWARD_SUBADDRESS] .contains(&output.output.metadata.subaddress)); } } diff --git a/processor/src/tests/addresses.rs b/processor/src/tests/addresses.rs index f9c621ee..8f36f1a5 100644 --- a/processor/src/tests/addresses.rs +++ b/processor/src/tests/addresses.rs @@ -22,7 +22,7 @@ async fn spend( keys: &HashMap>, scanner: &mut ScannerHandle, outputs: Vec, -) -> Vec { +) { let key = keys[&Participant::new(1).unwrap()].group_key(); let mut keys_txs = HashMap::new(); @@ -62,11 +62,11 @@ async fn spend( assert_eq!(outputs.len(), 1); // Make sure this is actually a change output assert_eq!(outputs[0].kind(), OutputType::Change); + assert_eq!(outputs[0].key(), key); let mut txn = db.txn(); assert_eq!(scanner.ack_block(&mut txn, block).await.1, outputs); scanner.release_lock().await; txn.commit(); - outputs } ScannerEvent::Completed(_, _, _, _) => { panic!("unexpectedly got eventuality completion"); @@ -96,31 +96,37 @@ pub async fn test_addresses(network: N) { network.mine_block().await; } - // Receive funds to the branch address and make sure it's properly identified - let block_id = network.test_send(N::branch_address(key)).await.id(); + // Receive funds to the various addresses and make sure they're properly identified + let mut received_outputs = vec![]; + for (kind, address) in [ + (OutputType::External, N::address(key)), + (OutputType::Branch, N::branch_address(key)), + (OutputType::Change, N::change_address(key)), + (OutputType::Forwarded, N::forward_address(key)), + ] { + let block_id = network.test_send(address).await.id(); - // Verify the Scanner picked them up - let outputs = + // Verify the Scanner picked them up match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() { ScannerEvent::Block { is_retirement_block, block, outputs } => { scanner.multisig_completed.send(false).unwrap(); assert!(!is_retirement_block); assert_eq!(block, block_id); assert_eq!(outputs.len(), 1); - assert_eq!(outputs[0].kind(), OutputType::Branch); + assert_eq!(outputs[0].kind(), kind); + assert_eq!(outputs[0].key(), key); let mut txn = db.txn(); assert_eq!(scanner.ack_block(&mut txn, block).await.1, outputs); scanner.release_lock().await; txn.commit(); - outputs + received_outputs.extend(outputs); } ScannerEvent::Completed(_, _, _, _) => { panic!("unexpectedly got eventuality completion"); } }; + } // Spend the branch output, creating a change output and ensuring we actually get change - let outputs = spend(&mut db, &network, &keys, &mut scanner, outputs).await; - // Also test spending the change output - spend(&mut db, &network, &keys, &mut scanner, outputs).await; + spend(&mut db, &network, &keys, &mut scanner, received_outputs).await; }