fix(ledger): archive after replying instead of before

rs/ledger_suite/common/ledger_canister_core/src/archive.rs

@@ -63,6 +63,25 @@ impl<LA: LedgerAccess> LedgerArchivingGuard<LA> {
     }
 }
 
+impl<LA: LedgerAccess> Drop for LedgerArchivingGuard<LA> {
+    fn drop(&mut self) {
+        // Archiving that fails gracefully is counted where it fails, but
+        // archiving that *traps* cannot be: the trap discards everything the
+        // failing message did, including any attempt to record it. Destructors
+        // are the exception. They run while the task is being canceled, in the
+        // cleanup callback, whose state changes are kept — which is how the
+        // archiving lock below gets released as well.
+        //
+        // Without this, a ledger whose archiving keeps trapping looks exactly
+        // like a ledger that is not archiving because it has nothing to
+        // archive. Since archiving no longer holds up the reply, nobody else
+        // would notice either.
+        if ic_cdk::futures::is_recovering_from_trap() {
+            LA::with_ledger_mut(|ledger| ledger.increment_archiving_failure_metric());
+        }
+    }
+}
+
 pub enum ArchivingGuardError {
     /// There is no archive to lock, the archiving is disabled.
     NoArchive,

rs/ledger_suite/icp/ledger/BUILD.bazel

@@ -121,6 +121,7 @@ rust_ic_test(
         ":ledger-canister-wasm-allowance-getter",
         ":ledger-canister-wasm-next-version",
         ":ledger-canister-wasm-prev-version",
+        "//rs/universal_canister/impl:universal_canister.wasm.gz",
         "@mainnet_canisters//:ledger.wasm.gz",
     ],
     env = {
@@ -131,6 +132,7 @@ rust_ic_test(
         "LEDGER_CANISTER_ALLOWANCE_GETTER_WASM_PATH": "$(rootpath :ledger-canister-wasm-allowance-getter)",
         "LEDGER_CANISTER_NEXT_VERSION_WASM_PATH": "$(rootpath :ledger-canister-wasm-next-version)",
         "LEDGER_CANISTER_PREV_VERSION_WASM_PATH": "$(rootpath :ledger-canister-wasm-prev-version)",
+        "UNIVERSAL_CANISTER_WASM_PATH": "$(rootpath //rs/universal_canister/impl:universal_canister.wasm.gz)",
     },
     exec_properties = {"cpu": "4"},
     deps = [

rs/ledger_suite/icp/ledger/src/main.rs

@@ -189,6 +189,25 @@ fn init(
 /// * `to` - The account you want to send the funds to.
 /// * `created_at_time`: When the transaction has been created. If not set then
 ///   now is used.
+/// Starts archiving as a background task instead of awaiting it.
+///
+/// The ledger applies a transaction synchronously but can only archive by
+/// calling the archive canisters, so archiving has to await. Awaiting it before
+/// replying would make the reply depend on continuations that run *after* the
+/// transaction was committed: an await is a commit point, so a trap in one of
+/// them — the replica refusing a memory growth, for instance — cannot roll the
+/// transaction back, but it does turn the reply into a reject, which a caller
+/// cannot tell apart from a transaction that never happened.
+///
+/// Spawning puts archiving on its own chain of messages. The reply is produced
+/// in the same message that commits the transaction, and a failure while
+/// archiving can no longer contradict it — the blocks simply stay in the ledger
+/// until the next attempt.
+fn spawn_archiving() {
+    let max_msg_size = *MAX_MESSAGE_SIZE_BYTES.read().unwrap();
+    ic_cdk::futures::spawn(archive_blocks::<Access>(DebugOutSink, max_msg_size as u64));
+}
+
 async fn send(
     memo: Memo,
     amount: Tokens,
@@ -255,11 +274,10 @@ async fn send(
     };
     certified_data_set(hash.into_bytes());
 
-    // Don't put anything that could ever trap after this call or people using this
-    // endpoint. If something did panic the payment would appear to fail, but would
-    // actually succeed on chain.
-    let max_msg_size = *MAX_MESSAGE_SIZE_BYTES.read().unwrap();
-    archive_blocks::<Access>(DebugOutSink, max_msg_size as u64).await;
+    // Nothing after this point may trap: the payment is already committed, so a
+    // trap here would make it appear to fail while it actually succeeded on
+    // chain. Archiving is spawned rather than awaited for exactly that reason.
+    spawn_archiving();
     Ok(height)
 }
 
@@ -388,8 +406,7 @@ async fn icrc1_send(
         created_at_time,
     )?;
 
-    let max_msg_size = *MAX_MESSAGE_SIZE_BYTES.read().unwrap();
-    archive_blocks::<Access>(DebugOutSink, max_msg_size as u64).await;
+    spawn_archiving();
     Ok(block_index)
 }
 
@@ -1419,8 +1436,7 @@ fn icrc2_approve_not_async(
 async fn icrc2_approve(arg: ApproveArgs) -> Result<Nat, ApproveError> {
     let block_index = icrc2_approve_not_async(caller(), arg, None)?;
 
-    let max_msg_size = *MAX_MESSAGE_SIZE_BYTES.read().unwrap();
-    archive_blocks::<Access>(DebugOutSink, max_msg_size as u64).await;
+    spawn_archiving();
     Ok(block_index)
 }
 
@@ -1454,8 +1470,7 @@ async fn remove_approval(args: RemoveApprovalArgs) -> Result<Nat, ApproveError>
     });
     let block_index = icrc2_approve_not_async(caller(), approve_arg, Some(spender))?;
 
-    let max_msg_size = *MAX_MESSAGE_SIZE_BYTES.read().unwrap();
-    archive_blocks::<Access>(DebugOutSink, max_msg_size as u64).await;
+    spawn_archiving();
     Ok(block_index)
 }
 

rs/ledger_suite/icp/ledger/tests/tests.rs

@@ -2071,6 +2071,25 @@ fn test_archiving_respects_num_blocks_to_archive_upper_limit() {
     );
 }
 
