diff --git a/Cargo.lock b/Cargo.lock index c2e1fc00702c..6d933143d57a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13834,7 +13834,6 @@ dependencies = [ "pocket-ic", "proptest", "prost 0.13.5", - "rand 0.8.5", "rand_chacha 0.3.1", "registry-canister", "reqwest", diff --git a/rs/rosetta-api/icp/BUILD.bazel b/rs/rosetta-api/icp/BUILD.bazel index 87c56e459c03..3a59d79d33ed 100644 --- a/rs/rosetta-api/icp/BUILD.bazel +++ b/rs/rosetta-api/icp/BUILD.bazel @@ -53,7 +53,6 @@ rust_library( "@crate_index//:ic-management-canister-types", "@crate_index//:lazy_static", "@crate_index//:num-bigint", - "@crate_index//:rand", "@crate_index//:reqwest", "@crate_index//:rusqlite", "@crate_index//:serde", @@ -108,7 +107,6 @@ rust_binary( "@crate_index//:ic-management-canister-types", "@crate_index//:lazy_static", "@crate_index//:num-bigint", - "@crate_index//:rand", "@crate_index//:reqwest", "@crate_index//:rolling-file", "@crate_index//:rusqlite", @@ -167,7 +165,6 @@ rust_binary( "@crate_index//:ic-management-canister-types", "@crate_index//:lazy_static", "@crate_index//:num-bigint", - "@crate_index//:rand", "@crate_index//:reqwest", "@crate_index//:rolling-file", "@crate_index//:rusqlite", @@ -244,7 +241,6 @@ rust_test( "@crate_index//:num-traits", "@crate_index//:proptest", "@crate_index//:prost", - "@crate_index//:rand", "@crate_index//:rand_chacha", "@crate_index//:reqwest", "@crate_index//:rolling-file", @@ -338,7 +334,6 @@ rust_test_suite_with_extra_srcs( "@crate_index//:num-traits", "@crate_index//:proptest", "@crate_index//:prost", - "@crate_index//:rand", "@crate_index//:rand_chacha", "@crate_index//:reqwest", "@crate_index//:rolling-file", @@ -451,7 +446,6 @@ rust_test( "@crate_index//:num-traits", "@crate_index//:proptest", "@crate_index//:prost", - "@crate_index//:rand", "@crate_index//:rand_chacha", "@crate_index//:reqwest", "@crate_index//:rolling-file", diff --git a/rs/rosetta-api/icp/CHANGELOG.md b/rs/rosetta-api/icp/CHANGELOG.md index bd4c81bfbdd4..915a356d1ee9 100644 --- a/rs/rosetta-api/icp/CHANGELOG.md +++ b/rs/rosetta-api/icp/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## Unreleased +### Changed +- When no memo is specified for a transfer in `construction/payloads`, use a deterministic memo of 0 instead of a random one. Reconstructing the same transfer now produces the same transaction hash, so the ledger's `created_at_time` based deduplication catches retried transfers instead of applying them twice. ([#10537](https://github.com/dfinity/ic/pull/10537)) ## [2.1.9] - 2026-02-02 ### Added diff --git a/rs/rosetta-api/icp/Cargo.toml b/rs/rosetta-api/icp/Cargo.toml index 132a9213e93d..b2382dfcd42c 100644 --- a/rs/rosetta-api/icp/Cargo.toml +++ b/rs/rosetta-api/icp/Cargo.toml @@ -36,7 +36,6 @@ icrc-ledger-types = { path = "../../../packages/icrc-ledger-types" } lazy_static = { workspace = true } num-bigint = { workspace = true } on_wire = { path = "../../rust_canisters/on_wire" } -rand = { workspace = true } registry-canister = { path = "../../registry/canister" } reqwest = { workspace = true } rolling-file = { workspace = true } diff --git a/rs/rosetta-api/icp/src/models.rs b/rs/rosetta-api/icp/src/models.rs index ed80e63ea44d..5e980b322ea6 100644 --- a/rs/rosetta-api/icp/src/models.rs +++ b/rs/rosetta-api/icp/src/models.rs @@ -200,7 +200,7 @@ impl TryFrom for ParsedTransaction { #[derive(Clone, Eq, PartialEq, Debug, Default, Deserialize, Serialize)] pub struct ConstructionPayloadsRequestMetadata { /// The memo to use for a ledger transfer. - /// A random number is used by default. + /// Defaults to 0 if not specified. #[serde(skip_serializing_if = "Option::is_none")] pub memo: Option, diff --git a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs index 0e8be5d6438c..ad2e33a02918 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs @@ -672,7 +672,7 @@ mod tests { models::{ Amount, ConstructionCombineRequest, ConstructionDeriveRequest, ConstructionParseRequest, ConstructionPayloadsRequest, - ConstructionPayloadsRequestMetadata, Currency, CurveType, Operation, + ConstructionPayloadsRequestMetadata, Currency, CurveType, NetworkIdentifier, Operation, OperationIdentifier, PublicKey, Signature, SignatureType, operation::OperationType, }, request_handler::RosettaRequestHandler, @@ -681,106 +681,8 @@ mod tests { #[test] fn test_payloads_parse_identity() { - let key = ic_ed25519::PrivateKey::generate_using_rng(&mut OsRng); - let ledger_client = futures::executor::block_on(LedgerClient::new( - Url::from_str("http://localhost:1234").unwrap(), - CanisterId::from_u64(1), - "TKN".into(), - CanisterId::from_u64(2), - None, - None, - true, - None, - false, - false, // optimize_search_indexes: disabled for tests - )) - .unwrap(); - // Create a mock canister ID for testing - let mock_canister_id_hex = "00000000000000000101"; - let initial_sync_complete = AtomicBool::new(true); - let handler = RosettaRequestHandler::new( - "Internet Computer".into(), - ledger_client.into(), - RosettaMetrics::new("TKN".into(), mock_canister_id_hex.into()), - Arc::new(initial_sync_complete), - ); - - // get the nextwork identifier - let network_identifier = handler.network_id(); - let currency = Currency { - symbol: "TKN".into(), - decimals: 8, - metadata: None, - }; - - // get the account from the public key - let pub_key = crate::models::PublicKey { - hex_bytes: hex::encode(key.public_key().serialize_raw()), - curve_type: CurveType::Edwards25519, - }; - let account = handler - .construction_derive(ConstructionDeriveRequest { - network_identifier: network_identifier.clone(), - public_key: pub_key.clone(), - metadata: None, - }) - .unwrap() - .account_identifier; + let (handler, network_identifier, operations, pub_key, key) = setup_transfer_test(); - // create the unsigned transaction - let operations = vec![ - Operation { - operation_identifier: OperationIdentifier { - index: 0, - network_index: None, - }, - related_operations: None, - type_: OperationType::Transaction.to_string(), - status: None, - account: account.clone(), - amount: Some(Amount { - value: "-100000000".into(), - currency: currency.clone(), - metadata: None, - }), - coin_change: None, - metadata: None, - }, - Operation { - operation_identifier: OperationIdentifier { - index: 1, - network_index: None, - }, - related_operations: None, - type_: OperationType::Transaction.to_string(), - status: None, - account: account.clone(), - amount: Some(Amount { - value: "100000000".into(), - currency: currency.clone(), - metadata: None, - }), - coin_change: None, - metadata: None, - }, - Operation { - operation_identifier: OperationIdentifier { - index: 2, - network_index: None, - }, - related_operations: None, - type_: OperationType::Fee.to_string(), - status: None, - account, - amount: Some(Amount { - value: "-1000000".into(), - currency, - metadata: None, - }), - coin_change: None, - metadata: None, - }, - ]; let gen_opt_u64 = proptest::option::of(proptest::prelude::any::()); const ONE_HOUR_NANOS: u64 = 60 * 60 * 1_000_000_000; let now = SystemTime::now() @@ -811,7 +713,8 @@ mod tests { ) -> std::result::Result<(), TestCaseError> { match expected_metadata { None => { - // memo and created_at_time should always be set but the value is random + // memo and created_at_time are always populated by construction_payloads + // (memo defaults to 0, created_at_time to the current time) prop_assert!( actual_metadata.contains_key("memo"), "Metadata should always contain a memo" @@ -924,4 +827,178 @@ mod tests { check_metadata(metadata, parsed.metadata.unwrap()).unwrap() }); } + + /// Builds a `RosettaRequestHandler` together with a single ICP transfer + /// (debit + credit + fee) and the signer's public and private keys, shared + /// by the construction tests below. + fn setup_transfer_test() -> ( + RosettaRequestHandler, + NetworkIdentifier, + Vec, + PublicKey, + ic_ed25519::PrivateKey, + ) { + let key = ic_ed25519::PrivateKey::generate_using_rng(&mut OsRng); + let ledger_client = futures::executor::block_on(LedgerClient::new( + Url::from_str("http://localhost:1234").unwrap(), + CanisterId::from_u64(1), + "TKN".into(), + CanisterId::from_u64(2), + None, + None, + true, + None, + false, + false, // optimize_search_indexes: disabled for tests + )) + .unwrap(); + let mock_canister_id_hex = "00000000000000000101"; + let initial_sync_complete = AtomicBool::new(true); + let handler = RosettaRequestHandler::new( + "Internet Computer".into(), + ledger_client.into(), + RosettaMetrics::new("TKN".into(), mock_canister_id_hex.into()), + Arc::new(initial_sync_complete), + ); + + let network_identifier = handler.network_id(); + let currency = Currency { + symbol: "TKN".into(), + decimals: 8, + metadata: None, + }; + + let pub_key = PublicKey { + hex_bytes: hex::encode(key.public_key().serialize_raw()), + curve_type: CurveType::Edwards25519, + }; + let account = handler + .construction_derive(ConstructionDeriveRequest { + network_identifier: network_identifier.clone(), + public_key: pub_key.clone(), + metadata: None, + }) + .unwrap() + .account_identifier; + + let operations = vec![ + Operation { + operation_identifier: OperationIdentifier { + index: 0, + network_index: None, + }, + related_operations: None, + type_: OperationType::Transaction.to_string(), + status: None, + account: account.clone(), + amount: Some(Amount { + value: "-100000000".into(), + currency: currency.clone(), + metadata: None, + }), + coin_change: None, + metadata: None, + }, + Operation { + operation_identifier: OperationIdentifier { + index: 1, + network_index: None, + }, + related_operations: None, + type_: OperationType::Transaction.to_string(), + status: None, + account: account.clone(), + amount: Some(Amount { + value: "100000000".into(), + currency: currency.clone(), + metadata: None, + }), + coin_change: None, + metadata: None, + }, + Operation { + operation_identifier: OperationIdentifier { + index: 2, + network_index: None, + }, + related_operations: None, + type_: OperationType::Fee.to_string(), + status: None, + account, + amount: Some(Amount { + value: "-1000000".into(), + currency, + metadata: None, + }), + coin_change: None, + metadata: None, + }, + ]; + + (handler, network_identifier, operations, pub_key, key) + } + + // When the caller does not specify a memo, `construction_payloads` uses a + // deterministic memo of 0 (the value the ledger treats as "no memo") + // instead of a random one. With the time-based inputs pinned, reconstructing + // the same transfer must therefore produce a byte-identical unsigned + // transaction every time -- and hence the same transaction hash -- so that + // the ledger's `created_at_time` based deduplication can catch a retried + // transfer. This test reconstructs the same transfer three times and checks + // both that the unsigned transactions are identical and that the embedded + // memo is 0. + #[test] + fn test_payloads_without_memo_is_deterministic() { + let (handler, network_identifier, operations, pub_key, _key) = setup_transfer_test(); + + // Pin all time-based inputs so that the only thing that could vary + // across reconstructions is the (previously random) memo. + const NANOS: u64 = 1_000_000_000; + let created_at_time = 1_700_000_000 * NANOS; + let metadata = ConstructionPayloadsRequestMetadata { + memo: None, + created_at_time: Some(created_at_time), + ingress_start: Some(created_at_time), + ingress_end: Some(created_at_time + 600 * NANOS), + }; + + let reconstruct = || { + handler + .construction_payloads(ConstructionPayloadsRequest { + network_identifier: network_identifier.clone(), + operations: operations.clone(), + metadata: Some(metadata.clone().try_into().unwrap()), + public_keys: Some(vec![pub_key.clone()]), + }) + .unwrap() + .unsigned_transaction + }; + + let unsigned_transactions: Vec = (0..3).map(|_| reconstruct()).collect(); + + // Reconstruction must be deterministic: identical bytes => identical + // transaction hash, which is what lets the ledger deduplicate retries. + assert!( + unsigned_transactions + .iter() + .all(|tx| *tx == unsigned_transactions[0]), + "expected identical unsigned transactions across reconstructions, got {unsigned_transactions:?}" + ); + + // And the memo embedded in the (memo-less) transfer must be 0. + let parsed = handler + .construction_parse(ConstructionParseRequest { + network_identifier: network_identifier.clone(), + signed: false, + transaction: unsigned_transactions[0].clone(), + }) + .unwrap(); + let memo = parsed + .metadata + .expect("metadata should always be returned") + .get("memo") + .expect("memo should always be present") + .clone(); + assert_eq!(memo, serde_json::json!(0), "expected a memo of 0"); + } } diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index a160a6fe0634..04bf8f8296ab 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -29,7 +29,6 @@ use ic_types::{ messages::{Blob, HttpCanisterUpdate, MessageId}, }; use icp_ledger::{Memo, Operation, SendArgs, Tokens}; -use rand::Rng; use rosetta_core::convert::principal_id_from_public_key; use std::{collections::HashMap, sync::Arc, time::Duration}; use tracing::log::debug; @@ -89,12 +88,16 @@ impl RosettaRequestHandler { .map(ic_ledger_core::timestamp::TimeStamp::from_nanos_since_unix_epoch) .unwrap_or_else(|| std::time::SystemTime::now().into()); - // FIXME: the memo field needs to be associated with the operation + // When the caller does not specify a memo, default to Memo(0) (the + // ledger's "no memo" value) rather than a random memo. A random memo + // would change the transaction hash on every reconstruction of the + // same transfer, defeating the ledger's created_at_time based + // deduplication and allowing a retried transfer to be applied twice. let memo: Memo = meta .as_ref() .and_then(|meta| meta.memo) .map(Memo) - .unwrap_or_else(|| Memo(rand::thread_rng().r#gen())); + .unwrap_or(Memo(0)); let mut ingress_expiries = vec![]; let mut now = ingress_start;