feat(ckbtc): Use the new KYT canister in ckbtc deposit flow #2304

ninegua · 2024-10-29T02:57:18Z

Switch to the new KYT canister in the ckBTC deposit flow.

Also enable real https outcalls in system tests with a SSL reverse proxy + bitcoind regtest network.

…for bitcoind so that https-outcall can go through.

basvandijk · 2024-11-15T08:33:13Z

rs/tests/ckbtc/BUILD.bazel

+oci_tar(
+    name = "minica.tar",
+    image = "@minica",
+    repo_tags = ["minica:image"],
+)
+
+uvm_config_image(
+    name = "ckbtc_uvm_config_image",
+    srcs = [
+        ":minica.tar",


What about moving the equivalent //rs/tests/networking/canister_http:minica.tar to /rs/tests/BUILD.bazel and removing this one?

basvandijk · 2024-11-15T08:38:59Z

rs/tests/ckbtc/src/lib.rs

+# Run nginx auto proxy
+docker run -d --name=proxy -e ENABLE_IPV6=true -e DEFAULT_HOST=localhost -p 80:80 -p {HTTPS_PORT}:443 \
+           -v /tmp/certs:/etc/nginx/certs -v /var/run/docker.sock:/tmp/docker.sock:ro \
+           nginxproxy/nginx-proxy


Note that since this runs in a fresh Universal VM it will have to download nginxproxy/nginx-proxy for every run. This will very soon hit the docker hub rate limit and cause these tests to start failing.

Could you move this image into the UVM config image like you do for minica above?

basvandijk · 2024-11-15T08:42:40Z

rs/tests/ckbtc/src/lib.rs

+docker run  --name=bitcoind-node -d \
+  -e VIRTUAL_HOST=localhost -e VIRTUAL_PORT={BITCOIND_RPC_PORT} -v /tmp:/bitcoin/.bitcoin \
+  -p {BITCOIN_CLI_PORT}:{BITCOIN_CLI_PORT} -p {BITCOIND_RPC_PORT}:{BITCOIND_RPC_PORT} \
+  ghcr.io/dfinity/bitcoind@sha256:17c7dd21690f3be34630db7389d2f0bff14649e27a964afef03806a6d631e0f1


Ideally this image is also moved in the UVM config image so that it doesn't have to be downloaded each time.

gregorydemay

Thanks a lot @ninegua for this PR! I have a couple of minor comments but nothing critical.

gregorydemay · 2024-11-15T13:56:43Z

rs/tests/ckbtc/src/lib.rs

    InternetComputer::new()
-        .with_bitcoind_addr(SocketAddr::new(IpAddr::V6(btc_node_ipv6), 18444))
+        .with_bitcoind_addr(SocketAddr::new(IpAddr::V6(btc_node_ipv6), BITCOIN_CLI_PORT))
        .add_subnet(
            Subnet::new(SubnetType::System)


nit: shouldn't it be an application subnet since the ckBTC minter runs on pzp6e?

gregorydemay · 2024-11-15T14:40:19Z

rs/tests/ckbtc/src/lib.rs

@@ -390,7 +405,7 @@ pub async fn install_minter(
        min_confirmations: Some(BTC_MIN_CONFIRMATIONS as u32),


Here for a lack of a better place: Why do we use

ecdsa_key_name: TEST_KEY_LOCAL.parse().unwrap(),

?

Shouldn't we use a real tECDSA setup, meaning ecdsa_key_name should be something like key_1 or test_key_1? If that's the case, then it's probably better to address this as a separate PR/ticket, since it probably won't be sufficient to just change the key name (one has to activate tECDSA by proposal, similarly to what is described here)

gregorydemay · 2024-11-15T14:42:41Z

rs/tests/ckbtc/src/lib.rs

    );
    let kyt_init_args = NewKytArg::InitArg(NewKytInitArg {
        btc_network: BtcNetwork::Regtest { json_rpc_url },
-        kyt_mode: NewKytMode::AcceptAll,
+        kyt_mode: NewKytMode::Normal,


gregorydemay · 2024-11-15T14:48:58Z

rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs

@@ -230,9 +230,9 @@ pub async fn update_balance(
            utxo_statuses.push(UtxoStatus::ValueTooSmall(utxo));
            continue;
        }
-        let (uuid, status, kyt_provider) = kyt_check_utxo(caller_account.owner, &utxo).await?;
+        let status = kyt_check_utxo(&utxo).await?;


Here for a lack of a better place, we should update the value of kyt_fee used L.221 in the NNS upgrade proposal when we rollout this feature, correct? I don't remember, is the value going to be 0?

gregorydemay · 2024-11-15T14:51:33Z

rs/bitcoin/ckbtc/minter/src/updates/update_balance.rs

        mutate_state(|s| {
-            crate::state::audit::mark_utxo_checked(s, &utxo, uuid.clone(), status, kyt_provider);
+            crate::state::audit::mark_utxo_checked(s, &utxo, None, status, None);
        });
        if status == UtxoCheckStatus::Tainted {


nit: it would be preferable to match against the value of status to ensure by the compiler that we exhausted all variants

gregorydemay · 2024-11-15T15:16:49Z

rs/bitcoin/ckbtc/minter/src/state/eventlog.rs

                state.mark_utxo_checked(
                    utxo,
-                    uuid,
+                    if uuid.is_empty() { None } else { Some(uuid) },
                    UtxoCheckStatus::from_clean_flag(clean),
                    kyt_provider,


Similarly to the initial code, shouldn't we check that kyt_provider either corresponds to kyt_principal or to new_kyt_principal?

gregorydemay · 2024-11-15T15:28:22Z

rs/bitcoin/ckbtc/minter/src/state/eventlog.rs

-                    return Err(ReplayLogError::InconsistentLog(format!("Attempted to distribute {amount} to {kyt_provider}, causing an overdraft of {overdraft}")));
+                if Some(kyt_provider) != previous_kyt_principal {
+                    if let Err(Overdraft(overdraft)) =
+                        state.distribute_kyt_fee(kyt_provider, amount)


I'm a bit confused here. Shouldn't we only do this if State::new_kyt_principal.is_none()?

gregorydemay · 2024-11-15T15:29:07Z

rs/bitcoin/ckbtc/minter/src/tests.rs

-            kyt_principal: None,
-            new_kyt_principal: None
+            retrieve_btc_min_amount: 1000,
+            ..default_init_args()


much more readable, thanks!

gregorydemay · 2024-11-15T15:32:16Z

rs/bitcoin/ckbtc/minter/tests/tests.rs

        mode: Some(Mode::RestrictedTo(vec![authorized_principal])),
-        kyt_fee: None,
-        new_kyt_principal: Some(CanisterId::from(0)),


nit: Shouldn't we enforce in validate_config that either kyt_principal or new_kyt_principal is set, but not both?

gregorydemay · 2024-11-15T15:35:09Z

rs/tests/ckbtc/ckbtc_minter_kyt.rs

@@ -193,11 +173,10 @@ pub fn test_kyt(env: TestEnv) {
                );
            }
        }
-        start_canister(&kyt_canister).await;
+        start_canister(&new_kyt_canister).await;

        // Put the kyt canister into accept all utxos mode.


nit: I would remove this comment, IMO it doesn't add anything and is no longer actual.

Suggested change

// Put the kyt canister into accept all utxos mode.

github-actions bot added the feat label Oct 29, 2024

ninegua changed the title ~~feat(ckbtc): Use the new KYT canister in ckbtc withdrawal flow~~ feat(ckbtc): Use the new KYT canister in ckbtc deposit flow Oct 29, 2024

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-withdrawal branch from 8dba269 to c0302d0 Compare November 1, 2024 07:38

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch from f72fb2d to 950a000 Compare November 1, 2024 07:40

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-withdrawal branch from 27b6316 to 8362c92 Compare November 4, 2024 12:21

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch from 3211b0b to 7e73089 Compare November 5, 2024 04:30

ninegua mentioned this pull request Nov 5, 2024

feat(ckbtc): Use the new KYT canister in ckbtc withdrawal flow #2240

Merged

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch 2 times, most recently from 948c6d8 to ddbd46e Compare November 12, 2024 06:00

ninegua changed the base branch from paulliu/new-kyt-in-ckbtc-withdrawal to paulliu/check-address-before-burn November 12, 2024 06:00

ninegua force-pushed the paulliu/check-address-before-burn branch from 17fa74e to e3fa5ac Compare November 13, 2024 05:52

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch from ddd5024 to 4b68ab1 Compare November 13, 2024 06:01

ninegua force-pushed the paulliu/check-address-before-burn branch from e3fa5ac to 5ee77b5 Compare November 14, 2024 00:52

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch 4 times, most recently from 9baf221 to 1f6fc46 Compare November 15, 2024 03:38

ninegua added the CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 label Nov 15, 2024

Base automatically changed from paulliu/check-address-before-burn to master November 15, 2024 08:15

ninegua added 3 commits November 15, 2024 00:17

Call check_transaction on the new KYT canister for checking utxos

be5a8e1

Run kyt canister in Normal mode if possible. Setup SSL reverse proxy …

7c798bb

…for bitcoind so that https-outcall can go through.

Fix a replay compatibility problem

be92bbc

ninegua force-pushed the paulliu/new-kyt-in-ckbtc-deposit branch from 1f6fc46 to be92bbc Compare November 15, 2024 08:18

ninegua marked this pull request as ready for review November 15, 2024 08:28

ninegua requested review from a team as code owners November 15, 2024 08:28

github-actions bot added @cross-chain-team @idx labels Nov 15, 2024

ninegua requested review from THLO and gregorydemay November 15, 2024 08:33

basvandijk reviewed Nov 15, 2024

View reviewed changes

Fix tests

c3921ba

gregorydemay reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ckbtc): Use the new KYT canister in ckbtc deposit flow #2304

feat(ckbtc): Use the new KYT canister in ckbtc deposit flow #2304

ninegua commented Oct 29, 2024 •

edited

Loading

basvandijk Nov 15, 2024

basvandijk Nov 15, 2024

basvandijk Nov 15, 2024

gregorydemay left a comment

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

gregorydemay Nov 15, 2024

		@@ -390,7 +405,7 @@ pub async fn install_minter(
		min_confirmations: Some(BTC_MIN_CONFIRMATIONS as u32),

feat(ckbtc): Use the new KYT canister in ckbtc deposit flow #2304

Are you sure you want to change the base?

feat(ckbtc): Use the new KYT canister in ckbtc deposit flow #2304

Conversation

ninegua commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorydemay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninegua commented Oct 29, 2024 •

edited

Loading