+#[test]
+fn test_trapped_archiving_is_counted() {
+    ic_ledger_suite_state_machine_tests::subnet_memory::test_trapped_archiving_is_counted(
+        ledger_wasm(),
+        encode_init_args,
+        icp_archives,
+        ic_ledger_suite_state_machine_tests::archiving::query_encoded_blocks,
+    );
+}
+
+#[test]
+fn test_transfer_when_subnet_is_out_of_memory() {
+    ic_ledger_suite_state_machine_tests::subnet_memory::test_transfer_when_subnet_is_out_of_memory(
+        ledger_wasm(),
+        encode_init_args,
+        ic_ledger_suite_state_machine_tests::archiving::query_encoded_blocks,
+    );
+}
+
 #[test]
 fn test_archiving_fails_on_app_subnet_if_ledger_does_not_have_enough_cycles() {
     ic_ledger_suite_state_machine_tests::archiving::test_archiving_fails_on_app_subnet_if_ledger_does_not_have_enough_cycles(

rs/ledger_suite/icrc1/ledger/BUILD.bazel

@@ -462,3 +462,34 @@ rust_ic_test(
         "@crate_index//:candid",
     ],
 )
+
+rust_ic_test(
+    name = "ledger_archiving_atomicity_tests",
+    srcs = ["tests/archiving_atomicity_tests.rs"],
+    crate_features = [],
+    data = [
+        ":ledger_canister.wasm.gz",
+        "//rs/universal_canister/impl:universal_canister.wasm.gz",
+    ],
+    env = {
+        "IC_ICRC1_LEDGER_WASM_PATH": "$(rootpath :ledger_canister.wasm.gz)",
+        "UNIVERSAL_CANISTER_WASM_PATH": "$(rootpath //rs/universal_canister/impl:universal_canister.wasm.gz)",
+    },
+    deps = [
+        # Keep sorted.
+        ":ledger",
+        "//packages/icrc-ledger-types:icrc_ledger_types_storable",
+        "//rs/config",
+        "//rs/ledger_suite/common/ledger_canister_core",
+        "//rs/ledger_suite/test_utils/state_machine_helpers:ic-ledger-suite-state-machine-helpers",
+        "//rs/registry/subnet_type",
+        "//rs/state_machine_tests",
+        "//rs/types/base_types",
+        "//rs/types/cycles",
+        "//rs/types/management_canister_types",
+        "//rs/types/types",
+        "//rs/universal_canister/lib",
+        "@crate_index//:candid",
+        "@crate_index//:num-traits",
+    ],
+)

rs/ledger_suite/icrc1/ledger/src/main.rs

@@ -540,7 +540,26 @@ fn icrc1_total_supply() -> Nat {
     Access::with_ledger(|ledger| ledger.balances().total_supply().into())
 }
 
-async fn execute_transfer(
+/// Starts archiving as a background task instead of awaiting it.
+///
+/// The ledger applies a transaction synchronously but can only archive by
+/// calling the archive canisters, so archiving has to await. Awaiting it before
+/// replying would make the reply depend on continuations that run *after* the
+/// transaction was committed: an await is a commit point, so a trap in one of
+/// them — the replica refusing a memory growth, for instance — cannot roll the
+/// transaction back, but it does turn the reply into a reject. Callers cannot
+/// tell such a reject apart from one where nothing happened, and clients that
+/// retry on reject (the ck minters) would mint a deposit twice.
+///
+/// Spawning puts archiving on its own chain of messages. The reply is produced
+/// in the same message that commits the transaction, and a failure while
+/// archiving can no longer contradict it — the blocks simply stay in the ledger
+/// until the next attempt.
+fn spawn_archiving() {
+    ic_cdk::futures::spawn(archive_blocks::<Access>(&LOG, MAX_MESSAGE_SIZE));
+}
+
+fn execute_transfer(
     from_account: Account,
     to: Account,
     spender: Option<Account>,
@@ -559,11 +578,11 @@ async fn execute_transfer(
         created_at_time,
     )?;
 
-    // NB. we need to set the certified data before the first async call to make sure that the
+    // NB. we need to set the certified data before spawning the archiving to make sure that the
     // blockchain state agrees with the certificate while archiving is in progress.
     ic_cdk::api::certified_data_set(Access::with_ledger(Ledger::root_hash));
 
-    archive_blocks::<Access>(&LOG, MAX_MESSAGE_SIZE).await;
+    spawn_archiving();
     Ok(Nat::from(block_idx))
 }
 
@@ -687,7 +706,6 @@ async fn icrc1_transfer(arg: TransferArg) -> Result<Nat, TransferError> {
         arg.memo,
         arg.created_at_time,
     )
-    .await
     .map_err(convert_transfer_error)
     .map_err(|err| {
         let err: TransferError = match err.try_into() {
@@ -713,7 +731,6 @@ async fn icrc2_transfer_from(arg: TransferFromArgs) -> Result<Nat, TransferFromE
         arg.memo,
         arg.created_at_time,
     )
-    .await
     .map_err(convert_transfer_error)
     .map_err(|err| {
         let err: TransferFromError = match err.try_into() {
@@ -894,11 +911,11 @@ fn icrc2_approve_not_async(caller: Principal, arg: ApproveArgs) -> Result<u64, A
 async fn icrc2_approve(arg: ApproveArgs) -> Result<Nat, ApproveError> {
     let block_idx = icrc2_approve_not_async(ic_cdk::api::msg_caller(), arg)?;
 
-    // NB. we need to set the certified data before the first async call to make sure that the
+    // NB. we need to set the certified data before spawning the archiving to make sure that the
     // blockchain state agrees with the certificate while archiving is in progress.
     ic_cdk::api::certified_data_set(Access::with_ledger(Ledger::root_hash));
 
-    archive_blocks::<Access>(&LOG, MAX_MESSAGE_SIZE).await;
+    spawn_archiving();
     Ok(Nat::from(block_idx))
 }
 
@@ -991,7 +1008,7 @@ fn icrc152_mint_not_async(
 async fn icrc152_mint(args: Icrc152MintArgs) -> Result<Nat, Icrc152MintError> {
     let block_idx = icrc152_mint_not_async(ic_cdk::api::msg_caller(), args)?;
     ic_cdk::api::certified_data_set(Access::with_ledger(Ledger::root_hash));
-    archive_blocks::<Access>(&LOG, MAX_MESSAGE_SIZE).await;
+    spawn_archiving();
     Ok(Nat::from(block_idx))
 }
 
@@ -1089,7 +1106,7 @@ fn icrc152_burn_not_async(
 async fn icrc152_burn(args: Icrc152BurnArgs) -> Result<Nat, Icrc152BurnError> {
     let block_idx = icrc152_burn_not_async(ic_cdk::api::msg_caller(), args)?;
     ic_cdk::api::certified_data_set(Access::with_ledger(Ledger::root_hash));
-    archive_blocks::<Access>(&LOG, MAX_MESSAGE_SIZE).await;
+    spawn_archiving();
     Ok(Nat::from(block_idx))
 }