diff --git a/Cargo.lock b/Cargo.lock index 0a8f796..3419f35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4049,12 +4049,12 @@ dependencies = [ "serde_yaml", "sha2", "subtle", - "tari_common 5.6.0-pre.1", - "tari_common_types 5.6.0-pre.1", + "tari_common 5.7.0-pre.8", + "tari_common_types 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_script 5.6.0-pre.1", - "tari_sidechain 5.6.0-pre.1", - "tari_transaction_components 5.6.0-pre.1", + "tari_script 5.7.0-pre.8", + "tari_sidechain 5.7.0-pre.8", + "tari_transaction_components 5.7.0-pre.8", "tari_utilities 0.10.0", "tempfile", "thiserror 2.0.17", @@ -4079,7 +4079,7 @@ dependencies = [ "getrandom 0.2.16", "js-sys", "log", - "minotari_app_grpc 5.6.0-pre.1", + "minotari_app_grpc 5.7.0-pre.8", "num_cpus", "primitive-types", "rayon", @@ -4087,11 +4087,11 @@ dependencies = [ "serde", "serde-wasm-bindgen", "serde_json", - "tari_common_types 5.6.0-pre.1", + "tari_common_types 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_node_components 5.6.0-pre.1", - "tari_script 5.6.0-pre.1", - "tari_transaction_components 5.6.0-pre.1", + "tari_node_components 5.7.0-pre.8", + "tari_script 5.7.0-pre.8", + "tari_transaction_components 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 1.0.69", "tokio", @@ -4135,9 +4135,9 @@ dependencies = [ [[package]] name = "minotari_app_grpc" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2db387e9903e6d216352dbf3f89eb292c6089ce159eb3d68cd9871cf175a16ce" +checksum = "b4fa18e955af2e5db38656bcfc4cbdfdc2f52224a7eaf0fa60135c605f13ac6f" dependencies = [ "argon2 0.6.0-rc.8", "base64 0.22.1", @@ -4147,14 +4147,14 @@ dependencies = [ "rand 0.10.1", "rcgen", "subtle", - "tari_common_types 5.6.0-pre.1", + "tari_common_types 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_features 5.6.0-pre.1", - "tari_max_size 5.6.0-pre.1", - "tari_node_components 5.6.0-pre.1", - "tari_script 5.6.0-pre.1", - "tari_sidechain 5.6.0-pre.1", - "tari_transaction_components 5.6.0-pre.1", + "tari_features 5.7.0-pre.8", + "tari_max_size 5.7.0-pre.8", + "tari_node_components 5.7.0-pre.8", + "tari_script 5.7.0-pre.8", + "tari_sidechain 5.7.0-pre.8", + "tari_transaction_components 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", "tokio", @@ -4197,9 +4197,9 @@ dependencies = [ [[package]] name = "minotari_ledger_wallet_common" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd68f821f88a52ca6ddcc06083ed101c7a3259588691f2a8a0623e34f9d1d02c" +checksum = "9d0989ac957d754aa0b8b531c1ea69070659350e4aa5dc82ef336f69fe4971f3" dependencies = [ "borsh", "bs58 0.5.1", @@ -7201,9 +7201,9 @@ dependencies = [ [[package]] name = "tari_common" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8bbc37a4e637b34588cc84c8cfd12afdd12861085a96145a51f4746861177efa" +checksum = "2e75dbd275de2ef744dde446cef55ee817c9975ad756f0d9fada1c3a10dba314" dependencies = [ "anyhow", "cargo_toml", @@ -7217,7 +7217,7 @@ dependencies = [ "serde_json", "serde_yaml", "sha2", - "tari_features 5.6.0-pre.1", + "tari_features 5.7.0-pre.8", "tempfile", "thiserror 2.0.17", ] @@ -7277,9 +7277,9 @@ dependencies = [ [[package]] name = "tari_common_types" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b34229731978ccc3997da1b1ad9cf4404e3b839a5a8646aed7f5661e60ed5646" +checksum = "a0a24434012a43e2449f03423197a55b44395a645795f7339efbca2f897e265b" dependencies = [ "argon2 0.6.0-rc.8", "base64 0.22.1", @@ -7303,10 +7303,10 @@ dependencies = [ "strum", "strum_macros", "subtle", - "tari_common 5.6.0-pre.1", + "tari_common 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_hashing 5.6.0-pre.1", - "tari_max_size 5.6.0-pre.1", + "tari_hashing 5.7.0-pre.8", + "tari_max_size 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", "utoipa", @@ -7519,9 +7519,9 @@ source = "git+https://github.com/tari-project/tari/#9f5adb7183dc2ec285f5c8fae05f [[package]] name = "tari_features" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b5dad44a52b745035c6f943328c2c9164093436bddd9486ef00d9b36fe4c6d7" +checksum = "799652caf0cc72be10afabc2abeb6406e63e23023b0c87d6b01dde66796d0e89" [[package]] name = "tari_hashing" @@ -7536,9 +7536,9 @@ dependencies = [ [[package]] name = "tari_hashing" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6961913a253d3f8837fffd6b3e485c41d744cd78b862e5d62542ba61c96c044" +checksum = "1dfdd7c99f842cda33376d5f8714f5b3e6209aec1284a3627f55e2f4af25be22" dependencies = [ "blake2 0.10.6", "borsh", @@ -7562,16 +7562,16 @@ dependencies = [ [[package]] name = "tari_jellyfish" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c645cbe070f6afc845177aa3aa1d53f55d340bf51f1ebd1da2c80af2c2b0122a" +checksum = "3fef176712a5e7642a4d826b34bd9f5d5d5818ad9960eca5010a6f12cc9d9ded" dependencies = [ "borsh", "digest 0.10.7", "indexmap 2.12.0", "serde", "tari_crypto 0.23.2", - "tari_hashing 5.6.0-pre.1", + "tari_hashing 5.7.0-pre.8", "thiserror 2.0.17", ] @@ -7602,9 +7602,9 @@ dependencies = [ [[package]] name = "tari_max_size" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "70a3cf75440373d4ec3126eb70624afd56ec6279ed6664cd33c42ff52c492b07" +checksum = "a1b5d59ba3d7c70112648b678c20985d0d20b7c346ca7af7d3e6d52da6ef2153" dependencies = [ "borsh", "serde", @@ -7674,9 +7674,9 @@ dependencies = [ [[package]] name = "tari_node_components" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a46638ca51e9142cf70d28c287a1bb0483c751ed34545456ba7585fdbd0e6bba" +checksum = "b8be4ff38f9e9f6395d13ee643d6f7310bd258065a979f1af3f40ce44c6e9485" dependencies = [ "blake2 0.10.6", "borsh", @@ -7686,9 +7686,9 @@ dependencies = [ "js-sys", "primitive-types", "serde", - "tari_common_types 5.6.0-pre.1", - "tari_hashing 5.6.0-pre.1", - "tari_transaction_components 5.6.0-pre.1", + "tari_common_types 5.7.0-pre.8", + "tari_hashing 5.7.0-pre.8", + "tari_transaction_components 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", ] @@ -7742,9 +7742,9 @@ dependencies = [ [[package]] name = "tari_script" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "461ad204f38ec913bf993cb4409ee85b0a665a672e979b97c7ae3e61e8c1fb0e" +checksum = "b443e73e887fe107e5caa9d5cfc9b1b6e8dd727b0760a39641a923c4c3d417f7" dependencies = [ "blake2 0.10.6", "borsh", @@ -7754,7 +7754,7 @@ dependencies = [ "sha2", "sha3", "tari_crypto 0.23.2", - "tari_max_size 5.6.0-pre.1", + "tari_max_size 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", ] @@ -7801,18 +7801,18 @@ dependencies = [ [[package]] name = "tari_sidechain" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6bbbb84a74b80ba0e604161368d6c45744f983b0fef0b84efdc661eb58a4099b" +checksum = "b8181afddbee6f12d59d6ae097cbc1915f2b4dadd1baeb9e39e4cb719a06d2ee" dependencies = [ "borsh", "hex", "log", "serde", - "tari_common_types 5.6.0-pre.1", + "tari_common_types 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_hashing 5.6.0-pre.1", - "tari_jellyfish 5.6.0-pre.1", + "tari_hashing 5.7.0-pre.8", + "tari_jellyfish 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", ] @@ -7889,9 +7889,9 @@ dependencies = [ [[package]] name = "tari_transaction_components" -version = "5.6.0-pre.1" +version = "5.7.0-pre.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f50877e6b1c7497074a5a185f1ac48acac5c0d8cf2e448398229ce5284de605" +checksum = "191a787d575052ab0bd22463fd8eaba002d3685002811faecb68790f0de72821" dependencies = [ "anyhow", "base64 0.22.1", @@ -7906,7 +7906,7 @@ dependencies = [ "digest 0.10.7", "integer-encoding", "log", - "minotari_ledger_wallet_common 5.6.0-pre.1", + "minotari_ledger_wallet_common 5.7.0-pre.8", "newtype-ops", "num-derive 0.4.2", "num-format", @@ -7919,13 +7919,13 @@ dependencies = [ "serde_repr", "serde_valid 2.0.3", "strum_macros", - "tari_common 5.6.0-pre.1", - "tari_common_types 5.6.0-pre.1", + "tari_common 5.7.0-pre.8", + "tari_common_types 5.7.0-pre.8", "tari_crypto 0.23.2", - "tari_hashing 5.6.0-pre.1", - "tari_max_size 5.6.0-pre.1", - "tari_script 5.6.0-pre.1", - "tari_sidechain 5.6.0-pre.1", + "tari_hashing 5.7.0-pre.8", + "tari_max_size 5.7.0-pre.8", + "tari_script 5.7.0-pre.8", + "tari_sidechain 5.7.0-pre.8", "tari_utilities 0.10.0", "thiserror 2.0.17", "utoipa", diff --git a/Cargo.toml b/Cargo.toml index 619e7cd..e87beeb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,15 +15,15 @@ repository = "https://github.com/tari-project/minotari-cli" [workspace.dependencies] -tari_common = { version = "5.6.0-pre.1"} -tari_common_types = { version = "5.6.0-pre.1" } +tari_common = { version = "5.7.0-pre.8"} +tari_common_types = { version = "5.7.0-pre.8" } tari_crypto = { version = "0.23", features = ["borsh"] } tari_utilities = { version = "0.10", features = ["std"] } -tari_script = { version = "5.6.0-pre.1" } -tari_sidechain = { version = "5.6.0-pre.1" } -tari_transaction_components = { version = "5.6.0-pre.1" } -minotari_app_grpc = { version = "5.6.0-pre.1", default-features = false } -tari_node_components = {version = "5.6.0-pre.1" } +tari_script = { version = "5.7.0-pre.8" } +tari_sidechain = { version = "5.7.0-pre.8" } +tari_transaction_components = { version = "5.7.0-pre.8" } +minotari_app_grpc = { version = "5.7.0-pre.8", default-features = false } +tari_node_components = {version = "5.7.0-pre.8" } minotari-scanning = { path = "minotari-scanning", version = "0.2.0" } [profile.release] diff --git a/minotari/src/main.rs b/minotari/src/main.rs index 90046ba..014822c 100644 --- a/minotari/src/main.rs +++ b/minotari/src/main.rs @@ -66,6 +66,7 @@ use minotari::{ models::WalletEvent, scan::{self, reorg::rollback_from_height}, transactions::{ + fee_estimator::estimated_output_size_for_payment_id, fund_locker::FundLocker, idempotency::{IdempotencyBinding, IdempotencyOperation, RequestFingerprint}, one_sided_transaction::{OneSidedTransaction, Recipient, unsigned_transaction_binding}, @@ -777,7 +778,15 @@ fn handle_create_unsigned_transaction( let amount = recipients.iter().map(|r| r.amount).sum(); let num_outputs = recipients.len(); let fee_per_gram = MicroMinotari(5); - let estimated_output_size = None; + // One size is charged for every output, so quote the largest memo in the batch rather than + // the default: anything smaller reserves inputs that cannot pay the fee the builder lands on. + let estimated_output_size = Some(estimated_output_size_for_payment_id( + recipients + .iter() + .map(|r| r.payment_id.as_ref().map_or(0, String::len)) + .max() + .unwrap_or(0), + )?); // Same binding the REST endpoint builds, so a key means the same thing // whichever way the request arrives. diff --git a/minotari/src/migrate/output_converter.rs b/minotari/src/migrate/output_converter.rs index 7cd221f..e31bcfa 100644 --- a/minotari/src/migrate/output_converter.rs +++ b/minotari/src/migrate/output_converter.rs @@ -199,6 +199,8 @@ pub fn convert_output(row: &ConsoleOutputRow) -> Result, ) })?; + reject_unspendable_script_key(&script_key_id, legacy_status, &row_label(row))?; + let metadata_signature = ComAndPubSignature::new( CompressedCommitment::from_canonical_bytes(&row.metadata_signature_ephemeral_commitment) .map_err(|e| anyhow!("Output {}: bad metadata ephemeral commitment: {e}", row_label(row)))?, @@ -275,6 +277,32 @@ pub fn convert_output(row: &ConsoleOutputRow) -> Result, })) } +/// Refuse a spendable row whose script key is `TariKeyId::Zero`. +/// +/// `"zero"` parses cleanly to `TariKeyId::Zero`, so such a row would import as an ordinary +/// spendable UTXO and then poison every transaction that selects it. The key manager refuses to +/// compute a script offset when any input script key is `Zero` - such a key contributes nothing +/// to the sum, so honouring it would hand back an offset that unblinds the wallet's root spend +/// key - and it fails the whole call even when real keys are present alongside. The failure is +/// therefore permanent, surfaces only once coin selection happens to pick that output, and takes +/// an entire send down with it, which is far worse than refusing the row here. +/// +/// A spent row is inert: it is never selected, so it migrates as normal. +fn reject_unspendable_script_key( + script_key_id: &TariKeyId, + legacy_status: LegacyOutputStatus, + label: &str, +) -> Result<(), anyhow::Error> { + if script_key_id == &TariKeyId::Zero && legacy_status.is_unspent() { + return Err(anyhow!( + "Output {label} has a zero script key, so this wallet cannot spend it - a zero script \ + key marks an output the source wallet sent to someone else rather than one it owns. \ + Re-check the source wallet before migrating, or exclude this output." + )); + } + Ok(()) +} + /// Decode the console wallet's `outputs.output_type` i32 column into a /// canonical `OutputType`. The i32 encoding is stable across versions of /// the console wallet (it maps directly to `OutputType as i32`). Unknown @@ -367,4 +395,28 @@ mod tests { assert!(matches!(decode_output_type(99), OutputType::Standard)); assert!(matches!(decode_output_type(-1), OutputType::Standard)); } + + #[test] + fn a_zero_script_key_is_refused_only_while_the_output_is_still_spendable() { + // 5.7's key manager rejects a script offset computed over any `Zero` key, so importing + // such a row as spendable would make every send that selects it fail permanently. + for unspent in [ + LegacyOutputStatus::Unspent, + LegacyOutputStatus::UnspentMinedUnconfirmed, + LegacyOutputStatus::EncumberedToBeSpent, + ] { + let err = reject_unspendable_script_key(&TariKeyId::Zero, unspent, "(test)") + .expect_err("a spendable zero script key must be refused"); + assert!(err.to_string().contains("zero script key"), "got: {err}"); + } + + // A spent row is never selected, so it is harmless and must still migrate. + for spent in [LegacyOutputStatus::Spent, LegacyOutputStatus::SpentMinedUnconfirmed] { + assert!(reject_unspendable_script_key(&TariKeyId::Zero, spent, "(test)").is_ok()); + } + + // A real script key is fine in every state. + let real = TariKeyId::SpendKey; + assert!(reject_unspendable_script_key(&real, LegacyOutputStatus::Unspent, "(test)").is_ok()); + } } diff --git a/minotari/src/transactions/burn/mod.rs b/minotari/src/transactions/burn/mod.rs index 511a903..0314bff 100644 --- a/minotari/src/transactions/burn/mod.rs +++ b/minotari/src/transactions/burn/mod.rs @@ -27,8 +27,10 @@ use tari_transaction_components::{ MicroMinotari, TransactionBuilder, consensus::ConsensusConstantsBuilder, key_manager::{TariKeyId, TransactionKeyManagerInterface}, + transaction_builder::PendingOutput, transaction_components::{ KernelFeatures, OutputFeatures, WalletOutputBuilder, + covenants::Covenant, memo_field::{MemoField, TxType}, }, }; @@ -38,6 +40,7 @@ use crate::{ db::{AccountRow, NewBurnProof}, models::PendingTransactionStatus, transactions::{ + fee_estimator::{estimated_output_size_for_payment_id, measure_output_size}, fund_locker::FundLocker, idempotency::{IdempotencyBinding, IdempotencyConflict, IdempotencyOperation, RequestFingerprint}, }, @@ -137,6 +140,34 @@ pub fn create_burn_tx( ); let sender_address = account.get_address(network, password)?; + + let output_features = match ¶ms.claim_public_key { + Some(cpk) => { + OutputFeatures::create_burn_confidential_output(cpk.clone(), params.sidechain_deployment_key.as_ref()) + }, + None => OutputFeatures::create_burn_output(), + }; + + let memo = params + .payment_id + .as_deref() + .and_then(|s| MemoField::new_open_from_string(s, TxType::Burn).ok()) + .unwrap_or_else(|| MemoField::new_open_from_string("", TxType::Burn).unwrap_or_default()); + + // The burn output is built by hand rather than from a recipient spec, so the script is ours to + // choose; it is needed both to measure the output for the reservation and to build it. + let burn_script = script!(Nop)?; + let weight_params = *consensus_constants.transaction_weight_params(); + + // Measure before locking, not after. A burn output carries the claim key and the sidechain + // key in its features, so it is materially larger than the generic estimate; selection that + // charged the generic size would lock inputs that cannot cover the real fee, and the burn + // would fail at build time with the funds already reserved. The change output is measured + // too, because selection charges one size for every output it plans. + let burn_output_size = measure_output_size(&output_features, &burn_script, &memo)?; + let change_output_size = estimated_output_size_for_payment_id(params.payment_id.as_deref().map_or(0, str::len))?; + let estimated_output_size = burn_output_size.max(change_output_size); + let fund_locker = FundLocker::new(); let locked_funds = fund_locker.lock( conn, @@ -144,7 +175,7 @@ pub fn create_burn_tx( params.amount, 1, params.fee_per_gram, - None, + Some(estimated_output_size), params.idempotency_binding(), params.seconds_to_lock, params.confirmation_window, @@ -152,16 +183,35 @@ pub fn create_burn_tx( let key_manager = account.get_key_manager(password)?; - let output_features = match ¶ms.claim_public_key { - Some(cpk) => { - OutputFeatures::create_burn_confidential_output(cpk.clone(), params.sidechain_deployment_key.as_ref()) - }, - None => OutputFeatures::create_burn_output(), - }; + // Assemble the transaction. + let mut tx_builder = TransactionBuilder::new(consensus_constants, key_manager.clone(), network)?; + tx_builder.with_fee_per_gram(params.fee_per_gram); + tx_builder.with_kernel_features(KernelFeatures::create_burn()); + tx_builder.with_tx_type(TxType::Burn); + tx_builder.with_memo(memo.clone()); - // Derive the commitment mask key and sender offset key for the burn output. + for utxo in &locked_funds.utxos { + tx_builder.with_input(utxo.clone())?; + } + + // Every sender offset key comes from this one reservation, which is also where the fee and the + // change decision are made — so the burn output, which is attached afterwards, has to be + // declared here by value and weight even though it does not exist yet. + let pending_burn_output = PendingOutput::measured( + &weight_params, + params.amount, + &output_features, + &burn_script, + &Covenant::default(), + &memo, + )?; + let sender_offset_key = tx_builder + .reserve_sender_offset_keys(&[pending_burn_output])? + .pop() + .ok_or_else(|| anyhow!("Transaction builder reserved no sender offset key for the burn output"))?; + + // Derive the commitment mask key for the burn output. let (commitment_mask_key, _script_key) = key_manager.get_next_commitment_mask_and_script_key()?; - let sender_offset_key = key_manager.get_random_key(None, None)?; // The encrypted data in the burn output is DH-encrypted to the claim_public_key // (so the L2 wallet can decrypt it). Fall back to the wallet's view key if no @@ -174,16 +224,10 @@ pub fn create_burn_tx( None => key_manager.get_view_key().key_id, }; - let memo = params - .payment_id - .as_deref() - .and_then(|s| MemoField::new_open_from_string(s, TxType::Burn).ok()) - .unwrap_or_else(|| MemoField::new_open_from_string("", TxType::Burn).unwrap_or_default()); - // Build the burn output with explicit key material (not stealth-address derivation). let burn_output = WalletOutputBuilder::new(params.amount, commitment_mask_key.key_id.clone()) .with_features(output_features) - .with_script(script!(Nop)?) + .with_script(burn_script) .with_input_data(Default::default()) .with_sender_offset_public_key(sender_offset_key.pub_key.clone()) .with_script_key(TariKeyId::Zero) @@ -195,17 +239,6 @@ pub fn create_burn_tx( let output_hash = burn_output.output_hash(); let commitment = burn_output.commitment().clone(); - // Assemble the transaction. - let mut tx_builder = TransactionBuilder::new(consensus_constants, key_manager.clone(), network)?; - tx_builder.with_fee_per_gram(params.fee_per_gram); - tx_builder.with_kernel_features(KernelFeatures::create_burn()); - tx_builder.with_tx_type(TxType::Burn); - tx_builder.with_memo(memo); - - for utxo in &locked_funds.utxos { - tx_builder.with_input(utxo.clone())?; - } - // Default address used as placeholder — burn outputs have no real "recipient". tx_builder.add_recipient( TariAddress::new_dual_address( @@ -216,7 +249,7 @@ pub fn create_burn_tx( None, )?, burn_output, - Some(sender_offset_key.key_id), + sender_offset_key.key_id, Some(recovery_key_id), )?; diff --git a/minotari/src/transactions/fee_estimator.rs b/minotari/src/transactions/fee_estimator.rs index 5b953e2..27e9bde 100644 --- a/minotari/src/transactions/fee_estimator.rs +++ b/minotari/src/transactions/fee_estimator.rs @@ -1,11 +1,15 @@ use anyhow::{Result, anyhow}; use log::debug; -use tari_script::TariScript; -use tari_transaction_components::helpers::borsh::SerializedSize; +use tari_common_types::{tari_address::TariAddress, types::FixedHash}; +use tari_script::{TariScript, script}; use tari_transaction_components::{ - fee::Fee, + fee::{Fee, recipient_output_features_and_scripts_size}, tari_amount::MicroMinotari, - transaction_components::{OutputFeatures, covenants::Covenant}, + transaction_components::{ + OutputFeatures, + covenants::Covenant, + memo_field::{MemoField, TxType}, + }, weight::TransactionWeight, }; @@ -137,15 +141,123 @@ impl FeeEstimator { } } +/// Measure one output the way the transaction builder charges for it. +/// +/// Upstream's `recipient_output_features_and_scripts_size` is the authority here: it counts +/// the features, the script, the covenant **and the memo carried in the output's encrypted +/// data**, then rounds up to the fee's gram boundary. Summing the first three by hand - which +/// is what this module used to do - silently drops the memo, which is usually the largest of +/// the four. +pub fn measure_output_size(features: &OutputFeatures, script: &TariScript, memo: &MemoField) -> Result { + recipient_output_features_and_scripts_size( + &TransactionWeight::latest(), + features, + script, + &Covenant::default(), + memo, + ) + .map_err(|e| anyhow!("Failed to measure output size: {e}")) +} + +/// A conservative per-output size for a payment whose memo carries `payment_id_len` bytes. +/// +/// UTXO selection charges this once per output and once for change, so it must over-estimate +/// rather than under-estimate: a short answer locks inputs that cannot cover the fee the +/// builder later computes, and `reserve_sender_offset_keys` then fails the send *after* the +/// funds are reserved, leaving the user to wait out the lock. Over-estimating only makes the +/// change output slightly smaller than it needed to be. +/// +/// The shape measured is the change output, which is the largest the builder emits on its own +/// account: a `PushPubKey` script and a `TransactionInfo` memo holding the recipient address, +/// one sent-output hash and the caller's payment id, padded to a 130-byte floor. +pub fn estimated_output_size_for_payment_id(payment_id_len: usize) -> Result { + // The script the builder derives for a recipient, and for its own change output. + let script = script!(PushPubKey(Box::default())).map_err(|e| anyhow!("Failed to build the default script: {e}"))?; + + // A change memo: a dual address, one sent-output hash and the payment id. `TariAddress` + // defaults to the dual form, which is the larger of the two it can take. + let change_memo = |payment_id: Vec| { + MemoField::new_transaction_info( + TariAddress::default(), + MicroMinotari::zero(), + MicroMinotari::zero(), + true, + TxType::PaymentToOther, + vec![FixedHash::zero()], + payment_id, + ) + }; + + // A memo has a hard 256-byte ceiling, so a payment id past it cannot go in a change memo + // at all - upstream's own `change_features_and_scripts_size` measures such a memo as zero + // and the send fails when the builder tries to construct it for real. There is no estimate + // that saves that transaction, so fall back to the floor rather than refusing to quote. + let memo = change_memo(vec![0u8; payment_id_len]) + .or_else(|_| change_memo(Vec::new())) + .map_err(|e| anyhow!("Failed to build the default change memo: {e}"))?; + + measure_output_size(&OutputFeatures::default(), &script, &memo) +} + +/// The per-output size to assume when even the payment id is unknown. pub fn get_default_features_and_scripts_size() -> Result { - let fee_calc = Fee::new(TransactionWeight::latest()); + estimated_output_size_for_payment_id(0) +} - let get_size = |res: Result| res.map_err(|e| anyhow!("Serialization error: {}", e)); - let output_features_size = get_size(OutputFeatures::default().get_serialized_size())?; - let tari_script_size = get_size(TariScript::default().get_serialized_size())?; - let covenant_size = get_size(Covenant::default().get_serialized_size())?; +#[cfg(test)] +mod tests { + use super::*; + use tari_script::script; - Ok(fee_calc - .weighting() - .round_up_features_and_scripts_size(output_features_size + tari_script_size + covenant_size)) + /// The estimate has to cover what the builder actually charges, or selection locks inputs + /// that cannot pay the fee and the send dies after the funds are reserved. + #[test] + fn the_default_estimate_covers_a_one_sided_recipient_output() { + let recipient_memo = MemoField::new_open_from_string("invoice-12345", TxType::PaymentToOther).unwrap(); + let recipient = measure_output_size( + &OutputFeatures::default(), + &script!(PushPubKey(Box::default())).unwrap(), + &recipient_memo, + ) + .unwrap(); + + assert!( + get_default_features_and_scripts_size().unwrap() >= recipient, + "default estimate must not be smaller than a real recipient output" + ); + } + + /// The old implementation summed features + an *empty* script + covenant and stopped, which + /// is where the under-estimate came from. Both missing terms have to be back. + #[test] + fn the_default_estimate_counts_the_script_and_the_memo() { + let bare = measure_output_size( + &OutputFeatures::default(), + &TariScript::default(), + &MemoField::new_empty(), + ) + .unwrap(); + + assert!( + get_default_features_and_scripts_size().unwrap() > bare, + "default estimate must count the script and the memo, not just features and covenant" + ); + } + + /// A longer payment id is a bigger output, and selection has to be told so. + #[test] + fn a_longer_payment_id_raises_the_estimate() { + let short = estimated_output_size_for_payment_id(0).unwrap(); + let long = estimated_output_size_for_payment_id(100).unwrap(); + assert!(long > short, "a 100-byte payment id must cost more than an empty one"); + } + /// A payment id too long for a memo cannot be quoted exactly; the estimate falls back to + /// the floor instead of failing the caller's request outright. + #[test] + fn an_oversized_payment_id_falls_back_to_the_floor() { + assert_eq!( + estimated_output_size_for_payment_id(4096).unwrap(), + estimated_output_size_for_payment_id(0).unwrap() + ); + } } diff --git a/minotari/src/transactions/manager.rs b/minotari/src/transactions/manager.rs index 211b51f..7836d34 100644 --- a/minotari/src/transactions/manager.rs +++ b/minotari/src/transactions/manager.rs @@ -108,6 +108,7 @@ use crate::{ DisplayedTransaction, DisplayedTransactionBuilder, TransactionDirection, TransactionDisplayStatus, TransactionInput, TransactionSource, }, + fee_estimator::estimated_output_size_for_payment_id, fund_locker::{check_replay_allowed, lock_expiry_at}, idempotency::{IdempotencyBinding, IdempotencyConflict, IdempotencyOperation, RequestFingerprint}, input_selector::{InputSelector, UtxoSelection}, @@ -396,7 +397,16 @@ impl TransactionSender { ) -> Result { let amount = processed_transaction.recipient.amount; let num_outputs = 1; - let estimated_output_size = None; + // Selection has to charge for the memo this send will actually carry. Left at the + // default, a long payment id is weight nobody reserved inputs for, and the send fails + // at build time with the funds already locked. + let estimated_output_size = Some(estimated_output_size_for_payment_id( + processed_transaction + .recipient + .payment_id + .as_ref() + .map_or(0, String::len), + )?); let input_selector = InputSelector::new(self.account.id, self.confirmation_window); let utxo_selection = input_selector.fetch_unspent_outputs( @@ -494,10 +504,12 @@ impl TransactionSender { Ok(pending_tx_id) } + /// Returns the builder together with the key manager it was built around: the payload the builder is handed to + /// is signed with that same key manager, so both halves have to come from one call. fn prepare_transaction_builder( &self, locked_utxos: Vec, - ) -> Result, anyhow::Error> { + ) -> Result<(TransactionBuilder, KeyManager), anyhow::Error> { let key_manager = self.account.get_key_manager(&self.password)?; let consensus_constants = ConsensusConstantsBuilder::new(self.network).build(); let mut tx_builder = TransactionBuilder::new(consensus_constants, key_manager.clone(), self.network)?; @@ -508,7 +520,7 @@ impl TransactionSender { tx_builder.with_input(utxo.clone())?; } - Ok(tx_builder) + Ok((tx_builder, key_manager)) } /// Starts a new transaction and returns an unsigned transaction for signing. @@ -588,7 +600,7 @@ impl TransactionSender { } let utxos = utxo_selection.into_iter().map(|db_out| db_out.output).collect(); - let tx_builder = self.prepare_transaction_builder(utxos)?; + let (tx_builder, key_manager) = self.prepare_transaction_builder(utxos)?; let sender_address = self.account.get_address(self.network, &self.password)?; let tx_id = TxId::new_random(); @@ -607,6 +619,7 @@ impl TransactionSender { }; let res = prepare_one_sided_transaction_for_signing( + &key_manager, tx_id, tx_builder, &[payment_recipient], diff --git a/minotari/src/transactions/one_sided_transaction.rs b/minotari/src/transactions/one_sided_transaction.rs index 0c621bc..64b9a46 100644 --- a/minotari/src/transactions/one_sided_transaction.rs +++ b/minotari/src/transactions/one_sided_transaction.rs @@ -288,6 +288,7 @@ impl OneSidedTransaction { let main_payment_id = payment_recipients.first().expect("Already checked").payment_id.clone(); let result = prepare_one_sided_transaction_for_signing( + &key_manager, tx_id, tx_builder, &payment_recipients, diff --git a/minotari/src/transactions/validator_node/common.rs b/minotari/src/transactions/validator_node/common.rs index 7d87a53..1eb1f09 100644 --- a/minotari/src/transactions/validator_node/common.rs +++ b/minotari/src/transactions/validator_node/common.rs @@ -93,7 +93,7 @@ pub(crate) fn build_vn_pay_to_self_tx( )?; let key_manager = account.get_key_manager(password)?; - let mut tx_builder = TransactionBuilder::new(consensus_constants, key_manager, network)?; + let mut tx_builder = TransactionBuilder::new(consensus_constants, key_manager.clone(), network)?; tx_builder.with_fee_per_gram(fee_per_gram); for utxo in &locked_funds.utxos { tx_builder.with_input(utxo.clone())?; @@ -111,6 +111,13 @@ pub(crate) fn build_vn_pay_to_self_tx( payment_id: memo.clone(), }; - prepare_one_sided_transaction_for_signing(tx_id, tx_builder, &[payment_recipient], memo, sender_address) - .map_err(Into::into) + prepare_one_sided_transaction_for_signing( + &key_manager, + tx_id, + tx_builder, + &[payment_recipient], + memo, + sender_address, + ) + .map_err(Into::into) }