From b4199f0578e94fe68516f29173607bd01ba77d5e Mon Sep 17 00:00:00 2001 From: Linwei Shang Date: Tue, 26 Mar 2024 10:14:53 -0400 Subject: [PATCH] fix: call bitcoin query API on the mainnet (#3668) * add e2e tests * refactor * turn off verify_query_signatures for bitcoin query * fix * warning query management canister * fix * changelog * upgrade agent and simplify * improve warning * improve changelog * update agent-rs rev * improve e2e * tracking TODO with Jira ticket * shellcheck --- CHANGELOG.md | 16 +- Cargo.lock | 8 +- Cargo.toml | 6 +- e2e/tests-dfx/bitcoin.bash | 3 +- e2e/tests-dfx/call.bash | 46 ++++ src/dfx/src/commands/canister/call.rs | 308 ++++++++++++++------------ src/dfx/src/commands/canister/sign.rs | 15 +- 7 files changed, 251 insertions(+), 151 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d765ff1c2b..7202b35952 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ # UNRELEASED +### fix: call management canister Bitcoin query API without replica-signed query + +`dfx canister call --query` defaults to use "Replica-signed query" feature. + +It doesn't work with bitcoin query calls to the management canister because the Boundary Nodes cannot route the `read_state` call. + +Only for these particular queries, `dfx` will make the query calls without checking the replica signatures. + +If the response reliability is a concern, you can make update calls to the secure alternatives. + ### feat(beta): enable cycles ledger support If the environment variable `DFX_CYCLES_LEDGER_SUPPORT_ENABLE` is set and no cycles wallet is configured, then dfx will try to use the cycles ledger to perform any operation that the cycles wallet usually is used for. @@ -24,15 +34,15 @@ See also: https://github.com/dfinity/idl2json Added commas in between fields, and newlines to improve formatting. -### fix canister status output to be grep compatible +### fix: canister status output to be grep compatible `dfx canister status` now outputs to `stdout`, rather than `stderr`, so that its output is `grep` compatible. -### fix fetching canister logs to be grep & tail compatible +### fix: fetching canister logs to be grep & tail compatible `dfx canister logs` now outputs to stdout, rather than stderr, so that its output is `grep` and `tail` compatible. -### fix fetching canister logs +### fix: fetching canister logs The management canister method `fetch_canister_logs` can be called only as a query, not as an update call. Therefore, `dfx canister logs ` now uses a query call for this purpose. diff --git a/Cargo.lock b/Cargo.lock index f8ac0d8927..73b4c7fd99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2496,7 +2496,7 @@ dependencies = [ [[package]] name = "ic-agent" version = "0.34.0" -source = "git+https://github.com/dfinity/agent-rs.git?rev=7869e06b048a69210849bd43ed939fe0c67df564#7869e06b048a69210849bd43ed939fe0c67df564" +source = "git+https://github.com/dfinity/agent-rs.git?rev=8c39e26236e3e3db6db51ffa71e34140c19d207e#8c39e26236e3e3db6db51ffa71e34140c19d207e" dependencies = [ "backoff", "cached 0.46.1", @@ -2933,7 +2933,7 @@ dependencies = [ [[package]] name = "ic-identity-hsm" version = "0.34.0" -source = "git+https://github.com/dfinity/agent-rs.git?rev=7869e06b048a69210849bd43ed939fe0c67df564#7869e06b048a69210849bd43ed939fe0c67df564" +source = "git+https://github.com/dfinity/agent-rs.git?rev=8c39e26236e3e3db6db51ffa71e34140c19d207e#8c39e26236e3e3db6db51ffa71e34140c19d207e" dependencies = [ "hex", "ic-agent", @@ -3033,7 +3033,7 @@ dependencies = [ [[package]] name = "ic-transport-types" version = "0.34.0" -source = "git+https://github.com/dfinity/agent-rs.git?rev=7869e06b048a69210849bd43ed939fe0c67df564#7869e06b048a69210849bd43ed939fe0c67df564" +source = "git+https://github.com/dfinity/agent-rs.git?rev=8c39e26236e3e3db6db51ffa71e34140c19d207e#8c39e26236e3e3db6db51ffa71e34140c19d207e" dependencies = [ "candid", "hex", @@ -3102,7 +3102,7 @@ dependencies = [ [[package]] name = "ic-utils" version = "0.34.0" -source = "git+https://github.com/dfinity/agent-rs.git?rev=7869e06b048a69210849bd43ed939fe0c67df564#7869e06b048a69210849bd43ed939fe0c67df564" +source = "git+https://github.com/dfinity/agent-rs.git?rev=8c39e26236e3e3db6db51ffa71e34140c19d207e#8c39e26236e3e3db6db51ffa71e34140c19d207e" dependencies = [ "async-trait", "candid", diff --git a/Cargo.toml b/Cargo.toml index 1bfe183b16..1a8b7c757e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,11 +21,11 @@ license = "Apache-2.0" [workspace.dependencies] candid = "0.10.4" candid_parser = "0.1.4" -ic-agent = { git = "https://github.com/dfinity/agent-rs.git", rev = "7869e06b048a69210849bd43ed939fe0c67df564" } +ic-agent = { git = "https://github.com/dfinity/agent-rs.git", rev = "8c39e26236e3e3db6db51ffa71e34140c19d207e" } ic-asset = { path = "src/canisters/frontend/ic-asset" } ic-cdk = "0.13.1" -ic-identity-hsm = { git = "https://github.com/dfinity/agent-rs.git", rev = "7869e06b048a69210849bd43ed939fe0c67df564" } -ic-utils = { git = "https://github.com/dfinity/agent-rs.git", rev = "7869e06b048a69210849bd43ed939fe0c67df564" } +ic-identity-hsm = { git = "https://github.com/dfinity/agent-rs.git", rev = "8c39e26236e3e3db6db51ffa71e34140c19d207e" } +ic-utils = { git = "https://github.com/dfinity/agent-rs.git", rev = "8c39e26236e3e3db6db51ffa71e34140c19d207e" } aes-gcm = "0.10.3" anyhow = "1.0.56" diff --git a/e2e/tests-dfx/bitcoin.bash b/e2e/tests-dfx/bitcoin.bash index dac572b1c3..605f7a043a 100644 --- a/e2e/tests-dfx/bitcoin.bash +++ b/e2e/tests-dfx/bitcoin.bash @@ -207,7 +207,8 @@ set_local_network_bitcoin_enabled() { min_confirmations = opt (1 : nat32); } )' - assert_eq "(0 : nat64)" + # shellcheck disable=SC2154 + assert_eq "(0 : nat64)" "$stdout" # bitcoin_get_balance_query assert_command dfx canister call --query aaaaa-aa --candid bitcoin.did bitcoin_get_utxos_query '( diff --git a/e2e/tests-dfx/call.bash b/e2e/tests-dfx/call.bash index ed48ebd40d..c5a07123ba 100644 --- a/e2e/tests-dfx/call.bash +++ b/e2e/tests-dfx/call.bash @@ -236,3 +236,49 @@ teardown() { assert_command dfx canister call "$CANISTER_ID" greet '("you")' assert_match '("Hello, you!")' } + +@test "call management canister - bitcoin query API on the IC mainnet" { + WARNING="call to the management canister cannot be benefit from the \"Replica Signed Queries\" feature. +The response might not be trustworthy. +If you want to get reliable result, you can make an update call to the secure alternative:" + # bitcoin_get_balance_query + ## bitcoin mainnet + assert_command dfx canister call --network ic --query aaaaa-aa bitcoin_get_balance_query '( + record { + network = variant { mainnet }; + address = "bcrt1qu58aj62urda83c00eylc6w34yl2s6e5rkzqet7"; + } +)' + # shellcheck disable=SC2154 + assert_contains "bitcoin_get_balance_query $WARNING bitcoin_get_balance" "$stderr" + ## bitcoin testnet + assert_command dfx canister call --network ic --query aaaaa-aa bitcoin_get_balance_query '( + record { + network = variant { testnet }; + address = "bcrt1qu58aj62urda83c00eylc6w34yl2s6e5rkzqet7"; + } +)' + # shellcheck disable=SC2154 + assert_contains "bitcoin_get_balance_query $WARNING bitcoin_get_balance" "$stderr" + + # bitcoin_get_utxos_query + ## bitcoin mainnet + assert_command dfx canister call --network ic --query aaaaa-aa bitcoin_get_utxos_query '( + record { + network = variant { mainnet }; + address = "bcrt1qu58aj62urda83c00eylc6w34yl2s6e5rkzqet7"; + } +)' + # shellcheck disable=SC2154 + assert_contains "bitcoin_get_utxos_query $WARNING bitcoin_get_utxos" "$stderr" + + ## bitcoin testnet + assert_command dfx canister call --network ic --query aaaaa-aa bitcoin_get_utxos_query '( + record { + network = variant { testnet }; + address = "bcrt1qu58aj62urda83c00eylc6w34yl2s6e5rkzqet7"; + } +)' + # shellcheck disable=SC2154 + assert_contains "bitcoin_get_utxos_query $WARNING bitcoin_get_utxos" "$stderr" +} diff --git a/src/dfx/src/commands/canister/call.rs b/src/dfx/src/commands/canister/call.rs index 6ec0d2b336..a28985e4a3 100644 --- a/src/dfx/src/commands/canister/call.rs +++ b/src/dfx/src/commands/canister/call.rs @@ -13,7 +13,6 @@ use candid_parser::utils::CandidSource; use clap::Parser; use dfx_core::canister::build_wallet_canister; use dfx_core::identity::CallSender; -use fn_error_context::context; use ic_utils::canister::Argument; use ic_utils::interfaces::management_canister::builders::{CanisterInstall, CanisterSettings}; use ic_utils::interfaces::management_canister::MgmtMethod; @@ -136,91 +135,71 @@ async fn request_id_via_wallet_call( .map_err(|err| anyhow!("Agent error {}", err)) } -#[context( - "Failed to determine effective canister id of method '{}' regarding canister {}.", - method_name, - canister_id -)] +// TODO: move to ic_utils? SDKTG-302 pub fn get_effective_canister_id( - is_management_canister: bool, - method_name: &str, + method_name: &MgmtMethod, arg_value: &[u8], - canister_id: CanisterId, ) -> DfxResult { - if is_management_canister { - let method_name = MgmtMethod::from_str(method_name).map_err(|_| { - anyhow!( - "Attempted to call an unsupported management canister method: {}", - method_name - ) - })?; - match method_name { - MgmtMethod::CreateCanister | MgmtMethod::RawRand - | MgmtMethod::BitcoinGetBalance | MgmtMethod::BitcoinGetUtxos - | MgmtMethod::BitcoinSendTransaction | MgmtMethod::BitcoinGetCurrentFeePercentiles - | MgmtMethod::EcdsaPublicKey | MgmtMethod::SignWithEcdsa - | MgmtMethod::NodeMetricsHistory => { - Err(DiagnosedError::new( - format!( - "{} can only be called by a canister, not by an external user.", - method_name.as_ref() - ), - format!("The easiest way to call {} externally is to proxy this call through a wallet. Try calling this with 'dfx canister call (--network ic) --wallet '.\n\ - To figure out the id of your wallet, run 'dfx identity get-wallet (--network ic)'.", method_name.as_ref()) - )).context("Method only callable by a canister.") - } - MgmtMethod::InstallCode => { - let install_args = candid::Decode!(arg_value, CanisterInstall) - .context("Failed to decode arguments.")?; - Ok(install_args.canister_id) - } - MgmtMethod::UpdateSettings => { - #[derive(CandidType, Deserialize)] - struct In { - canister_id: CanisterId, - settings: CanisterSettings, - } - let in_args = - candid::Decode!(arg_value, In).context("Failed to decode arguments.")?; - Ok(in_args.canister_id) - } - MgmtMethod::StartCanister - | MgmtMethod::StopCanister - | MgmtMethod::CanisterStatus - | MgmtMethod::DeleteCanister - | MgmtMethod::DepositCycles - | MgmtMethod::UninstallCode - | MgmtMethod::ProvisionalTopUpCanister - | MgmtMethod::UploadChunk - | MgmtMethod::ClearChunkStore - | MgmtMethod::StoredChunks - | MgmtMethod::FetchCanisterLogs => { - #[derive(CandidType, Deserialize)] - struct In { - canister_id: CanisterId, - } - let in_args = - candid::Decode!(arg_value, In).context("Failed to decode arguments.")?; - Ok(in_args.canister_id) - } - MgmtMethod::ProvisionalCreateCanisterWithCycles => { - Ok(CanisterId::management_canister()) + match method_name { + MgmtMethod::CreateCanister + | MgmtMethod::RawRand + | MgmtMethod::BitcoinGetBalance + | MgmtMethod::BitcoinGetUtxos + | MgmtMethod::BitcoinSendTransaction + | MgmtMethod::BitcoinGetCurrentFeePercentiles + | MgmtMethod::EcdsaPublicKey + | MgmtMethod::SignWithEcdsa + | MgmtMethod::NodeMetricsHistory => Ok(CanisterId::management_canister()), + MgmtMethod::InstallCode => { + // TODO: Maybe this case can be merged with the following one. + let install_args = candid::Decode!(arg_value, CanisterInstall) + .context("Failed to decode arguments.")?; + Ok(install_args.canister_id) + } + MgmtMethod::UpdateSettings => { + // TODO: Maybe this case can be merged with the following one. + #[derive(CandidType, Deserialize)] + struct In { + canister_id: CanisterId, + settings: CanisterSettings, } - MgmtMethod::InstallChunkedCode => { - #[derive(CandidType, Deserialize)] - struct In { - target_canister: Principal, - } - let in_args = Decode!(arg_value, In) - .context("Argument is not valid for InstallChunkedCode")?; - Ok(in_args.target_canister) + let in_args = candid::Decode!(arg_value, In).context("Failed to decode arguments.")?; + Ok(in_args.canister_id) + } + MgmtMethod::StartCanister + | MgmtMethod::StopCanister + | MgmtMethod::CanisterStatus + | MgmtMethod::DeleteCanister + | MgmtMethod::DepositCycles + | MgmtMethod::UninstallCode + | MgmtMethod::ProvisionalTopUpCanister + | MgmtMethod::UploadChunk + | MgmtMethod::ClearChunkStore + | MgmtMethod::StoredChunks + | MgmtMethod::FetchCanisterLogs => { + #[derive(CandidType, Deserialize)] + struct In { + canister_id: CanisterId, } - MgmtMethod::BitcoinGetUtxosQuery | MgmtMethod::BitcoinGetBalanceQuery => { - Ok(CanisterId::management_canister()) + let in_args = candid::Decode!(arg_value, In).context("Failed to decode arguments.")?; + Ok(in_args.canister_id) + } + MgmtMethod::ProvisionalCreateCanisterWithCycles => { + // TODO: Should we use the provisional_create_canister_effective_canister_id option from main.rs? + Ok(CanisterId::management_canister()) + } + MgmtMethod::InstallChunkedCode => { + #[derive(CandidType, Deserialize)] + struct In { + target_canister: Principal, } + let in_args = + Decode!(arg_value, In).context("Argument is not valid for InstallChunkedCode")?; + Ok(in_args.target_canister) + } + MgmtMethod::BitcoinGetUtxosQuery | MgmtMethod::BitcoinGetBalanceQuery => { + Ok(CanisterId::management_canister()) } - } else { - Ok(canister_id) } } @@ -260,11 +239,97 @@ pub async fn exec( warn!(env.get_logger(), "Cannot fetch Candid interface for {method_name}, sending arguments with inferred types."); } - let is_management_canister = canister_id == CanisterId::management_canister(); + let (argument_from_cli, argument_type) = opts.argument_from_cli.get_argument_and_type()?; - let is_query_method = method_type.as_ref().map(|(_, f)| f.is_query()); + // Get the argument, get the type, convert the argument to the type and return + // an error if any of it doesn't work. + let arg_value = blob_from_arguments( + Some(env), + argument_from_cli.as_deref(), + opts.random.as_deref(), + argument_type.as_deref(), + &method_type, + false, + opts.always_assist, + )?; - let (argument_from_cli, argument_type) = opts.argument_from_cli.get_argument_and_type()?; + let mut disable_verify_query_signatures = false; + let effective_canister_id = if canister_id == CanisterId::management_canister() { + let management_method = MgmtMethod::from_str(method_name).map_err(|_| { + anyhow!( + "Attempted to call an unsupported management canister method: {}", + method_name + ) + })?; + + if matches!( + management_method, + MgmtMethod::BitcoinGetBalanceQuery | MgmtMethod::BitcoinGetUtxosQuery + ) { + match call_sender { + CallSender::SelectedId => { + // Query calls to those two bitcoin query methods can not make a following read_state call. + // We have to use a non-verify_query_signatures agent to make the call. + // Rust's lifetime rule forces us to create the special env/agent outside this block. + // agent = non_verify_query_signatures_agent; + disable_verify_query_signatures = true; + let secure_alt = match management_method { + MgmtMethod::BitcoinGetBalanceQuery => "bitcoin_get_balance", + MgmtMethod::BitcoinGetUtxosQuery => "bitcoin_get_utxos", + _ => unreachable!(), + }; + warn!(env.get_logger(), "{method_name} call to the management canister cannot be benefit from the \"Replica Signed Queries\" feature. +The response might not be trustworthy. +If you want to get reliable result, you can make an update call to the secure alternative: {secure_alt}"); + } + CallSender::Wallet(_) => { + return Err(DiagnosedError::new( + format!( + "{} can only be called directly without a wallet proxy.", + method_name + ), + "Please remove the `--wallet ` option.".to_string(), + )) + .context("Method only callable without wallet proxy."); + } + } + } + + if matches!(call_sender, CallSender::SelectedId) + && matches!( + management_method, + MgmtMethod::CreateCanister + | MgmtMethod::RawRand + | MgmtMethod::BitcoinGetBalance + | MgmtMethod::BitcoinGetUtxos + | MgmtMethod::BitcoinSendTransaction + | MgmtMethod::BitcoinGetCurrentFeePercentiles + | MgmtMethod::EcdsaPublicKey + | MgmtMethod::SignWithEcdsa + | MgmtMethod::NodeMetricsHistory + ) + { + return Err(DiagnosedError::new( + format!( + "{} can only be called by a canister, not by an external user.", + method_name + ), + format!( + "The easiest way to call {} externally is to proxy this call through a wallet. +Try calling this with 'dfx canister call (--network ic) --wallet '. +To figure out the id of your wallet, run 'dfx identity get-wallet (--network ic)'.", + method_name + ), + )) + .context("Method only callable by a canister."); + } + + get_effective_canister_id(&management_method, &arg_value)? + } else { + canister_id + }; + + let is_query_method = method_type.as_ref().map(|(_, f)| f.is_query()); let output_type = opts.output.as_deref(); let is_query = if opts.r#async { @@ -287,18 +352,6 @@ pub async fn exec( } }; - // Get the argument, get the type, convert the argument to the type and return - // an error if any of it doesn't work. - let arg_value = blob_from_arguments( - Some(env), - argument_from_cli.as_deref(), - opts.random.as_deref(), - argument_type.as_deref(), - &method_type, - false, - opts.always_assist, - )?; - // amount has been validated by cycle_amount_validator let cycles = opts.with_cycles.unwrap_or(0); @@ -310,19 +363,17 @@ pub async fn exec( if is_query { let blob = match call_sender { CallSender::SelectedId => { - let effective_canister_id = get_effective_canister_id( - is_management_canister, - method_name, - &arg_value, - canister_id, - )?; - agent + let query_builder = agent .query(&canister_id, method_name) .with_effective_canister_id(effective_canister_id) - .with_arg(arg_value) - .call() - .await - .context("Failed query call.")? + .with_arg(arg_value); + match disable_verify_query_signatures { + true => query_builder + .call_without_verification() + .await + .context("Failed query call.")?, + false => query_builder.call().await.context("Failed query call.")?, + } } CallSender::Wallet(wallet_id) => { let wallet = build_wallet_canister(*wallet_id, agent).await?; @@ -342,21 +393,13 @@ pub async fn exec( print_idl_blob(&blob, output_type, &method_type)?; } else if opts.r#async { let request_id = match call_sender { - CallSender::SelectedId => { - let effective_canister_id = get_effective_canister_id( - is_management_canister, - method_name, - &arg_value, - canister_id, - )?; - agent - .update(&canister_id, method_name) - .with_effective_canister_id(effective_canister_id) - .with_arg(arg_value) - .call() - .await - .context("Failed update call.")? - } + CallSender::SelectedId => agent + .update(&canister_id, method_name) + .with_effective_canister_id(effective_canister_id) + .with_arg(arg_value) + .call() + .await + .context("Failed update call.")?, CallSender::Wallet(wallet_id) => { let wallet = build_wallet_canister(*wallet_id, agent).await?; let mut args = Argument::default(); @@ -371,21 +414,13 @@ pub async fn exec( println!("0x{}", String::from(request_id)); } else { let blob = match call_sender { - CallSender::SelectedId => { - let effective_canister_id = get_effective_canister_id( - is_management_canister, - method_name, - &arg_value, - canister_id, - )?; - agent - .update(&canister_id, method_name) - .with_effective_canister_id(effective_canister_id) - .with_arg(arg_value) - .call_and_wait() - .await - .context("Failed update call.")? - } + CallSender::SelectedId => agent + .update(&canister_id, method_name) + .with_effective_canister_id(effective_canister_id) + .with_arg(arg_value) + .call_and_wait() + .await + .context("Failed update call.")?, CallSender::Wallet(wallet_id) => { let wallet = build_wallet_canister(*wallet_id, agent).await?; do_wallet_call( @@ -398,10 +433,9 @@ pub async fn exec( }, ) .await - .context("Failet to do wallet call.")? + .context("Failed to do wallet call.")? } }; - print_idl_blob(&blob, output_type, &method_type)?; } diff --git a/src/dfx/src/commands/canister/sign.rs b/src/dfx/src/commands/canister/sign.rs index 0a1051ae79..4bcf45e453 100644 --- a/src/dfx/src/commands/canister/sign.rs +++ b/src/dfx/src/commands/canister/sign.rs @@ -13,6 +13,7 @@ use clap::Parser; use dfx_core::identity::CallSender; use ic_agent::AgentError; use ic_agent::RequestId; +use ic_utils::interfaces::management_canister::MgmtMethod; use slog::info; use std::convert::TryInto; use std::fs::File; @@ -160,9 +161,17 @@ pub async fn exec( let mut sign_agent = agent.clone(); sign_agent.set_transport(SignTransport::new(file_name.clone(), message_template)); - let is_management_canister = canister_id == Principal::management_canister(); - let effective_canister_id = - get_effective_canister_id(is_management_canister, method_name, &arg_value, canister_id)?; + let effective_canister_id = if canister_id == Principal::management_canister() { + let management_method = MgmtMethod::from_str(method_name).map_err(|_| { + anyhow!( + "Attempted to call an unsupported management canister method: {}", + method_name + ) + })?; + get_effective_canister_id(&management_method, &arg_value)? + } else { + canister_id + }; if is_query { let res = sign_agent