From 2512851c8cfaca0ca623caa93d36dfe2df8a0fdf Mon Sep 17 00:00:00 2001 From: Mykhailo Kremniov Date: Tue, 1 Jul 2025 19:12:01 +0300 Subject: [PATCH 1/2] Remove destination from FillOrder v1 inputs; require that such inputs are never signed; --- Cargo.lock | 1 + .../scanner-lib/src/blockchain_state/mod.rs | 6 +- .../stack-test-suite/tests/v2/orders.rs | 10 +- api-server/web-server/src/api/json_helpers.rs | 6 +- .../src/constraints_accumulator.rs | 16 +- .../src/tests/orders_constraints.rs | 41 +- chainstate/src/detail/chainstateref/mod.rs | 31 +- chainstate/src/rpc/types/account.rs | 4 +- .../test-framework/src/random_tx_maker.rs | 14 +- .../src/signature_destination_getter.rs | 6 +- .../test-suite/src/tests/input_commitments.rs | 8 +- .../test-suite/src/tests/orders_tests.rs | 502 +++++++++++++----- .../transaction_verifier/check_transaction.rs | 2 +- .../input_check/signature_only_check.rs | 7 + .../src/transaction_verifier/mod.rs | 4 +- .../src/chain/transaction/account_outpoint.rs | 23 +- common/src/chain/transaction/signature/mod.rs | 32 +- .../sighash/input_commitments/mod.rs | 2 +- .../signature/tests/sign_and_mutate.rs | 10 +- mintscript/src/tests/translate/mod.rs | 112 +++- mintscript/src/translate.rs | 29 +- .../tests/no_discouragement_after_tx_reorg.rs | 1 - ...t_order_double_fill_with_same_dest_impl.py | 9 +- trezor-common/src/lib.rs | 4 +- trezor-common/src/tests/mod.rs | 4 +- wallet/src/account/mod.rs | 27 +- wallet/src/account/output_cache/mod.rs | 6 +- .../tests/generic_fixed_signature_tests.rs | 189 +++---- wallet/src/signer/tests/generic_tests.rs | 22 +- wallet/src/signer/trezor_signer/mod.rs | 3 +- wallet/types/Cargo.toml | 1 + .../types/src/partially_signed_transaction.rs | 28 +- wallet/wallet-controller/src/helpers.rs | 2 +- wasm-wrappers/WASM-API.md | 17 +- .../js-bindings-test/tests/test_orders.ts | 11 +- wasm-wrappers/src/encode_input.rs | 25 +- 36 files changed, 791 insertions(+), 424 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 625e40e918..e7b8f1379c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9370,6 +9370,7 @@ dependencies = [ "crypto", "hex", "itertools 0.14.0", + "logging", "parity-scale-codec", "randomness", "rpc-description", diff --git a/api-server/scanner-lib/src/blockchain_state/mod.rs b/api-server/scanner-lib/src/blockchain_state/mod.rs index d9a4e10b48..6f6a1880b0 100644 --- a/api-server/scanner-lib/src/blockchain_state/mod.rs +++ b/api-server/scanner-lib/src/blockchain_state/mod.rs @@ -594,7 +594,7 @@ async fn calculate_tx_fee_and_collect_token_info( } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { let order = db_tx.get_order(*order_id).await?.expect("must exist"); @@ -829,7 +829,7 @@ async fn prefetch_orders( match input { TxInput::Utxo(_) | TxInput::Account(_) => {} TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { let order = db_tx.get_order(*order_id).await?.expect("must be present "); @@ -1227,7 +1227,7 @@ async fn update_tables_from_transaction_inputs( } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency, _) => { + OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency) => { let order = db_tx.get_order(*order_id).await?.expect("must exist"); let order = order.fill(&chain_config, block_height, *fill_amount_in_ask_currency); diff --git a/api-server/stack-test-suite/tests/v2/orders.rs b/api-server/stack-test-suite/tests/v2/orders.rs index 91ea40a703..dabb09d78b 100644 --- a/api-server/stack-test-suite/tests/v2/orders.rs +++ b/api-server/stack-test-suite/tests/v2/orders.rs @@ -86,13 +86,9 @@ async fn create_fill_conclude_order(#[case] seed: Seed, #[case] version: OrdersV Destination::AnyoneCanSpend, ), ), - OrdersVersion::V1 => { - TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - Amount::from_atoms(1), - Destination::AnyoneCanSpend, - )) - } + OrdersVersion::V1 => TxInput::OrderAccountCommand( + OrderAccountCommand::FillOrder(order_id, Amount::from_atoms(1)), + ), }; let tx2 = TransactionBuilder::new() .add_input( diff --git a/api-server/web-server/src/api/json_helpers.rs b/api-server/web-server/src/api/json_helpers.rs index da28fd0d4c..4b5819823b 100644 --- a/api-server/web-server/src/api/json_helpers.rs +++ b/api-server/web-server/src/api/json_helpers.rs @@ -323,13 +323,12 @@ pub fn tx_input_to_json( } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(id, fill, dest) => { + OrderAccountCommand::FillOrder(id, fill) => { json!({ "input_type": "OrderAccountCommand", "command": "FillOrder", "order_id": Address::new(chain_config, *id).expect("addressable").to_string(), "fill_atoms": json!({"atoms": fill.into_atoms().to_string()}), - "destination": Address::new(chain_config, dest.clone()).expect("addressable").as_str(), }) } OrderAccountCommand::ConcludeOrder(order_id) => { @@ -410,13 +409,12 @@ pub fn tx_input_to_json( "order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(), }) } - AccountCommand::FillOrder(order_id, fill, dest) => { + AccountCommand::FillOrder(order_id, fill, _) => { json!({ "input_type": "AccountCommand", "command": "FillOrder", "order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(), "fill_atoms": json!({"atoms": fill.into_atoms().to_string()}), - "destination": Address::new(chain_config, dest.clone()).expect("no error").as_str(), }) } AccountCommand::ChangeTokenMetadataUri(token_id, metadata_uri) => { diff --git a/chainstate/constraints-value-accumulator/src/constraints_accumulator.rs b/chainstate/constraints-value-accumulator/src/constraints_accumulator.rs index 7184c9ce1b..e204a0c809 100644 --- a/chainstate/constraints-value-accumulator/src/constraints_accumulator.rs +++ b/chainstate/constraints-value-accumulator/src/constraints_accumulator.rs @@ -329,15 +329,13 @@ impl ConstrainedValueAccumulator { O: OrdersAccountingOperations + OrdersAccountingView, { let result = match command { - OrderAccountCommand::FillOrder(id, amount, _) => { - Some(self.process_fill_order_command( - chain_config, - block_height, - *id, - *amount, - orders_accounting_delta, - )?) - } + OrderAccountCommand::FillOrder(id, amount) => Some(self.process_fill_order_command( + chain_config, + block_height, + *id, + *amount, + orders_accounting_delta, + )?), OrderAccountCommand::ConcludeOrder(order_id) => { Some(self.process_conclude_order_command(*order_id, orders_accounting_delta)?) } diff --git a/chainstate/constraints-value-accumulator/src/tests/orders_constraints.rs b/chainstate/constraints-value-accumulator/src/tests/orders_constraints.rs index 9068fa8b3d..21bdc96291 100644 --- a/chainstate/constraints-value-accumulator/src/tests/orders_constraints.rs +++ b/chainstate/constraints-value-accumulator/src/tests/orders_constraints.rs @@ -309,7 +309,6 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, (ask_amount + Amount::from_atoms(1)).unwrap(), - Destination::AnyoneCanSpend, )), }; let inputs = vec![ @@ -354,11 +353,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let inputs = vec![ TxInput::Utxo(UtxoOutPoint::new( @@ -395,11 +392,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let inputs = vec![ TxInput::Utxo(UtxoOutPoint::new( @@ -451,11 +446,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let inputs = vec![ TxInput::Utxo(UtxoOutPoint::new( @@ -512,11 +505,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let inputs = vec![ TxInput::Utxo(UtxoOutPoint::new( @@ -571,11 +562,9 @@ fn fill_order_constraints(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let inputs = vec![ TxInput::Utxo(UtxoOutPoint::new( diff --git a/chainstate/src/detail/chainstateref/mod.rs b/chainstate/src/detail/chainstateref/mod.rs index c72b10b81d..63aa5ba39c 100644 --- a/chainstate/src/detail/chainstateref/mod.rs +++ b/chainstate/src/detail/chainstateref/mod.rs @@ -41,8 +41,8 @@ use common::{ }, config::EpochIndex, tokens::{TokenAuxiliaryData, TokenId}, - AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, PoolId, Transaction, - TxOutput, UtxoOutPoint, + AccountNonce, AccountType, Block, ChainConfig, GenBlock, GenBlockId, OrderAccountCommand, + PoolId, Transaction, TxInput, TxOutput, UtxoOutPoint, }, primitives::{ id::WithId, time::Time, Amount, BlockCount, BlockDistance, BlockHeight, Id, Idable, @@ -779,14 +779,29 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat #[log_error] fn check_duplicate_inputs(&self, block: &Block) -> Result<(), CheckBlockTransactionsError> { - // check for duplicate inputs (see CVE-2018-17144) - let mut block_inputs = BTreeSet::new(); + // Reject the block if it has duplicate inputs, with the exception of v1 FillOrder inputs, + // which can't be unique (because they only contain the order id and the amount). + // Note that this is a precaution "inspired" by the Bitcoin vulnerability CVE-2018-17144. + // I.e. even if this check is removed, the corresponding erroneous conditions (like spending + // the same UTXO twice or concluding an already concluded order) should be captured elsewhere. + let mut block_unique_inputs = BTreeSet::new(); for tx in block.transactions() { for input in tx.inputs() { - ensure!( - block_inputs.insert(input), - CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id()) - ); + let must_be_unique = match input { + TxInput::Utxo(_) | TxInput::Account(_) | TxInput::AccountCommand(_, _) => true, + TxInput::OrderAccountCommand(cmd) => match cmd { + OrderAccountCommand::FillOrder(_, _) => false, + | OrderAccountCommand::FreezeOrder(_) + | OrderAccountCommand::ConcludeOrder(_) => true, + }, + }; + + if must_be_unique { + ensure!( + block_unique_inputs.insert(input), + CheckBlockTransactionsError::DuplicateInputInBlock(block.get_id()) + ); + } } } Ok(()) diff --git a/chainstate/src/rpc/types/account.rs b/chainstate/src/rpc/types/account.rs index c78dddb266..6ee0681ef3 100644 --- a/chainstate/src/rpc/types/account.rs +++ b/chainstate/src/rpc/types/account.rs @@ -145,7 +145,6 @@ pub enum RpcOrderAccountCommand { Fill { order_id: RpcAddress, fill_value: RpcAmountOut, - destination: RpcAddress, }, Freeze { order_id: RpcAddress, @@ -161,10 +160,9 @@ impl RpcOrderAccountCommand { OrderAccountCommand::ConcludeOrder(order_id) => RpcOrderAccountCommand::Conclude { order_id: RpcAddress::new(chain_config, *order_id)?, }, - OrderAccountCommand::FillOrder(id, fill, dest) => RpcOrderAccountCommand::Fill { + OrderAccountCommand::FillOrder(id, fill) => RpcOrderAccountCommand::Fill { order_id: RpcAddress::new(chain_config, *id)?, fill_value: RpcAmountOut::from_amount(*fill, chain_config.coin_decimals()), - destination: RpcAddress::new(chain_config, dest.clone())?, }, OrderAccountCommand::FreezeOrder(id) => RpcOrderAccountCommand::Freeze { order_id: RpcAddress::new(chain_config, *id)?, diff --git a/chainstate/test-framework/src/random_tx_maker.rs b/chainstate/test-framework/src/random_tx_maker.rs index ee5c328800..194fdc6c8f 100644 --- a/chainstate/test-framework/src/random_tx_maker.rs +++ b/chainstate/test-framework/src/random_tx_maker.rs @@ -1021,13 +1021,9 @@ impl<'a> RandomTxMaker<'a> { if !is_frozen_token(&filled_value, tokens_cache) && !is_frozen_order(&orders_cache, order_id) { - let input = - TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - amount_to_spend, - key_manager - .new_destination(self.chainstate.get_chain_config(), rng), - )); + let input = TxInput::OrderAccountCommand( + OrderAccountCommand::FillOrder(order_id, amount_to_spend), + ); let output = TxOutput::Transfer( filled_value, @@ -1260,10 +1256,6 @@ impl<'a> RandomTxMaker<'a> { OrderAccountCommand::FillOrder( order_id, Amount::from_atoms(atoms), - key_manager.new_destination( - self.chainstate.get_chain_config(), - rng, - ), ), )); diff --git a/chainstate/test-framework/src/signature_destination_getter.rs b/chainstate/test-framework/src/signature_destination_getter.rs index 7d83fb3b1c..f0be0eb391 100644 --- a/chainstate/test-framework/src/signature_destination_getter.rs +++ b/chainstate/test-framework/src/signature_destination_getter.rs @@ -184,10 +184,12 @@ impl<'a> SignatureDestinationGetter<'a> { ))?; Ok(order_data.conclude_key().clone()) } - AccountCommand::FillOrder(_, _, d) => Ok(d.clone()), + // Signatures on FillOrder v0 inputs are not checked. + AccountCommand::FillOrder(_, _, _) => Ok(Destination::AnyoneCanSpend), }, TxInput::OrderAccountCommand(command) => match command { - OrderAccountCommand::FillOrder(_, _, d) => Ok(d.clone()), + // FillOrder v1 inputs must not be signed. + OrderAccountCommand::FillOrder(_, _) => Ok(Destination::AnyoneCanSpend), OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { let order_data = orders_view diff --git a/chainstate/test-suite/src/tests/input_commitments.rs b/chainstate/test-suite/src/tests/input_commitments.rs index 1d742cfb76..47b39f88b6 100644 --- a/chainstate/test-suite/src/tests/input_commitments.rs +++ b/chainstate/test-suite/src/tests/input_commitments.rs @@ -853,11 +853,9 @@ fn make_fill_order_input( nonce, AccountCommand::FillOrder(*order_id, fill_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - *order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(*order_id, fill_amount)) + } } } diff --git a/chainstate/test-suite/src/tests/orders_tests.rs b/chainstate/test-suite/src/tests/orders_tests.rs index dad2137445..29b7ccd180 100644 --- a/chainstate/test-suite/src/tests/orders_tests.rs +++ b/chainstate/test-suite/src/tests/orders_tests.rs @@ -17,7 +17,10 @@ use std::borrow::Cow; use rstest::rstest; -use chainstate::{CheckBlockError, CheckBlockTransactionsError, ConnectTransactionError}; +use chainstate::{ + BlockError, ChainstateError, CheckBlockError, CheckBlockTransactionsError, + ConnectTransactionError, +}; use chainstate_test_framework::{output_value_amount, TestFramework, TransactionBuilder}; use common::{ address::pubkeyhash::PublicKeyHash, @@ -27,7 +30,7 @@ use common::{ signature::{ inputsig::{standard_signature::StandardInputSignature, InputWitness}, sighash::{input_commitments::SighashInputCommitment, sighashtype::SigHashType}, - DestinationSigError, + verify_signature, DestinationSigError, EvaluatedInputWitness, }, tokens::{IsTokenFreezable, TokenId, TokenTotalSupply}, AccountCommand, AccountNonce, ChainstateUpgradeBuilder, Destination, OrderAccountCommand, @@ -44,7 +47,8 @@ use test_utils::{ random::{gen_random_bytes, make_seedable_rng, Seed}, }; use tx_verifier::{ - error::{InputCheckError, ScriptError, TranslationError}, + error::{InputCheckError, InputCheckErrorPayload, ScriptError, TranslationError}, + input_check::signature_only_check::verify_tx_signature, CheckTransactionError, }; @@ -142,7 +146,7 @@ fn create_order_check_storage(#[case] seed: Seed) { #[rstest] #[trace] #[case(Seed::from_entropy())] -fn create_two_same_orders_in_tx(#[case] seed: Seed) { +fn create_two_identical_orders_same_tx(#[case] seed: Seed) { utils::concurrency::model(move || { let mut rng = make_seedable_rng(seed); let mut tf = TestFramework::builder(&mut rng).build(); @@ -184,7 +188,7 @@ fn create_two_same_orders_in_tx(#[case] seed: Seed) { #[rstest] #[trace] #[case(Seed::from_entropy())] -fn create_two_orders_same_tx(#[case] seed: Seed) { +fn create_two_different_orders_same_tx(#[case] seed: Seed) { utils::concurrency::model(move || { let mut rng = make_seedable_rng(seed); let mut tf = TestFramework::builder(&mut rng).build(); @@ -232,7 +236,7 @@ fn create_two_orders_same_tx(#[case] seed: Seed) { #[rstest] #[trace] #[case(Seed::from_entropy())] -fn create_two_orders_same_block(#[case] seed: Seed) { +fn create_two_identical_orders_same_block(#[case] seed: Seed) { utils::concurrency::model(move || { let mut rng = make_seedable_rng(seed); let mut tf = TestFramework::builder(&mut rng).build(); @@ -656,11 +660,9 @@ fn fill_order_check_storage(#[case] seed: Seed, #[case] version: OrdersVersion) AccountNonce::new(0), AccountCommand::FillOrder(order_id, fill_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)) + } }; let tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) @@ -698,11 +700,9 @@ fn fill_order_check_storage(#[case] seed: Seed, #[case] version: OrdersVersion) AccountNonce::new(1), AccountCommand::FillOrder(order_id, left_to_fill, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - left_to_fill, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, left_to_fill)) + } }; let tx = TransactionBuilder::new() @@ -789,11 +789,9 @@ fn fill_partially_then_conclude(#[case] seed: Seed, #[case] version: OrdersVersi AccountNonce::new(0), AccountCommand::FillOrder(order_id, fill_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)) + } }; let tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) @@ -929,7 +927,6 @@ fn fill_partially_then_conclude(#[case] seed: Seed, #[case] version: OrdersVersi OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, (give_amount - filled_amount).unwrap(), - Destination::AnyoneCanSpend, )), }; let tx = TransactionBuilder::new() @@ -1025,11 +1022,9 @@ fn try_overbid_order_in_multiple_txs(#[case] seed: Seed, #[case] version: Orders AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let tx1 = TransactionBuilder::new() .add_input( @@ -1055,7 +1050,6 @@ fn try_overbid_order_in_multiple_txs(#[case] seed: Seed, #[case] version: Orders OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, Amount::from_atoms(1), - Destination::AnyoneCanSpend, )), }; let tx2 = TransactionBuilder::new() @@ -1134,9 +1128,7 @@ fn fill_completely_then_conclude(#[case] seed: Seed, #[case] version: OrdersVers AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, + order_id, ask_amount, )), }; let tx = TransactionBuilder::new() @@ -1178,7 +1170,6 @@ fn fill_completely_then_conclude(#[case] seed: Seed, #[case] version: OrdersVers OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, (ask_amount + Amount::from_atoms(1)).unwrap(), - Destination::AnyoneCanSpend, )), }; let tx = TransactionBuilder::new() @@ -1218,11 +1209,9 @@ fn fill_completely_then_conclude(#[case] seed: Seed, #[case] version: OrdersVers AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; let tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) @@ -1477,11 +1466,9 @@ fn reorg_before_create(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, fill_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)) + } }; let fill_order_tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) @@ -1598,11 +1585,9 @@ fn reorg_after_create(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, fill_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)) + } }; let fill_order_tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) @@ -1850,9 +1835,7 @@ fn create_order_with_nft(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, + order_id, ask_amount, )), }; let tx = TransactionBuilder::new() @@ -1886,11 +1869,9 @@ fn create_order_with_nft(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, ask_amount, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, ask_amount)) + } }; tf.make_block_builder() .add_transaction( @@ -2229,7 +2210,6 @@ fn partially_fill_order_with_nft_v1(#[case] seed: Seed) { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, underbid_amount, - Destination::AnyoneCanSpend, )), InputWitness::NoSignature(None), ) @@ -2265,9 +2245,7 @@ fn partially_fill_order_with_nft_v1(#[case] seed: Seed) { ) .add_input( TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - ask_amount, - Destination::AnyoneCanSpend, + order_id, ask_amount, )), InputWitness::NoSignature(None), ) @@ -2331,11 +2309,9 @@ fn fill_order_with_zero(#[case] seed: Seed, #[case] version: OrdersVersion) { AccountNonce::new(0), AccountCommand::FillOrder(order_id, Amount::ZERO, Destination::AnyoneCanSpend), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - Amount::ZERO, - Destination::AnyoneCanSpend, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, Amount::ZERO)) + } }; let tx = TransactionBuilder::new() .add_input(fill_input, InputWitness::NoSignature(None)) @@ -2434,8 +2410,6 @@ fn fill_orders_shuffle(#[case] seed: Seed, #[case] fills: Vec) { let mut fill_txs = Vec::new(); for (i, fill_atoms) in fill_order_atoms.iter().enumerate() { - // Destination of fill order must be unique to avoid duplicating inputs - let (_, pk) = PrivateKey::new_from_rng(&mut rng, KeyKind::Secp256k1Schnorr); let tx = TransactionBuilder::new() .add_input( TxInput::from_utxo(tx_with_coins_to_fill_id.into(), i as u32), @@ -2445,7 +2419,6 @@ fn fill_orders_shuffle(#[case] seed: Seed, #[case] fills: Vec) { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, Amount::from_atoms(*fill_atoms), - Destination::PublicKey(pk), )), InputWitness::NoSignature(None), ) @@ -2537,7 +2510,6 @@ fn orders_v1_activation(#[case] seed: Seed) { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, Amount::ZERO, - Destination::AnyoneCanSpend, )), InputWitness::NoSignature(None), ) @@ -2760,7 +2732,6 @@ fn create_order_fill_activate_fork_fill_conclude(#[case] seed: Seed) { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, fill_amount, - Destination::AnyoneCanSpend, )), InputWitness::NoSignature(None), ) @@ -3087,11 +3058,7 @@ fn fill_freeze_conclude_order(#[case] seed: Seed) { let fill_tx = TransactionBuilder::new() .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) .add_input( - TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - Destination::AnyoneCanSpend, - )), + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)), InputWitness::NoSignature(None), ) .add_output(TxOutput::Transfer( @@ -3129,7 +3096,6 @@ fn fill_freeze_conclude_order(#[case] seed: Seed) { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, left_to_fill, - Destination::AnyoneCanSpend, )), InputWitness::NoSignature(None), ) @@ -3290,25 +3256,24 @@ fn order_with_zero_value(#[case] seed: Seed, #[case] version: OrdersVersion) { }); } -// The destination specified in FillOrder inputs is there only to make the inputs distinct -// across multiple transactions. So this test proves that: +// The destination specified in v0 FillOrder inputs exists only due to historical reasons. +// So this test proves that: // 1) The destination doesn't have to be the same as the actual output destination. // 2) The signature for a FillOrder input is not enforced, i.e. it can be empty or correspond // to some unrelated destination or contain arbitrary data. #[rstest] #[trace] -#[case(Seed::from_entropy(), OrdersVersion::V0)] -#[case(Seed::from_entropy(), OrdersVersion::V1)] -fn fill_order_destination_irrelevancy(#[case] seed: Seed, #[case] version: OrdersVersion) { +#[case(Seed::from_entropy())] +fn fill_order_v0_destination_irrelevancy(#[case] seed: Seed) { utils::concurrency::model(move || { let mut rng = make_seedable_rng(seed); - let mut tf = create_test_framework_with_orders(&mut rng, version); + let mut tf = create_test_framework_with_orders(&mut rng, OrdersVersion::V0); let (token_id, tokens_outpoint, mut coins_outpoint) = issue_and_mint_token_from_genesis(&mut rng, &mut tf); - let initial_ask_amount = Amount::from_atoms(1000); - let initial_give_amount = Amount::from_atoms(1000); + let initial_ask_amount = Amount::from_atoms(rng.gen_range(1000..2000)); + let initial_give_amount = Amount::from_atoms(rng.gen_range(1000..2000)); let order_data = OrderData::new( Destination::AnyoneCanSpend, OutputValue::Coin(initial_ask_amount), @@ -3340,13 +3305,15 @@ fn fill_order_destination_irrelevancy(#[case] seed: Seed, #[case] version: Order { let fill_amount1 = Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 10)); - let filled_amount1 = calculate_fill_order(&tf, &order_id, fill_amount1, version); - let fill_order_input1 = make_fill_order_input( - version, + let filled_amount1 = + calculate_fill_order(&tf, &order_id, fill_amount1, OrdersVersion::V0); + let fill_order_input1 = TxInput::AccountCommand( AccountNonce::new(0), - &order_id, - fill_amount1, - Destination::PublicKey(pk1.clone()), + AccountCommand::FillOrder( + order_id, + fill_amount1, + Destination::PublicKey(pk1.clone()), + ), ); let coins_left = tf.coin_amount_from_utxo(&coins_outpoint); @@ -3380,13 +3347,15 @@ fn fill_order_destination_irrelevancy(#[case] seed: Seed, #[case] version: Order { let fill_amount2 = Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 10)); - let filled_amount2 = calculate_fill_order(&tf, &order_id, fill_amount2, version); - let fill_order_input2 = make_fill_order_input( - version, + let filled_amount2 = + calculate_fill_order(&tf, &order_id, fill_amount2, OrdersVersion::V0); + let fill_order_input2 = TxInput::AccountCommand( AccountNonce::new(1), - &order_id, - fill_amount2, - Destination::PublicKey(pk1.clone()), + AccountCommand::FillOrder( + order_id, + fill_amount2, + Destination::PublicKey(pk1.clone()), + ), ); let coins_left = tf.coin_amount_from_utxo(&coins_outpoint); @@ -3439,13 +3408,15 @@ fn fill_order_destination_irrelevancy(#[case] seed: Seed, #[case] version: Order { let fill_amount3 = Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 10)); - let filled_amount3 = calculate_fill_order(&tf, &order_id, fill_amount3, version); - let fill_order_input3 = make_fill_order_input( - version, + let filled_amount3 = + calculate_fill_order(&tf, &order_id, fill_amount3, OrdersVersion::V0); + let fill_order_input3 = TxInput::AccountCommand( AccountNonce::new(2), - &order_id, - fill_amount3, - Destination::PublicKey(pk1), + AccountCommand::FillOrder( + order_id, + fill_amount3, + Destination::PublicKey(pk1.clone()), + ), ); let coins_left = tf.coin_amount_from_utxo(&coins_outpoint); @@ -3499,22 +3470,315 @@ fn fill_order_destination_irrelevancy(#[case] seed: Seed, #[case] version: Order }); } -fn make_fill_order_input( - orders_version: OrdersVersion, - nonce: AccountNonce, - order_id: &OrderId, - fill_amount: Amount, - destination: Destination, -) -> TxInput { - match orders_version { - OrdersVersion::V0 => TxInput::AccountCommand( - nonce, - AccountCommand::FillOrder(*order_id, fill_amount, destination), - ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - *order_id, - fill_amount, - destination, - )), - } +// In orders v1, the signature for a FillOrder input must be empty. +#[rstest] +#[trace] +#[case(Seed::from_entropy())] +fn fill_order_v1_must_not_be_signed(#[case] seed: Seed) { + utils::concurrency::model(move || { + let mut rng = make_seedable_rng(seed); + let mut tf = create_test_framework_with_orders(&mut rng, OrdersVersion::V1); + + let (token_id, tokens_outpoint, coins_outpoint) = + issue_and_mint_token_from_genesis(&mut rng, &mut tf); + let coins_utxo = tf.utxo(&coins_outpoint).take_output(); + + let initial_ask_amount = Amount::from_atoms(rng.gen_range(1000..2000)); + let initial_give_amount = Amount::from_atoms(rng.gen_range(1000..2000)); + let order_data = OrderData::new( + Destination::AnyoneCanSpend, + OutputValue::Coin(initial_ask_amount), + OutputValue::TokenV1(token_id, initial_give_amount), + ); + + let tx = TransactionBuilder::new() + .add_input(tokens_outpoint.into(), InputWitness::NoSignature(None)) + .add_output(TxOutput::CreateOrder(Box::new(order_data.clone()))) + .build(); + let order_id = make_order_id(tx.inputs()).unwrap(); + tf.make_block_builder().add_transaction(tx).build_and_process(&mut rng).unwrap(); + + let (sk, pk) = PrivateKey::new_from_rng(&mut rng, KeyKind::Secp256k1Schnorr); + let output_destination = Destination::PublicKey(pk); + + let fill_amount = + Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 10)); + let filled_amount = calculate_fill_order(&tf, &order_id, fill_amount, OrdersVersion::V1); + let fill_order_input = + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)); + let coins_left = tf.coin_amount_from_utxo(&coins_outpoint); + let change = (coins_left - fill_amount).unwrap(); + + let fill_tx = Transaction::new( + 0, + vec![coins_outpoint.clone().into(), fill_order_input], + vec![ + TxOutput::Transfer( + OutputValue::TokenV1(token_id, filled_amount), + output_destination.clone(), + ), + TxOutput::Transfer(OutputValue::Coin(change), Destination::AnyoneCanSpend), + ], + ) + .unwrap(); + + // The input is signed; this should be rejected with DestinationSigError::SignatureNotNeeded. + { + let input_commitments = [ + SighashInputCommitment::Utxo(Cow::Borrowed(&coins_utxo)), + SighashInputCommitment::None, + ]; + let fill_input_sig = StandardInputSignature::produce_uniparty_signature_for_input( + &sk, + Default::default(), + output_destination.clone(), + &fill_tx, + &input_commitments, + 0, + &mut rng, + ) + .unwrap(); + let fill_tx = SignedTransaction::new( + fill_tx.clone(), + vec![ + InputWitness::NoSignature(None), + InputWitness::Standard(fill_input_sig.clone()), + ], + ) + .unwrap(); + + let expected_input_check_err = InputCheckError::new( + 1, + InputCheckErrorPayload::Verification(ScriptError::Signature( + DestinationSigError::SignatureNotNeeded, + )), + ); + // First of all, verify the input via `verify_tx_signature`, this should fail with + // SignatureNotNeeded too. + let err = verify_tx_signature( + tf.chain_config(), + // Note: this destination will be ignored; mintscript should choose AnyoneCanSpend + // as the appropriate destination for FillOrder in orders v1. + &output_destination, + &fill_tx, + &input_commitments, + 1, + None, + ) + .unwrap_err(); + assert_eq!(err, expected_input_check_err); + + // As a sanity check, verify the actual signature via the lower-level `verify_signature` call. + let result = verify_signature( + tf.chain_config(), + &output_destination, + &fill_tx, + &EvaluatedInputWitness::Standard(fill_input_sig.clone()), + &input_commitments, + 1, + ); + assert_eq!(result, Ok(())); + + // Now try mining the tx, expecting SignatureNotNeeded. + let err = tf + .make_block_builder() + .add_transaction(fill_tx) + .build_and_process(&mut rng) + .unwrap_err(); + + let expected_err = ChainstateError::ProcessBlockError(BlockError::StateUpdateFailed( + ConnectTransactionError::InputCheck(expected_input_check_err), + )); + assert_eq!(err, expected_err); + } + + // The input is not signed. + { + let fill_tx = SignedTransaction::new( + fill_tx, + vec![InputWitness::NoSignature(None), InputWitness::NoSignature(None)], + ) + .unwrap(); + + tf.make_block_builder() + .add_transaction(fill_tx) + .build_and_process(&mut rng) + .unwrap(); + } + + let expected_ask_balance = (initial_ask_amount - fill_amount).unwrap(); + let expected_give_balance = (initial_give_amount - filled_amount).unwrap(); + + assert_eq!( + tf.chainstate.get_order_data(&order_id).unwrap(), + Some(order_data.into()), + ); + assert_eq!( + tf.chainstate.get_order_ask_balance(&order_id).unwrap(), + Some(expected_ask_balance), + ); + assert_eq!( + tf.chainstate.get_order_give_balance(&order_id).unwrap(), + Some(expected_give_balance), + ); + }); +} + +// Fill the same order twice using two different transactions in the same block. +// Note that we have 2 cases for each OrdersVersion - one where the fill amounts are different and +// another one where they are the same. The latter case is important in the v1 scenario, where +// it creates a block with 2 identical inputs among its transactions (which would normally be +// rejected with the DuplicateInputInBlock error, but v1 FillOrder inputs are an exception). +#[rstest] +#[trace] +#[case(Seed::from_entropy(), OrdersVersion::V0)] +#[case(Seed::from_entropy(), OrdersVersion::V1)] +fn fill_order_twice_in_same_block( + #[case] seed: Seed, + #[case] version: OrdersVersion, + #[values(false, true)] use_same_amount: bool, +) { + utils::concurrency::model(move || { + let mut rng = make_seedable_rng(seed); + let mut tf = create_test_framework_with_orders(&mut rng, version); + + let (token_id, tokens_outpoint, coins_outpoint) = + issue_and_mint_token_from_genesis(&mut rng, &mut tf); + let coins_amount = tf.coin_amount_from_utxo(&coins_outpoint); + + let initial_ask_amount = Amount::from_atoms(rng.gen_range(1000..2000)); + let initial_give_amount = Amount::from_atoms(rng.gen_range(1000..2000)); + let order_data = OrderData::new( + Destination::AnyoneCanSpend, + OutputValue::Coin(initial_ask_amount), + OutputValue::TokenV1(token_id, initial_give_amount), + ); + + let tx = TransactionBuilder::new() + .add_input(tokens_outpoint.into(), InputWitness::NoSignature(None)) + .add_output(TxOutput::CreateOrder(Box::new(order_data.clone()))) + .build(); + let order_id = make_order_id(tx.inputs()).unwrap(); + tf.make_block_builder().add_transaction(tx).build_and_process(&mut rng).unwrap(); + + let (_, pk) = PrivateKey::new_from_rng(&mut rng, KeyKind::Secp256k1Schnorr); + let output_destination = Destination::PublicKey(pk); + + let fill_amount1 = + Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 3)); + let fill_amount2 = if use_same_amount { + fill_amount1 + } else { + (|| { + for _ in 0..1000 { + let new_amount = + Amount::from_atoms(rng.gen_range(1..initial_ask_amount.into_atoms() / 3)); + if new_amount != fill_amount1 { + return new_amount; + } + } + panic!("Can't generate a distinct amount"); + })() + }; + + let filled_amount1 = calculate_fill_order(&tf, &order_id, fill_amount1, version); + let filled_amount2 = { + // Note: we can't use `calculate_fill_order` to calculate the second filled amount in orders v0, + // because it depends on the balances after the first fill, which hasn't been mined yet. + // So we have to use the low-level `calculate_filled_amount`. + let expected_ask_balance_after_first_fill = + (initial_ask_amount - fill_amount1).unwrap(); + let expected_give_balance_after_first_fill = + (initial_give_amount - filled_amount1).unwrap(); + + match version { + OrdersVersion::V0 => orders_accounting::calculate_filled_amount( + expected_ask_balance_after_first_fill, + expected_give_balance_after_first_fill, + fill_amount2, + ) + .unwrap(), + OrdersVersion::V1 => orders_accounting::calculate_filled_amount( + initial_ask_amount, + initial_give_amount, + fill_amount2, + ) + .unwrap(), + } + }; + + let fill_order_input1 = match version { + OrdersVersion::V0 => TxInput::AccountCommand( + AccountNonce::new(0), + AccountCommand::FillOrder(order_id, fill_amount1, Destination::AnyoneCanSpend), + ), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount1)) + } + }; + let fill_order_input2 = match version { + OrdersVersion::V0 => TxInput::AccountCommand( + AccountNonce::new(1), + AccountCommand::FillOrder(order_id, fill_amount2, Destination::AnyoneCanSpend), + ), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount2)) + } + }; + + let coins_after_first_fill = (coins_amount - fill_amount1).unwrap(); + let fill_tx_1 = TransactionBuilder::new() + .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) + .add_input(fill_order_input1, InputWitness::NoSignature(None)) + .add_output(TxOutput::Transfer( + OutputValue::TokenV1(token_id, filled_amount1), + output_destination.clone(), + )) + .add_output(TxOutput::Transfer( + OutputValue::Coin(coins_after_first_fill), + Destination::AnyoneCanSpend, + )) + .build(); + let fill_tx_1_id = fill_tx_1.transaction().get_id(); + + let coins_outpoint = UtxoOutPoint::new(fill_tx_1_id.into(), 1); + + let coins_after_second_fill = (coins_after_first_fill - fill_amount2).unwrap(); + let fill_tx_2 = TransactionBuilder::new() + .add_input(coins_outpoint.into(), InputWitness::NoSignature(None)) + .add_input(fill_order_input2, InputWitness::NoSignature(None)) + .add_output(TxOutput::Transfer( + OutputValue::TokenV1(token_id, filled_amount2), + output_destination.clone(), + )) + .add_output(TxOutput::Transfer( + OutputValue::Coin(coins_after_second_fill), + Destination::AnyoneCanSpend, + )) + .build(); + + tf.make_block_builder() + .add_transaction(fill_tx_1) + .add_transaction(fill_tx_2) + .build_and_process(&mut rng) + .unwrap(); + + let total_fill_amount = (fill_amount1 + fill_amount2).unwrap(); + let total_filled_amount = (filled_amount1 + filled_amount2).unwrap(); + let expected_ask_balance = (initial_ask_amount - total_fill_amount).unwrap(); + let expected_give_balance = (initial_give_amount - total_filled_amount).unwrap(); + + assert_eq!( + tf.chainstate.get_order_data(&order_id).unwrap(), + Some(order_data.into()), + ); + assert_eq!( + tf.chainstate.get_order_ask_balance(&order_id).unwrap(), + Some(expected_ask_balance), + ); + assert_eq!( + tf.chainstate.get_order_give_balance(&order_id).unwrap(), + Some(expected_give_balance), + ); + }); } diff --git a/chainstate/tx-verifier/src/transaction_verifier/check_transaction.rs b/chainstate/tx-verifier/src/transaction_verifier/check_transaction.rs index 2457a603c2..d4ade5d00b 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/check_transaction.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/check_transaction.rs @@ -414,7 +414,7 @@ fn check_order_inputs_outputs( OrdersVersion::V1 => {} }; match cmd { - common::chain::OrderAccountCommand::FillOrder(id, fill, _) => { + common::chain::OrderAccountCommand::FillOrder(id, fill) => { // Forbidding fills with zero amount ensures that tx has utxo and therefore is unique. // Unique txs cannot be replayed. ensure!( diff --git a/chainstate/tx-verifier/src/transaction_verifier/input_check/signature_only_check.rs b/chainstate/tx-verifier/src/transaction_verifier/input_check/signature_only_check.rs index 182051af18..4fb1c540b2 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/input_check/signature_only_check.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/input_check/signature_only_check.rs @@ -105,6 +105,13 @@ impl InputInfoProvider for InputVerifyContextSignature<'_, T> { pub trait SignatureOnlyVerifiable {} impl SignatureOnlyVerifiable for SignedTransaction {} +// Note: the passed `outpoint_destination` value is only used in a limited number of scenarios +// (see `impl SignatureInfoProvider for InputVerifyContextSignature` above). In all other cases +// this parameter is ignored and the actual destination to verify the signature against is taken +// from other sources (e.g. from the utxo or, in the case of a v1 FillOrder input, it is always +// AnyoneCanSpend). +// TODO: the parameter should at least be made optional. Or maybe some kind of `SignatureInfoProvider` +// should be passed here instead of the plain `Destination`. pub fn verify_tx_signature( chain_config: &ChainConfig, outpoint_destination: &Destination, diff --git a/chainstate/tx-verifier/src/transaction_verifier/mod.rs b/chainstate/tx-verifier/src/transaction_verifier/mod.rs index 6bd1c8fbf6..bd2c8bf138 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/mod.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/mod.rs @@ -774,7 +774,7 @@ where } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::ConcludeOrder(order_id) => { check_order_doesnt_use_frozen_token(order_id) } @@ -857,7 +857,7 @@ where } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, fill, _) => { + OrderAccountCommand::FillOrder(order_id, fill) => { let res = self .orders_accounting_cache .fill_order(*order_id, *fill, OrdersVersion::V1) diff --git a/common/src/chain/transaction/account_outpoint.rs b/common/src/chain/transaction/account_outpoint.rs index 4f13d9088a..4621832511 100644 --- a/common/src/chain/transaction/account_outpoint.rs +++ b/common/src/chain/transaction/account_outpoint.rs @@ -64,7 +64,7 @@ impl From<&AccountCommand> for AccountType { impl From for AccountType { fn from(cmd: OrderAccountCommand) -> Self { match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => AccountType::Order(order_id), } @@ -141,11 +141,10 @@ pub enum AccountCommand { ConcludeOrder(OrderId), // Satisfy an order completely or partially. - // The second parameter is the fill amount in the order's "ask" currency. - // The third parameter is an arbitrary destination, whose purpose is to make sure that all - // "FillOrder" inputs in a block are distinct, so that the same order can be filled in the same - // block multiple times (note: all transaction inputs in a block must be distinct, - // see CheckBlockTransactionsError::DuplicateInputInBlock). + // The second element is the fill amount in the order's "ask" currency. + // The third element is an arbitrary destination, which is present here due to historical reasons. + // (Though it can technically be the same as the actual output destination, this is not enforced). + // // Also note that though a FillOrder input can technically have a signature, it is not checked. // So it's better not to provide one, to reduce the transaction size and avoid needlessly exposing // the corresponding public key. @@ -207,16 +206,10 @@ impl AccountOutPoint { )] pub enum OrderAccountCommand { // Satisfy an order completely or partially. - // The second parameter is the fill amount in the order's "ask" currency. - // The third parameter is an arbitrary destination, whose purpose is to make sure that all - // "FillOrder" inputs in a block are distinct, so that the same order can be filled in the same - // block multiple times (note: all transaction inputs in a block must be distinct, - // see CheckBlockTransactionsError::DuplicateInputInBlock). - // Also note that though a FillOrder input can technically have a signature, it is not checked. - // So it's better not to provide one, to reduce the transaction size and avoid needlessly exposing - // the corresponding public key. + // The second element is the fill amount in the order's "ask" currency. + // Note that in orders v1 there is no `Destination` element inside `FillOrder`. #[codec(index = 0)] - FillOrder(OrderId, Amount, Destination), + FillOrder(OrderId, Amount), // Freeze an order which effectively forbids any fill operations. // Frozen order can only be concluded. diff --git a/common/src/chain/transaction/signature/mod.rs b/common/src/chain/transaction/signature/mod.rs index d6ad615399..8193a1a050 100644 --- a/common/src/chain/transaction/signature/mod.rs +++ b/common/src/chain/transaction/signature/mod.rs @@ -67,6 +67,8 @@ pub enum DestinationSigError { InvalidSignatureEncoding, #[error("No signature!")] SignatureNotFound, + #[error("Signature present where it's not needed")] + SignatureNotNeeded, #[error("Producing signature failed!")] ProducingSignatureFailed(crypto::key::SignatureError), #[error("Private key does not match with spender public key")] @@ -138,22 +140,28 @@ pub fn verify_signature( DestinationSigError::InvalidSignatureIndex(input_index, inputs.len(),) ); - match input_witness { - EvaluatedInputWitness::NoSignature(_) => match outpoint_destination { - Destination::PublicKeyHash(_) - | Destination::PublicKey(_) - | Destination::ScriptHash(_) - | Destination::ClassicMultisig(_) => { + match outpoint_destination { + Destination::PublicKeyHash(_) + | Destination::PublicKey(_) + | Destination::ScriptHash(_) + | Destination::ClassicMultisig(_) => match input_witness { + EvaluatedInputWitness::NoSignature(_) => { return Err(DestinationSigError::SignatureNotFound) } - Destination::AnyoneCanSpend => {} + EvaluatedInputWitness::Standard(witness) => { + let sighash = + signature_hash(witness.sighash_type(), tx, input_commitments, input_index)?; + witness.verify_signature(chain_config, outpoint_destination, &sighash)?; + } + }, + Destination::AnyoneCanSpend => match input_witness { + EvaluatedInputWitness::NoSignature(_) => {} + EvaluatedInputWitness::Standard(_) => { + return Err(DestinationSigError::SignatureNotNeeded) + } }, - EvaluatedInputWitness::Standard(witness) => { - let sighash = - signature_hash(witness.sighash_type(), tx, input_commitments, input_index)?; - witness.verify_signature(chain_config, outpoint_destination, &sighash)?; - } } + Ok(()) } diff --git a/common/src/chain/transaction/signature/sighash/input_commitments/mod.rs b/common/src/chain/transaction/signature/sighash/input_commitments/mod.rs index 5cb4de58fc..53f5753f0b 100644 --- a/common/src/chain/transaction/signature/sighash/input_commitments/mod.rs +++ b/common/src/chain/transaction/signature/sighash/input_commitments/mod.rs @@ -382,7 +382,7 @@ where | AccountCommand::ChangeTokenMetadataUri(_, _) => Ok(SighashInputCommitment::None), }, | TxInput::OrderAccountCommand(command) => match command { - OrderAccountCommand::FillOrder(order_id, _, _) => { + OrderAccountCommand::FillOrder(order_id, _) => { let order_info = order_info_getter(order_id, input_idx)?; Ok(SighashInputCommitment::FillOrderAccountCommand { diff --git a/common/src/chain/transaction/signature/tests/sign_and_mutate.rs b/common/src/chain/transaction/signature/tests/sign_and_mutate.rs index 2dca6eee9d..f47dce6ebb 100644 --- a/common/src/chain/transaction/signature/tests/sign_and_mutate.rs +++ b/common/src/chain/transaction/signature/tests/sign_and_mutate.rs @@ -924,13 +924,9 @@ fn mutate_first_input( } } TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(id, amount, destination) => { - TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - *id, - Amount::from_atoms(amount.into_atoms() + 1), - destination.clone(), - )) - } + OrderAccountCommand::FillOrder(id, amount) => TxInput::OrderAccountCommand( + OrderAccountCommand::FillOrder(*id, Amount::from_atoms(amount.into_atoms() + 1)), + ), OrderAccountCommand::ConcludeOrder(order_id) => { TxInput::OrderAccountCommand(OrderAccountCommand::ConcludeOrder(*order_id)) } diff --git a/mintscript/src/tests/translate/mod.rs b/mintscript/src/tests/translate/mod.rs index 9a97b566b0..96cbc666e3 100644 --- a/mintscript/src/tests/translate/mod.rs +++ b/mintscript/src/tests/translate/mod.rs @@ -24,7 +24,7 @@ use common::{ htlc::{HashedTimelockContract, HtlcSecret, HtlcSecretHash}, signature::inputsig::authorize_hashed_timelock_contract_spend::AuthorizedHashedTimelockContractSpend, stakelock::StakePoolData, - tokens, AccountNonce, AccountSpending, OrderData, OrderId, + tokens, AccountNonce, AccountSpending, OrderAccountCommand, OrderData, OrderId, }, primitives::per_thousand::PerThousand, }; @@ -57,6 +57,9 @@ enum TestInputInfo { AccountCommand { command: AccountCommand, }, + OrderAccountCommand { + command: OrderAccountCommand, + }, } impl TestInputInfo { @@ -73,6 +76,7 @@ impl TestInputInfo { }, Self::Account { outpoint } => InputInfo::Account { outpoint }, Self::AccountCommand { command } => InputInfo::AccountCommand { command }, + Self::OrderAccountCommand { command } => InputInfo::OrderAccountCommand { command }, } } } @@ -261,16 +265,31 @@ fn mint(id: TokenId, amount: u128) -> TestInputInfo { TestInputInfo::AccountCommand { command } } -fn conclude_order(id: OrderId) -> TestInputInfo { +fn conclude_order_v0(id: OrderId) -> TestInputInfo { let command = AccountCommand::ConcludeOrder(id); TestInputInfo::AccountCommand { command } } -fn fill_order(id: OrderId) -> TestInputInfo { +fn fill_order_v0(id: OrderId) -> TestInputInfo { let command = AccountCommand::FillOrder(id, Amount::from_atoms(1), dest_pk(0x4)); TestInputInfo::AccountCommand { command } } +fn conclude_order_v1(id: OrderId) -> TestInputInfo { + let command = OrderAccountCommand::ConcludeOrder(id); + TestInputInfo::OrderAccountCommand { command } +} + +fn fill_order_v1(id: OrderId) -> TestInputInfo { + let command = OrderAccountCommand::FillOrder(id, Amount::from_atoms(1)); + TestInputInfo::OrderAccountCommand { command } +} + +fn freeze_order(id: OrderId) -> TestInputInfo { + let command = OrderAccountCommand::FreezeOrder(id); + TestInputInfo::OrderAccountCommand { command } +} + #[derive( Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, strum::EnumIter, derive_more::Display, )] @@ -553,49 +572,120 @@ enum Mode { (Mode::TxTimelockOnly, "ERROR: Attempt to spend an unspendable output"), (Mode::TxFull, "ERROR: Attempt to spend an unspendable output"), ])] -#[case(conclude_order(order0().0), nosig(), &[ +// Conclude order v0 +#[case(conclude_order_v0(order0().0), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), +])] +#[case(conclude_order_v0(fake_id(0x88)), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "ERROR: Order with id 8888…8888 does not exist"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "ERROR: Order with id 8888…8888 does not exist"), +])] +#[case(conclude_order_v0(order0().0), stdsig(0x44), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), +])] +#[case(conclude_order_v0(order0().0), stdsig(0x45), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), +])] +// Conclude order v1 +#[case(conclude_order_v1(order0().0), nosig(), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), ])] -#[case(conclude_order(fake_id(0x88)), nosig(), &[ +#[case(conclude_order_v1(fake_id(0x88)), nosig(), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "ERROR: Order with id 8888…8888 does not exist"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "ERROR: Order with id 8888…8888 does not exist"), ])] -#[case(conclude_order(order0().0), stdsig(0x44), &[ +#[case(conclude_order_v1(order0().0), stdsig(0x44), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), ])] -#[case(conclude_order(order0().0), stdsig(0x45), &[ +#[case(conclude_order_v1(order0().0), stdsig(0x45), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), ])] -#[case(fill_order(order0().0), nosig(), &[ +// Fill order v0 +#[case(fill_order_v0(order0().0), nosig(), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "true"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "true"), ])] -#[case(fill_order(fake_id(0x77)), nosig(), &[ +#[case(fill_order_v0(fake_id(0x77)), nosig(), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "true"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "true"), ])] -#[case(fill_order(order0().0), stdsig(0x45), &[ +#[case(fill_order_v0(order0().0), stdsig(0x45), &[ (Mode::Reward, "ERROR: Illegal account spend"), (Mode::TxSigOnly, "true"), (Mode::TxTimelockOnly, "true"), (Mode::TxFull, "true"), ])] -fn translate_snap( +// Fill order v1 +#[case(fill_order_v1(order0().0), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x00, 0x0000)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x00, 0x0000)"), +])] +#[case(fill_order_v1(fake_id(0x77)), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x00, 0x0000)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x00, 0x0000)"), +])] +#[case(fill_order_v1(order0().0), stdsig(0x45), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x00, 0x0101084545)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x00, 0x0101084545)"), +])] +// Freeze order +#[case(freeze_order(order0().0), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0000)"), +])] +#[case(freeze_order(fake_id(0x88)), nosig(), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "ERROR: Order with id 8888…8888 does not exist"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "ERROR: Order with id 8888…8888 does not exist"), +])] +#[case(freeze_order(order0().0), stdsig(0x44), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084444)"), +])] +#[case(freeze_order(order0().0), stdsig(0x45), &[ + (Mode::Reward, "ERROR: Illegal account spend"), + (Mode::TxSigOnly, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), + (Mode::TxTimelockOnly, "true"), + (Mode::TxFull, "signature(0x02000236d8c927b785e27385737e82cdde2e06dc510ab8545d6eab0ca05c36040a437c, 0x0101084545)"), +])] +fn translate( #[case] test_input_info: TestInputInfo, #[case] witness: InputWitness, #[case] expected_results: &[(Mode, &str)], diff --git a/mintscript/src/translate.rs b/mintscript/src/translate.rs index 9479fa9217..c7c3bf1591 100644 --- a/mintscript/src/translate.rs +++ b/mintscript/src/translate.rs @@ -107,6 +107,7 @@ pub trait SignatureInfoProvider: InputInfoProvider { token_id: &TokenId, ) -> Result, tokens_accounting::Error>; + // Note: this is used for FreezeOrder too. fn get_orders_conclude_destination( &self, order_id: &OrderId, @@ -229,10 +230,20 @@ impl TranslateInput for SignedTransaction { .ok_or(TranslationError::OrderNotFound(*order_id))?; Ok(to_signature_witness_script(ctx, &dest)) } - AccountCommand::FillOrder(_, _, _) => Ok(WitnessScript::TRUE), + AccountCommand::FillOrder(_, _, _) => { + // In orders V0, FillOrder inputs can have arbitrary signatures. + Ok(WitnessScript::TRUE) + } }, InputInfo::OrderAccountCommand { command } => match command { - OrderAccountCommand::FillOrder(_, _, _) => Ok(WitnessScript::TRUE), + OrderAccountCommand::FillOrder(_, _) => { + // In orders V1, FillOrder inputs must not be signed. + // Note: Destination::AnyoneCanSpend requires InputWitness::NoSignature. + Ok(to_signature_witness_script( + ctx, + &Destination::AnyoneCanSpend, + )) + } OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { let dest = ctx @@ -450,10 +461,20 @@ impl TranslateInput for SignatureOnlyTx { .ok_or(TranslationError::OrderNotFound(*order_id))?; Ok(to_signature_witness_script(ctx, &dest)) } - AccountCommand::FillOrder(_, _, _) => Ok(WitnessScript::TRUE), + AccountCommand::FillOrder(_, _, _) => { + // In orders V0, FillOrder inputs can have arbitrary signatures. + Ok(WitnessScript::TRUE) + } }, InputInfo::OrderAccountCommand { command } => match command { - OrderAccountCommand::FillOrder(_, _, _) => Ok(WitnessScript::TRUE), + OrderAccountCommand::FillOrder(_, _) => { + // In orders V1, FillOrder inputs must not be signed. + // Note: Destination::AnyoneCanSpend requires InputWitness::NoSignature. + Ok(to_signature_witness_script( + ctx, + &Destination::AnyoneCanSpend, + )) + } OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { let dest = ctx diff --git a/p2p/src/sync/tests/no_discouragement_after_tx_reorg.rs b/p2p/src/sync/tests/no_discouragement_after_tx_reorg.rs index f837bfdac0..471be70d98 100644 --- a/p2p/src/sync/tests/no_discouragement_after_tx_reorg.rs +++ b/p2p/src/sync/tests/no_discouragement_after_tx_reorg.rs @@ -859,7 +859,6 @@ impl TestFixture { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, coin_amount_to_fill, - Destination::AnyoneCanSpend, )), InputWitness::NoSignature(None), ) diff --git a/test/functional/wallet_order_double_fill_with_same_dest_impl.py b/test/functional/wallet_order_double_fill_with_same_dest_impl.py index e618e75892..4e1e738b9c 100644 --- a/test/functional/wallet_order_double_fill_with_same_dest_impl.py +++ b/test/functional/wallet_order_double_fill_with_same_dest_impl.py @@ -14,12 +14,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -""" Try submitting two FillOrder transactions using the same "destination", without mining a block in between. +""" Try submitting two transactions with "similar" FillOrder inputs (i.e. using the same amount +and, in the case of orders v0, the same destination), without mining a block in between. Note: the exact result differs depending on the orders version; the purpose of the test is to -prove that nothing bad can happen as a result (e.g. in orders v1, where orders don't use nonces, using -the same destination and the same amount results in exactly the same FillOrder input being produced; -if both of the txs are included in the same block, the block will be invalid). +prove that nothing bad can happen as a result (e.g. in orders v1, where orders don't use nonces, +using the same amount results in exactly the same FillOrder input being produced; under older +consensus rules, if both of the txs are included in the same block, the block would be invalid). """ from test_framework.test_framework import BitcoinTestFramework diff --git a/trezor-common/src/lib.rs b/trezor-common/src/lib.rs index 62f1eb8aef..6eae91d792 100644 --- a/trezor-common/src/lib.rs +++ b/trezor-common/src/lib.rs @@ -397,9 +397,9 @@ pub enum AccountCommand { #[strum_discriminants(name(OrderAccountCommandTag), derive(EnumIter))] pub enum OrderAccountCommand { // Satisfy an order completely or partially. - // Second parameter is an amount provided to fill an order which corresponds to order's ask currency. + // The second element is the fill amount in the order's "ask" currency. #[codec(index = 0)] - FillOrder(OrderId, Amount, Destination), + FillOrder(OrderId, Amount), // Freeze an order which effectively forbids any fill operations. // Frozen order can only be concluded. // Only the address specified as `conclude_key` can authorize this command. diff --git a/trezor-common/src/tests/mod.rs b/trezor-common/src/tests/mod.rs index df9d301a6e..e04353341d 100644 --- a/trezor-common/src/tests/mod.rs +++ b/trezor-common/src/tests/mod.rs @@ -149,8 +149,8 @@ impl From for crate::AccountCommand { impl From for crate::OrderAccountCommand { fn from(value: chain::OrderAccountCommand) -> Self { match value { - chain::OrderAccountCommand::FillOrder(order_id, amount, destination) => { - Self::FillOrder(order_id.to_hash().into(), amount.into(), destination.into()) + chain::OrderAccountCommand::FillOrder(order_id, amount) => { + Self::FillOrder(order_id.to_hash().into(), amount.into()) } chain::OrderAccountCommand::FreezeOrder(order_id) => { Self::FreezeOrder(order_id.to_hash().into()) diff --git a/wallet/src/account/mod.rs b/wallet/src/account/mod.rs index 741fd9bf15..b217c4ed34 100644 --- a/wallet/src/account/mod.rs +++ b/wallet/src/account/mod.rs @@ -1108,10 +1108,15 @@ impl Account { AccountCommand::FillOrder( order_id, fill_amount_in_ask_currency, - output_destination.clone(), + // Note: this destination is only present here due to historical reasons + // and can be arbitrary. + Destination::AnyoneCanSpend, ), ), - output_destination, + // In orders v0, FillOrder input signatures are not checked. It's still better to + // use AnyoneCanSpend here instead of output_destination, to avoid revealing the + // public key associated with the destination (which is normally PublicKeyHash). + Destination::AnyoneCanSpend, )]) } common::chain::OrdersVersion::V1 => { @@ -1131,9 +1136,9 @@ impl Account { TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( order_id, fill_amount_in_ask_currency, - output_destination.clone(), )), - output_destination, + // In orders v1, FillOrder inputs are required not to have signatures. + Destination::AnyoneCanSpend, )]) } }; @@ -1594,7 +1599,8 @@ impl Account { cmd: &OrderAccountCommand, ) -> WalletResult { match cmd { - OrderAccountCommand::FillOrder(_, _, destination) => Ok(destination.clone()), + // In orders v1 FillOrder inputs must not be signed. + OrderAccountCommand::FillOrder(_, _) => Ok(Destination::AnyoneCanSpend), OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => self .output_cache @@ -2062,9 +2068,7 @@ impl Account { } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, dest) => { - self.find_order(order_id).is_ok() || self.is_destination_mine_or_watched(dest) - } + OrderAccountCommand::FillOrder(order_id, _) => self.find_order(order_id).is_ok(), OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => self.find_order(order_id).is_ok(), }, @@ -2082,9 +2086,8 @@ impl Account { || self.is_destination_mine_or_watched(address) } AccountCommand::ConcludeOrder(order_id) => self.find_order(order_id).is_ok(), - AccountCommand::FillOrder(order_id, _, dest) => { - self.find_order(order_id).is_ok() || self.is_destination_mine_or_watched(dest) - } + // Note: the destination in v0 FillOrder can be arbitrary, so we ignore it. + AccountCommand::FillOrder(order_id, _, _) => self.find_order(order_id).is_ok(), }, }); let relevant_outputs = self.mark_outputs_as_seen(db_tx, tx.outputs())?; @@ -2685,7 +2688,7 @@ fn group_preselected_inputs( } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(id, fill_amount_in_ask_currency, _) => { + OrderAccountCommand::FillOrder(id, fill_amount_in_ask_currency) => { handle_fill_order_op( *id, *fill_amount_in_ask_currency, diff --git a/wallet/src/account/output_cache/mod.rs b/wallet/src/account/output_cache/mod.rs index 6e306a043a..61e772c2b1 100644 --- a/wallet/src/account/output_cache/mod.rs +++ b/wallet/src/account/output_cache/mod.rs @@ -924,7 +924,7 @@ impl OutputCache { } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::ConcludeOrder(order_id) => { self.order_data(order_id).is_some_and(|data| { [data.ask_currency, data.give_currency].iter().any(|v| match v { @@ -1182,7 +1182,7 @@ impl OutputCache { } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { if !already_present { @@ -1573,7 +1573,7 @@ impl OutputCache { } }, TxInput::OrderAccountCommand(cmd) => match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { if let Some(data) = self.orders.get_mut(order_id) { diff --git a/wallet/src/signer/tests/generic_fixed_signature_tests.rs b/wallet/src/signer/tests/generic_fixed_signature_tests.rs index d063227ded..2353fa6f38 100644 --- a/wallet/src/signer/tests/generic_fixed_signature_tests.rs +++ b/wallet/src/signer/tests/generic_fixed_signature_tests.rs @@ -765,13 +765,13 @@ pub fn test_fixed_signatures_generic2( TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( filled_order_v1_id, Amount::from_atoms(100), - Destination::AnyoneCanSpend, )), ]; - let acc_dests: Vec = acc_inputs - .iter() + // Note: the last input is v1 FillOrder, which must not be signed. + let acc_dests = (0..acc_inputs.len() - 1) .map(|_| new_dest_from_account(&mut account1, &mut db_tx, KeyPurpose::ReceiveFunds)) - .collect(); + .chain(std::iter::once(Destination::AnyoneCanSpend)) + .collect_vec(); let pool_data = StakePoolData::new( Amount::from_atoms(1000), @@ -916,13 +916,7 @@ pub fn test_fixed_signatures_generic2( assert!(ptx.all_signatures_available()); for (i, dest) in destinations.iter().enumerate() { - let raw_sig = assert_matches_return_val!( - ptx.witnesses()[i].as_ref().unwrap(), - InputWitness::Standard(sig), - sig.raw_signature() - ); - - log_signature(i, raw_sig); + log_witness(i, ptx.witnesses()[i].as_ref().unwrap()); tx_verifier::input_check::signature_only_check::verify_tx_signature( &chain_config, @@ -939,16 +933,16 @@ pub fn test_fixed_signatures_generic2( SighashInputCommitmentVersion::V0 => { let htlc_multisig = { let sigs_hex = [ - (0, "32eaee75673ebe681c3d8a44e9ae7310f93f99c5021cb5746f64d26d12e22b59a8517943f828c9bff06521bd0a421552f320aae1bd04bbc731629c350289d5eb"), - (1, "43786e4d3def2f1f1dd5e1afa9489f6e79c7eb4b0dbc09456c9e75f6dac7f2313c18663895ecc3b76141cc58b3e3eab3e927cf43b56c75d6313ca5c8ff6626c2"), + (0, "0ff63e871249f4b6d8de07216617054c71a4b341e11afd405712cc108cb8e315f4b577444a29a81126aab9b7ad426594c2b39e5bc2e11169b1c64e62524620fe"), + (1, "cd190e9a746aefb9ccec04a178017db55154f6370993139819ab270e988ff2c510b5bbcd95da31dff124f22b323bc3547ec15045e92552f7a951433c064d07f0"), ]; make_htlc_multisig_spend_sig(htlc_multisig_challenge, sigs_hex) }; let multisig = { let sigs_hex = [ - (1, "8f15883ae42988b3e1e4d68183a92f9a7102e2f2abf4b17474881eb2630f87dfc5f050dea079440a14be325211195cf58dc3681e7c5665892d433daa9ea935ff"), - (2, "bc8e6694b066f64003ad92f894ee560de9f51aa8253a8e336188db7fd36fa992923bf948490144c7b74533d56efa699db547fd5605aaf8eeb8a5cbda02d5c8c8"), - (3, "3945b0d96a08b0bb6994d3d722d8359f34761a4407fc6f2dd03734a8065a3ba246deea95fa78411ef23c2d05f0efcefb1984f9e2c38c9e11b6dff410cb72eb80") + (1, "e94989eddcbb19eb87a02935a6b9e09e0cb0537d5aadf2d8bb37872949176820970c6d69320712be192805ad4609020237e46e461c0aeb42712f5feaceee5fd8"), + (2, "90da875f04bc2db203e5a4c322514bb17d01870a34f40bba5922439580db156c4d9b455ce2b2ceb8e070541fb7a503752dfbc670ac32c858c462044e1ac8e421"), + (3, "1ae75282887fcdca2f9cccbca269db14d368fd29658b92e32af98c9488ab6745cfd54dacafc91fcf63dbce69d196716345fd0d9cbcff25ef251286ab1bea025e") ]; make_multisig_spend_sig(multisig_challenge, sigs_hex) }; @@ -956,88 +950,85 @@ pub fn test_fixed_signatures_generic2( vec![ Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&account1_dest4, &account1), - "aa01ee31f91af118a2ceebb3995dbf5d8755f557b11fa5a68af9aa7da3998a1ee4ed41ea4f67c3658d51fbcbe7b24b65b8a99648d1dc20a3035541cd45e66a59", + "dcc2f6ee68a13910e01a087946bc8200ac2fc0e229c756fced7e35f398b48c9c6e50987ea5990db911a812b90684597f640055cfc3407e130253216fe12c1d39", ))), Some(InputWitness::Standard(make_pub_key_spend_sig( - "bc8e6694b066f64003ad92f894ee560de9f51aa8253a8e336188db7fd36fa992923bf948490144c7b74533d56efa699db547fd5605aaf8eeb8a5cbda02d5c8c8", + "90da875f04bc2db203e5a4c322514bb17d01870a34f40bba5922439580db156c4d9b455ce2b2ceb8e070541fb7a503752dfbc670ac32c858c462044e1ac8e421", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&decommission_dest, &account1), - "ee4b5a0febe45ebe8ce4ece0387111f7d53bb8acf7ce1635fcc13c41f6ff6b632ea434e8031781e53731839c782f33e87e06599a9e37d0f3aed63964635c1018", + "60ded17d5efd92ae32a8020bd0fe74aab43e095cb429a86e8b36e91fffa3fc04624e6c57663ab343da3ba6cc3853c4c8e26c0cf36b2b3fd96ba8b9afd6330260", ))), Some(InputWitness::Standard(make_htlc_secret_spend_sig( htlc_secret, - "0094a8bde764e97d9534d08c22cf8aa762e44338830ea058604d89b76c33c9b6dc8fd7f1f2e2d818841c10d47e8d504b9fc251a1d2ddb13eaeb7c045ea03727d8f", + "00765a0a326c9b0e7a4a7f890f6548df021163d25b034d568dbc5354eb11c40b567d0fa9bee9111eb6895d45fe4dec9e96f7bc8da5388a747ed1354b7425780abf", ))), Some(InputWitness::Standard(htlc_multisig)), Some(InputWitness::Standard(multisig)), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[0], &account1), - "5537743c018289f74aad5cc3672d14a502c31641fb8244f41e8412eac10128e8f5261c43b4f3615d20e97a99ab69f99625fa6adf3e9eeec890f4721044dbeb8b", + "89fc1b6acfc26c2abd019734fbc714c1a3f93157f0799fbc019eb64075130b5fa9ce675ba1b5fa6c80a3eef4d4cc01d2460e60ea996ab2acf158d3df9dc4c1b7", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[1], &account1), - "1b5f79c97ae5ab0717fa5460331271a328b8773735fd146082d6aab86023e94fd51613fcd6c76ab3e64a05548ae71a45920ba317cba8b04917f2b6ef38a1f72a", + "5628b5eb690254a711fd93824f177663d540365d26acf32f566cabddadbd4bd1fd1c48a4215448c22ff853f4e960fd46742ab82c50c68dd1a41a1e93ab03dd95", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[2], &account1), - "315d46daa4fb008207d7888c406c65aaca86fe708db505227cd13e4cea968034ce961432c8747f51a5f47139571f8b72c0905ac0b9834c030019ad29b9c77995", + "5f9c58fa90d14982fbe1e69ff91a21f60f7b8a7f069733baae744502d84e0d82e983558bcfaf9c8e77576a696784c1e71e87329cb132fde67316694c8cca84d7", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[3], &account1), - "b283bf358bfd9ea494f46c1b50485a2bd76aba5db918fea738b5f86ee950e95359dcbce5c1db7eab483e15788abb8f921c0bf9d83e3f0ea40bc4a0b5b2003378", + "bf61b338c7a59764b1ffbdf5751a6e017d1600eb70d89ab46114dd5ba130188847b7a3b0d67278ec33c5499ad5958c89245a2d76a6d580f343505bf03da2f10a", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[4], &account1), - "46ccf257425b3fce609d99062152166bb41f1dca503e46aa989146fbfccdb92c83fe90c5d232f934429fb4aa2c9c529412b7c1a678a736fe9c8b183de6e9dd59", + "04c102210c9bc1467c97aac2930946c0ece292a7ee040dfe3fad1212c7d139ecef437b09f0f97cee908981dfd6898a375d735e77fef4bc32692e861a2e16825a", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[5], &account1), - "66cb7bc3b42f40a08e15dd4539201da316b0ccd12594b0ef62cd5a0480f410a4791a5e928685ca764a9ed728bf960ad1b3ccce15cdd4e9369abe24853b6f395c", + "26ef8f25151bb1f7d098a9319bb18f1e54c64b7b8f80dc0eb6401ff924d5e6a9f4d343e808d56f6eead674b6a953a901f03615f592b2b11ee084d126ce533eea", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[6], &account1), - "ec29fa50efd02cf9bcceb9c43e01fea0ea16df2770aeced93cc1594fa8b6aeaca6e42cdb223a3dbd5daf978c4f716437ebe51bd079b5ea681c9587ec1ac1fa93", + "4647a7ffb7fd7dd7bac96a49ba7595b2ebf6c39484a70f56b5acc9cd7eb70e845bbf8ab5c11844d3188d6ad8320c793eeeb7125b71a5fa82580c538e5b379596", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[7], &account1), - "a7fbcc42b4d2af00cc2a1636aa89ba66a8d89008dcf0e0f830ccd6aa0340d638b930b07e0babcf93da0d1b7302445998779b5af11125f2e15ffe1a4f43dc901d", + "d835087b24f194ec8aa45b2174ee625f5c5e5be52370d828f8f01024ec25226f268682bedca4dee966ab99b4d790c694ba15ca26dac91733735fd9efc5c0ac39", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[8], &account1), - "0bd5c0fb6d8c5d0ad7d7883bdc9b9f03b147b9d6c540c949ac544ee326e8137532ca7eb1e2256952d200b5228c630d10b60a4d2b1d37a0d1408865a3188deb65", + "f412f8620df1ef92ca245df21ec0690a4f231c6ee11853cb67579f67a6782b03ebe6b9563a47c7552417fad354f5f9965cd522dd38fee36da61bf12d30a15484", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[9], &account1), - "47c5dfc79c953a672d7b4f74a2b60cf56c7d74253803e4629dfe80458cb0758b54172823a97486d63611f1d26fcb672e13d25c3fc42834b040d1d41460fd6f78", + "a0273f3af5cdfce947908ada352d8a57228bcaa9d167868190ad6c034d86cb43d7d29e2db14eb786243db1fe6573f4d0c88cf0e1fe88a974c8981bd05928d6d3", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[10], &account1), - "9ba0d5070c0ba2f35cd89d344b30b681eb5a69d5668216865189e84a15b8fc50fa49ace13a1702761d3a4632c04b5ef00088b0052250ff2f0e4fb51db56e295e", + "3ae6e814d6d3b7072575b1a1e69ec32612ede6a79a712a575eed8b76e3c16fbf410563d7cfc22df9b2f09417bbd3e3f73d141039530e2fbbc57c87b93ca6c87b", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[11], &account1), - "1eddbbfdf09404546c5a4a15e89b7811a658559cdde1960bcebc3aefe0ae86dd1c05a8418b208c75f3ad011d839e045ffa522135cb2cabc5c08f34b2060b5431", - ))), - Some(InputWitness::Standard(make_pub_key_hash_spend_sig( - find_pub_key_for_pkh_dest(&acc_dests[12], &account1), - "180a0f411eb920d7dd2b71cdf0a8f71a20ca82563c6cb6aa48fe20b7000ef05c1257968b3d6a7b369c8b131b83d421feae951c06ec27f5223e540a8eafb9842b", + "2638c6936859e41083f6254e7ee1eaa9f06c2e3c39f635224828d06befa0b78699776df9160b08a94d55c18fa7d7369f87d9842add12727874c60e12894349c9", ))), + Some(InputWitness::NoSignature(None)), ] } SighashInputCommitmentVersion::V1 => { let htlc_multisig = { let sigs_hex = [ - (0, "354fc2b8b65823568fa2648abb15eaa368229db43dbd54f6ba6516ff0b863f08ce01b7bae776412b96770f19077194107f02d65bcbacc0c6a1783ae7e1d5775b"), - (1, "c1ec02cf76e3059293663831a08431d264426d32269047b88895da56af0dc5ceb6a59311820418673355ce1c7f23bf680999b08ceb2213020952a45d87e0a92c"), + (0, "dbc526968f2506a1682cff368c2bb805f426dffbf85bcfc175b67f97444c0cb7285d1d80f582ae8192f7c329d895182a454735ffa9debbfbdf8517f9f3b5ddfc"), + (1, "2a9dabb9953ba0b7fb66b0cad67a569f8df8947b07e41a46b640a0addc19b0131b6b2f46d3d784a5ba66d991b3e3af9ac7d6720c7d80cc6040892226a7b41669"), ]; make_htlc_multisig_spend_sig(htlc_multisig_challenge, sigs_hex) }; let multisig = { let sigs_hex = [ - (1, "50be3f9fe0f59a9e91300ad70db576e7fb7e4c2a0c63bfbc6b1412cc8ae7b1477c0a09bb9dc424cd5968da525f7d09abe5355a433f9dbebf060ec93dbc872853"), - (2, "c3555a38e7a6165b6059c8757eac62d8017d6f182b4bd676285f0660493d75832804742893eb6eb6b2aff98946ca2e4786bac509ff3bbebfa835fca5fe693f4d"), - (3, "f20620cdc0756f59969fb3eb441986d4a7480316e897a547b5a547d341ae74b98a302a485e3e1f670c744fb100bd8bfadeed86af6a3fbccbc72c80dd5520e4cd") + (1, "8aacc97858a80bb779b000f18bf82c984977ca2a4d8560ae72421c9cd9cb7b513d35a62123553e0d24c0249eb2e04d2f99fe38eb41505a1081cd2d684a8a45d2"), + (2, "c13c5bb22e719b5c7966aa9b58c2ae8e6bf3f848e62df19d38c4d41c7acd9d48eda1e13f18a25c9524904c4866143a95fd963606b5a77979a6dfc04cdebc979d"), + (3, "93b5538c13a981dc8d6978361f13627603464de82d45f966de78b829a87eab642087b5bbc5b3aed5a888b8387491fe7fcd27f3e69bf7a70e5d12f004691da173") ]; make_multisig_spend_sig(multisig_challenge, sigs_hex) }; @@ -1045,73 +1036,70 @@ pub fn test_fixed_signatures_generic2( vec![ Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&account1_dest4, &account1), - "82df5fb0af60d649f864e329426fd5a4fcdc100754360732b6f1e58ec57daa8c80ae275afe42316c67ae1c95164388c8f6019795476ed291f8b76ec43bca83f2", + "79f8b16b721e0a92109f9deb15f67a7b5baf39196ba02240ad1038faa1ec55eead0bf3a328ffeee32cb751df208dbec2125345b853d276e11357b66657e222c0", ))), Some(InputWitness::Standard(make_pub_key_spend_sig( - "c3555a38e7a6165b6059c8757eac62d8017d6f182b4bd676285f0660493d75832804742893eb6eb6b2aff98946ca2e4786bac509ff3bbebfa835fca5fe693f4d", + "c13c5bb22e719b5c7966aa9b58c2ae8e6bf3f848e62df19d38c4d41c7acd9d48eda1e13f18a25c9524904c4866143a95fd963606b5a77979a6dfc04cdebc979d", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&decommission_dest, &account1), - "ae781212213ee3204c81118abde2a572dda03cc0855ef9d5e928dec1f8403e15950f436c58d9b9d1b18b3b811d15664c58ade0951fa761f06d6efbd095da0091", + "fb6ca8eb23d55a76284878e59f51c937bcccdbfc17074c248c4b4a39fac50084e072c6b643c297a38c2eb497c6a6b45f455be62cc7ff5d72dcfd75bc6d3f3643", ))), Some(InputWitness::Standard(make_htlc_secret_spend_sig( htlc_secret, - "00ed606c3d0d71bdfbc9740ff5e173fd7595bbd29ccd0bd599b6e9213e6c9570d0d9795bf2fd77b9780149534f4bc23049336d486534932715f9b067ef41e51665", + "004f3a9f36d2e54da540e6a5d5500649ef38abd9d89bcecb8a5558fe5316fcdd5efa867f26dd99595004453dd92647b11f487769478eb8b62b2826d17b29a24dde", ))), Some(InputWitness::Standard(htlc_multisig)), Some(InputWitness::Standard(multisig)), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[0], &account1), - "1ce510fc3821c46a8d2bac667bb18ed317a99cd356eb4076c141f1c690f501bc46b7efdd182cc2b967e7a1a9457797907580bcdf73c9ecb50759f3b591c9988b", + "f43511b1a2d22df92abbb40c6a6d5e553f3eb0701c3144eb8b37712bec8ee50f07011ec8921614f484d4cdd74a779657b3ece5a3f4edb57eb4c8a5813e247aeb", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[1], &account1), - "dfac524704f63c99cb28dccadaa5db1407d8098d7dd4be2cba520af5a676cb377a707dd2f5c722eed7c4a42a2efc9e515e921a0109c6c78a233d09a1d57eae16", + "8e2d4d375590368f975ae4e74bdc4a7ce4e17bee0028428a4597b2f4bfbfe2fb501f25933a56c0a2d47a58f04f99a5742bd44113b28ef0d673f7490f3e1760d9", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[2], &account1), - "99c4bb0537f33a38dbc417de14a3a0f1fd8067f8a03b2eaa1c1ceeeeb37a9124dc13b01437cff0c28ac0fbb27bf47bb7eb50ab1401546d67bd80c66bb8125a53", + "c3a49a0e3d43e9a016d865f1eab887de11d4cbc1bf67782ccc43b348ef4b25f65d668c23114e0606469f8ab4025be28ae3877e73ca14ea37a47ef68bba4e1065", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[3], &account1), - "9b92f34dc7a10cbe9c577f3969ceec4e0df80d3b9f052437f5b83f74638cd80260f304c78481cb574600a4f2e77da2b08a06925a6fdbb9e513f9d1315f0b547a", + "bb24a759259b6053b8c129dbebde37d4b3602b15f4c2a1c9b53736052993eec48da5ba6ed88caf976285ced1711d5fa2023fd18d41ac45c0841c0794e05a5239", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[4], &account1), - "85e2c6a26a4b2b6cfe95909fa76a83d98dbf687a5441d122cceca4ee39d011e1b7798ca97bb8129b595ff4361156e880a66bfe7416fac39e396ad7a1fe3bb37a", + "2b4dc6a7c85ea4f5c819472ea8b6ac1f4bc960113f462ab1a961b0b161e3097151f94343dea7f0a3201c43d39ab1e43c6ca074adbfac0d2d5e3a1e7855463aeb", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[5], &account1), - "0b520804361b8e89547cc2cd113c881734b353f7b96f5aa53949e78465cd430cb6428c558c7b44621f95be15129b3b95fbb2352dce63c7519d2cb733503bf94f", + "d778efa779f75cb8ac2a9ecc01e5276705a2ea3a0a1abb28be9edfd62f6463d1b771598981f281cea8c4e94c2bda0849b89b8b3f58c40d0903b2ef2f9fb57645", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[6], &account1), - "9e01c6d0129956cbc19a82588ba26ac90f2311c85e4ae64474a662672ceb1c3d1744f0e2255b2cf1de56f112629bc36cfe1329b5649c4b0cec2942639fe0bf11", + "9262a4e454d03493671feb406fa47db9b7c77c58cb52afff11f1ce448cf6437aad2b0e8aa252e282e91837251890a80a902d153aa4d4a8e9e85896588851c366", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[7], &account1), - "9d0b35755453f4fdb50c447022a484ca744bbf8598054c90db5f42325fc4bb8e5937118def5bad5adcaaba5e5296e8cfd44cd74ec3718e3d42bd5d7fab44a443", + "dce047ec8b1abc1cc6cc0191c2b05ffe56250fa3c6edf5923f900948a9024ea3fbfd0c9663b8de5ff6eadd7a7d5a7e49d40d0296ef9e9ecb3bec9567a65ed41e", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[8], &account1), - "598f717400807bbc45f4044a89157b6c4f041299feb805f13e98e2396ece65dea4ed680e0841fa07919e49216939c700b189d4b6f946889c9b35ed633504c147", + "4dd98d658178198e0bc944d4cb9b5483c156059f2a683a93c6f11f6357ad39a97a1f933f616203395e8daf4702b319cbe4069ea72bc577aaf2ccee1a85558917", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[9], &account1), - "2ed5e524d72535e19c78752e689e9d81d74df1d0378d4fe2842a7aba48d7eb475524c570e277612cbe1cf36453efa9545e407ff59dd62099be7e5f6310508157", + "d5e78f191f1966a7d9f78992514c76357fd837a3051c1cbf7b899099d74150389a843f960b24c650a619066efda0c149604c996989baee8bc9a37a42e633b2c7", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[10], &account1), - "60cabd1ded089d9325c4165d60690286b3426c2f6397f65636f24537556cd69407c80ee18e479fe62344a7a669009154ac752bf292d4947d88076c684783119b", + "f189b993fdc3d09e51b6fc8c962c9364d841641dfb074ab2cebfe13bf8ecc33ec2475fd26bc24a3a98bfec913e6eff5ad3d815bf8596f13b1752823febcc8955", ))), Some(InputWitness::Standard(make_pub_key_hash_spend_sig( find_pub_key_for_pkh_dest(&acc_dests[11], &account1), - "ba3598eb2b0a7e031d75ab70dd4599e67bc58a12754ef3e844e00fca22e3967b21fd12c812f3019a4af7cb2910bb4d448beac520699863898729f4688fb1d840", - ))), - Some(InputWitness::Standard(make_pub_key_hash_spend_sig( - find_pub_key_for_pkh_dest(&acc_dests[12], &account1), - "71b647ba2b911c93fa01395cf2c94d69178ebafb93b7e70d9d601a0259de47568e90c19ce71aa7669ca2f76caa7c7cbb8e2ca874930408fe3ec88dad688a608c", + "7f86429630aeec7d0c140c097cc4f5071fa1a991359ec9c4c96a0bc7612a3acf4f949b57e4b00a450b79a93768363e5370671380e02365eae3c072b353ab3e7c", ))), + Some(InputWitness::NoSignature(None)), ] } }; @@ -1124,44 +1112,65 @@ pub fn test_fixed_signatures_generic2( } } -fn log_signature(index: usize, raw_sig: &[u8]) { - if let Ok(spend) = AuthorizedPublicKeyHashSpend::from_data(raw_sig) { - let sig = - assert_matches_return_val!(spend.signature(), Signature::Secp256k1Schnorr(sig), sig); - log::debug!("sig #{index} is AuthorizedPublicKeyHashSpend, sig = {sig:x}"); - } else if let Ok(spend) = AuthorizedPublicKeySpend::from_data(raw_sig) { - let sig = - assert_matches_return_val!(spend.signature(), Signature::Secp256k1Schnorr(sig), sig); - log::debug!("sig #{index} is AuthorizedPublicKeySpend, sig = {sig:x}"); - } else if let Ok(spend) = AuthorizedClassicalMultisigSpend::from_data(raw_sig) { - log::debug!("sig #{index} is AuthorizedClassicalMultisigSpend"); - - for (i, sig) in spend.signatures() { - let sig = assert_matches_return_val!(sig, Signature::Secp256k1Schnorr(sig), sig); - log::debug!(" sig #{i} = {sig:x}"); +fn log_witness(index: usize, witness: &InputWitness) { + match witness { + InputWitness::NoSignature(_) => { + log::debug!("sig #{index} is NoSignature"); } - } else if let Ok(spend) = AuthorizedHashedTimelockContractSpend::from_data(raw_sig) { - match spend { - AuthorizedHashedTimelockContractSpend::Secret(_, sig) => { - let sig = hex::encode(&sig); - log::debug!( - "sig #{index} is AuthorizedHashedTimelockContractSpend::Secret, sig = {sig}" + InputWitness::Standard(sig) => { + let raw_sig = sig.raw_signature(); + + if let Ok(spend) = AuthorizedPublicKeyHashSpend::from_data(raw_sig) { + let sig = assert_matches_return_val!( + spend.signature(), + Signature::Secp256k1Schnorr(sig), + sig ); - } - AuthorizedHashedTimelockContractSpend::Multisig(inner_raw_sig) => { - let inner_spend = - AuthorizedClassicalMultisigSpend::from_data(&inner_raw_sig).unwrap(); - log::debug!("sig #{index} is AuthorizedHashedTimelockContractSpend::Multisig"); + log::debug!("sig #{index} is AuthorizedPublicKeyHashSpend, sig = {sig:x}"); + } else if let Ok(spend) = AuthorizedPublicKeySpend::from_data(raw_sig) { + let sig = assert_matches_return_val!( + spend.signature(), + Signature::Secp256k1Schnorr(sig), + sig + ); + log::debug!("sig #{index} is AuthorizedPublicKeySpend, sig = {sig:x}"); + } else if let Ok(spend) = AuthorizedClassicalMultisigSpend::from_data(raw_sig) { + log::debug!("sig #{index} is AuthorizedClassicalMultisigSpend"); - for (i, sig) in inner_spend.signatures() { + for (i, sig) in spend.signatures() { let sig = assert_matches_return_val!(sig, Signature::Secp256k1Schnorr(sig), sig); log::debug!(" sig #{i} = {sig:x}"); } + } else if let Ok(spend) = AuthorizedHashedTimelockContractSpend::from_data(raw_sig) { + match spend { + AuthorizedHashedTimelockContractSpend::Secret(_, sig) => { + let sig = hex::encode(&sig); + log::debug!( + "sig #{index} is AuthorizedHashedTimelockContractSpend::Secret, sig = {sig}" + ); + } + AuthorizedHashedTimelockContractSpend::Multisig(inner_raw_sig) => { + let inner_spend = + AuthorizedClassicalMultisigSpend::from_data(&inner_raw_sig).unwrap(); + log::debug!( + "sig #{index} is AuthorizedHashedTimelockContractSpend::Multisig" + ); + + for (i, sig) in inner_spend.signatures() { + let sig = assert_matches_return_val!( + sig, + Signature::Secp256k1Schnorr(sig), + sig + ); + log::debug!(" sig #{i} = {sig:x}"); + } + } + } + } else { + panic!("Cannot decode sig #{index}"); } } - } else { - panic!("Cannot decode sig #{index}"); } } diff --git a/wallet/src/signer/tests/generic_tests.rs b/wallet/src/signer/tests/generic_tests.rs index 690d95d99d..6d8ae5e5ad 100644 --- a/wallet/src/signer/tests/generic_tests.rs +++ b/wallet/src/signer/tests/generic_tests.rs @@ -506,6 +506,13 @@ pub fn test_sign_transaction_generic( Destination::AnyoneCanSpend, ), ), + TxInput::AccountCommand( + AccountNonce::new(rng.next_u64()), + AccountCommand::ChangeTokenMetadataUri( + TokenId::new(H256::random_using(rng)), + random_ascii_alphanumeric_string(rng, 10..20).into_bytes(), + ), + ), TxInput::AccountCommand( AccountNonce::new(rng.next_u64()), AccountCommand::ConcludeOrder(concluded_order1_id), @@ -527,20 +534,13 @@ pub fn test_sign_transaction_generic( Amount::from_atoms( rng.gen_range(1..filled_order2_info.initially_asked.amount().into_atoms()), ), - Destination::AnyoneCanSpend, )), - TxInput::AccountCommand( - AccountNonce::new(rng.next_u64()), - AccountCommand::ChangeTokenMetadataUri( - TokenId::new(H256::random_using(rng)), - random_ascii_alphanumeric_string(rng, 10..20).into_bytes(), - ), - ), ]; - let acc_dests: Vec = acc_inputs - .iter() + // Note: the last input is v1 FillOrder, which must not be signed. + let acc_dests = (0..acc_inputs.len() - 1) .map(|_| destination_from_account(&mut account, &mut db_tx, rng)) - .collect(); + .chain(std::iter::once(Destination::AnyoneCanSpend)) + .collect_vec(); let (_dest_prv, dest_pub) = PrivateKey::new_from_rng(rng, KeyKind::Secp256k1Schnorr); let (_, vrf_public_key) = VRFPrivateKey::new_from_rng(rng, crypto::vrf::VRFKeyKind::Schnorrkel); diff --git a/wallet/src/signer/trezor_signer/mod.rs b/wallet/src/signer/trezor_signer/mod.rs index 02bd8b5fff..3db2348485 100644 --- a/wallet/src/signer/trezor_signer/mod.rs +++ b/wallet/src/signer/trezor_signer/mod.rs @@ -1087,11 +1087,10 @@ fn to_trezor_order_command_input( inp_req.conclude = Some(req).into(); } - OrderAccountCommand::FillOrder(order_id, amount, dest) => { + OrderAccountCommand::FillOrder(order_id, amount) => { let mut req = MintlayerFillOrderV1::new(); req.set_order_id(Address::new(chain_config, *order_id)?.into_string()); req.set_amount(amount.into_atoms().to_be_bytes().to_vec()); - req.set_destination(Address::new(chain_config, dest.clone())?.into_string()); let OrderAdditionalInfo { initially_asked, diff --git a/wallet/types/Cargo.toml b/wallet/types/Cargo.toml index ed0d5f9b98..a6f7a59601 100644 --- a/wallet/types/Cargo.toml +++ b/wallet/types/Cargo.toml @@ -10,6 +10,7 @@ rust-version.workspace = true [dependencies] common = { path = "../../common/" } crypto = { path = "../../crypto/" } +logging = { path = "../../logging/" } tx-verifier = { path = "../../chainstate/tx-verifier" } rpc-description = { path = "../../rpc/description" } randomness = { path = "../../randomness" } diff --git a/wallet/types/src/partially_signed_transaction.rs b/wallet/types/src/partially_signed_transaction.rs index 1dd4fab7a1..091bec1612 100644 --- a/wallet/types/src/partially_signed_transaction.rs +++ b/wallet/types/src/partially_signed_transaction.rs @@ -353,7 +353,7 @@ impl PartiallySignedTransaction { } TxInput::OrderAccountCommand(command) => { let id = match command { - OrderAccountCommand::FillOrder(id, _, _) => id, + OrderAccountCommand::FillOrder(id, _) => id, OrderAccountCommand::FreezeOrder(id) => id, OrderAccountCommand::ConcludeOrder(id) => id, }; @@ -424,12 +424,26 @@ impl PartiallySignedTransaction { .iter() .enumerate() .zip(&self.destinations) - .all(|((_, w), d)| match (w, d) { - (Some(InputWitness::NoSignature(_)), None) => true, - (Some(InputWitness::NoSignature(_)), Some(_)) => false, - (Some(InputWitness::Standard(_)), None) => false, - (Some(InputWitness::Standard(_)), Some(_)) => true, - (None, _) => false, + .all(|((_, witness), dest)| { + let dest_needs_signature = match dest { + Some(dest) => match dest { + Destination::AnyoneCanSpend => false, + Destination::PublicKeyHash(_) + | Destination::PublicKey(_) + | Destination::ScriptHash(_) + | Destination::ClassicMultisig(_) => true, + }, + None => false, + }; + + match (witness, dest_needs_signature) { + (Some(InputWitness::NoSignature(_)), false) => true, + (Some(InputWitness::NoSignature(_)), true) => false, + // TODO: consider returning a Result and produce an error in this case. + (Some(InputWitness::Standard(_)), false) => false, + (Some(InputWitness::Standard(_)), true) => true, + (None, _) => false, + } }) } diff --git a/wallet/wallet-controller/src/helpers.rs b/wallet/wallet-controller/src/helpers.rs index 3fdd56eb08..06a025b225 100644 --- a/wallet/wallet-controller/src/helpers.rs +++ b/wallet/wallet-controller/src/helpers.rs @@ -364,7 +364,7 @@ async fn into_utxo_and_destination( let dest = wallet.find_order_account_command_destination(cmd); let additional_infos = match cmd { - OrderAccountCommand::FillOrder(order_id, _, _) + OrderAccountCommand::FillOrder(order_id, _) | OrderAccountCommand::FreezeOrder(order_id) | OrderAccountCommand::ConcludeOrder(order_id) => { fetch_order_additional_info(rpc_client, *order_id).await? diff --git a/wasm-wrappers/WASM-API.md b/wasm-wrappers/WASM-API.md index edf33aa53c..7c8ade4d0c 100644 --- a/wasm-wrappers/WASM-API.md +++ b/wasm-wrappers/WASM-API.md @@ -316,19 +316,10 @@ Given an order id and an amount in the order's ask currency, create an input tha Note: 1) The nonce is only needed before the orders V1 fork activation. After the fork the nonce is ignored and any value can be passed for the parameter. -2) Regarding the destination parameter: - a) It can be arbitrary, i.e. it doesn't have to be the same as the destination used - in the output that will transfer away the result. - b) Though a FillOrder input is technically allowed to have a signature, it is not enforced. - I.e. not only you don't have to sign it with the private key corresponding to this - destination, you may just provide an empty signature (use `encode_witness_no_signature` - for the input instead of `encode_witness`). - c) The reasons for having a destination in FillOrder inputs are historical, however it does - serve a purpose in orders V1. This is because the current consensus rules require all - transaction inputs in a block to be distinct. And since orders V1 don't use nonces, - re-using the same destination in the inputs of multiple order-filling transactions - for the same order may result in the later transactions being rejected, if they are - broadcast to mempool at the same time. +2) FillOrder inputs should not be signed, i.e. use `encode_witness_no_signature` for the inputs + instead of `encode_witness`). + Note that in orders v0 FillOrder inputs can technically have a signature, it's just not checked. + But in orders V1 we actually require that those inputs don't have signatures. ### Function: `encode_input_for_freeze_order` diff --git a/wasm-wrappers/js-bindings-test/tests/test_orders.ts b/wasm-wrappers/js-bindings-test/tests/test_orders.ts index d3c8699dc9..cb834d5725 100644 --- a/wasm-wrappers/js-bindings-test/tests/test_orders.ts +++ b/wasm-wrappers/js-bindings-test/tests/test_orders.ts @@ -112,14 +112,9 @@ export function test_orders() { Network.Testnet ); const expected_fill_order_v1_input = [ - 3, 0, 49, 150, 254, 178, 75, 93, - 83, 251, 1, 235, 159, 200, 27, 237, - 187, 175, 123, 34, 179, 118, 53, 6, - 173, 213, 7, 212, 102, 232, 253, 9, - 244, 197, 2, 113, 2, 0, 1, 91, - 58, 110, 176, 100, 207, 6, 194, 41, - 193, 30, 91, 4, 195, 202, 103, 207, - 80, 217, 178 + 3, 0, 49, 150, 254, 178, 75, 93, 83, 251, 1, 235, 159, 200, 27, 237, + 187, 175, 123, 34, 179, 118, 53, 6, 173, 213, 7, 212, 102, 232, 253, 9, + 244, 197, 2, 113, 2, 0 ]; assert_eq_arrays(fill_order_v1_input, expected_fill_order_v1_input); diff --git a/wasm-wrappers/src/encode_input.rs b/wasm-wrappers/src/encode_input.rs index 4089cd83e9..5c467a44fe 100644 --- a/wasm-wrappers/src/encode_input.rs +++ b/wasm-wrappers/src/encode_input.rs @@ -186,19 +186,10 @@ pub fn encode_input_for_change_token_metadata_uri( /// Note: /// 1) The nonce is only needed before the orders V1 fork activation. After the fork the nonce is /// ignored and any value can be passed for the parameter. -/// 2) Regarding the destination parameter: -/// a) It can be arbitrary, i.e. it doesn't have to be the same as the destination used -/// in the output that will transfer away the result. -/// b) Though a FillOrder input is technically allowed to have a signature, it is not enforced. -/// I.e. not only you don't have to sign it with the private key corresponding to this -/// destination, you may just provide an empty signature (use `encode_witness_no_signature` -/// for the input instead of `encode_witness`). -/// c) The reasons for having a destination in FillOrder inputs are historical, however it does -/// serve a purpose in orders V1. This is because the current consensus rules require all -/// transaction inputs in a block to be distinct. And since orders V1 don't use nonces, -/// re-using the same destination in the inputs of multiple order-filling transactions -/// for the same order may result in the later transactions being rejected, if they are -/// broadcast to mempool at the same time. +/// 2) FillOrder inputs should not be signed, i.e. use `encode_witness_no_signature` for the inputs +/// instead of `encode_witness`). +/// Note that in orders v0 FillOrder inputs can technically have a signature, it's just not checked. +/// But in orders V1 we actually require that those inputs don't have signatures. #[wasm_bindgen] pub fn encode_input_for_fill_order( order_id: &str, @@ -223,11 +214,9 @@ pub fn encode_input_for_fill_order( AccountNonce::new(nonce), AccountCommand::FillOrder(order_id, fill_amount, destination), ), - OrdersVersion::V1 => TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder( - order_id, - fill_amount, - destination, - )), + OrdersVersion::V1 => { + TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(order_id, fill_amount)) + } }; Ok(input.encode()) } From d4352705acc7b55a22ad920d449195430aa2366c Mon Sep 17 00:00:00 2001 From: Mykhailo Kremniov Date: Wed, 9 Jul 2025 11:08:19 +0300 Subject: [PATCH 2/2] Update trezor repo reference --- Cargo.lock | 2 +- Cargo.toml | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7b8f1379c..a63c320133 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8638,7 +8638,7 @@ dependencies = [ [[package]] name = "trezor-client" version = "0.1.4" -source = "git+https://github.com/mintlayer/mintlayer-trezor-firmware?rev=07229ff4943537b81c18a612fe971792d2a37574#07229ff4943537b81c18a612fe971792d2a37574" +source = "git+https://github.com/mintlayer/mintlayer-trezor-firmware?rev=8bde1135d9ea313cc04a6f1755893acd237e5515#8bde1135d9ea313cc04a6f1755893acd237e5515" dependencies = [ "bitcoin", "byteorder", diff --git a/Cargo.toml b/Cargo.toml index c4386c3d81..b2919b57ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -248,7 +248,8 @@ zeroize = "1.5" [workspace.dependencies.trezor-client] git = "https://github.com/mintlayer/mintlayer-trezor-firmware" -rev = "07229ff4943537b81c18a612fe971792d2a37574" # Commit "Support Mintlayer input commitments V1" +# The commit "Remove destination from MintlayerFillOrderV1; fail if the host asks to sign a FillOrder input" +rev = "8bde1135d9ea313cc04a6f1755893acd237e5515" features = ["bitcoin", "mintlayer"] [workspace.metadata.dist.dependencies.apt]