fix: Improve listing of ICRC-1 transactions

common/src/cache_transactions.rs

@@ -10,19 +10,13 @@ use std::collections::BTreeMap;
 
 use crate::FundsTransfer;
 
-const CACHE_MAX_LENGTH: usize = 100_000; // Keep at most 100k entries with the highest ids in the cache
+const CACHE_MAX_LENGTH: usize = 100_000_000; // Keep at most 100M entries with the highest ids in the cache
 
 thread_local! {
-    /// Total count of transactions that were committed and are in stable memory
-    pub static CACHE_TXS_NUM_COMMITTED: RefCell<u64> = const { RefCell::new(0) };
-    /// Recently committed transactions, that can be served from the get_transactions endpoint
+    /// Recently committed transactions, to serve from the get_transactions endpoint
     static RECENT_CACHE: RefCell<BTreeMap<u64, Transaction>> = const { RefCell::new(BTreeMap::new()) };
 }
 
-pub fn get_ledger_txs_num_committed() -> u64 {
-    CACHE_TXS_NUM_COMMITTED.with(|n| *n.borrow())
-}
-
 /// Caches up to <CACHE_MAX_LENGTH> entries with the highest entry number.
 /// The entry key can be the transaction number or the block number, and we keep in the cache
 /// the entries with the highest entry number
@@ -37,24 +31,31 @@ impl RecentCache {
         })
     }
 
+    /// Append transaction to the cache
+    pub fn append_entry(tx: Transaction) {
+        RECENT_CACHE.with(|cache| {
+            let mut cache = cache.borrow_mut();
+            let tx_num = cache.keys().next_back().unwrap_or(&0) + 1;
+            Self::_add_entry(&mut cache, tx_num, tx)
+        })
+    }
+
     fn _add_entry(cache: &mut BTreeMap<u64, Transaction>, tx_num: u64, tx: Transaction) {
         if cache.len() < CACHE_MAX_LENGTH || tx_num > *cache.keys().next().unwrap_or(&0) {
             // Only insert the entry if the cache is not full or if the new entry has a higher id than the minimal
             cache.insert(tx_num, tx);
         }
-
-        // If the number of entries exceeds the maximum length, remove the oldest entries
-        while cache.len() > CACHE_MAX_LENGTH {
-            if let Some((&first_key, _)) = cache.iter().next() {
-                cache.remove(&first_key);
-            }
-        }
     }
 
     pub fn get_min_tx_num() -> Option<u64> {
         RECENT_CACHE.with(|cache| cache.borrow().keys().next().copied())
     }
 
+    pub fn get_max_tx_num() -> Option<u64> {
+        RECENT_CACHE.with(|cache| cache.borrow().keys().next_back().copied())
+    }
+
+    // Get the current size of the cache, in number of transactions
     pub fn get_num_entries() -> usize {
         RECENT_CACHE.with(|cache| cache.borrow().len())
     }
@@ -87,11 +88,6 @@ impl RecentCache {
         });
     }
 
-    // Get the current size of the cache
-    pub fn cache_size() -> usize {
-        RECENT_CACHE.with(|cache| cache.borrow().len())
-    }
-
     /// Parse transactions from a LedgerBlock and append transactions to the cache.
     /// tx_num_start is the lowest transaction number in the block.
     /// ledger_block is the LedgerBlock that contains the transactions.
@@ -110,6 +106,20 @@ impl RecentCache {
             }
         });
     }
+
+    /// Ensure the cache does not exceed the maximum length.
+    pub fn ensure_cache_length() {
+        RECENT_CACHE.with(|cache| {
+            let mut cache = cache.borrow_mut();
+
+            // If the number of entries exceeds the maximum length, remove the oldest entries
+            while cache.len() > CACHE_MAX_LENGTH {
+                if let Some((&first_key, _)) = cache.iter().next() {
+                    cache.remove(&first_key);
+                }
+            }
+        });
+    }
 }
 
 #[cfg(test)]
@@ -158,8 +168,10 @@ mod tests {
         for i in 0..CACHE_MAX_LENGTH + 10 {
             RecentCache::add_entry(i as u64, create_dummy_transaction(i as u64));
         }
+        assert!(RecentCache::get_num_entries() >= CACHE_MAX_LENGTH);
+        RecentCache::ensure_cache_length();
 
-        assert_eq!(RecentCache::cache_size(), CACHE_MAX_LENGTH);
+        assert_eq!(RecentCache::get_num_entries(), CACHE_MAX_LENGTH);
         assert_eq!(RecentCache::get_min_tx_num(), Some(10));
     }
 
@@ -181,10 +193,10 @@ mod tests {
         RecentCache::clear_cache();
         RecentCache::add_entry(1, create_dummy_transaction(1));
 
-        assert_eq!(RecentCache::cache_size(), 1);
+        assert_eq!(RecentCache::get_num_entries(), 1);
         let removed_tx = RecentCache::remove_transaction(1);
         assert!(removed_tx.is_some());
-        assert_eq!(RecentCache::cache_size(), 0);
+        assert_eq!(RecentCache::get_num_entries(), 0);
     }
 
     #[test]
@@ -194,9 +206,9 @@ mod tests {
             RecentCache::add_entry(i, create_dummy_transaction(i));
         }
 
-        assert_eq!(RecentCache::cache_size(), 5);
+        assert_eq!(RecentCache::get_num_entries(), 5);
         RecentCache::clear_cache();
-        assert_eq!(RecentCache::cache_size(), 0);
+        assert_eq!(RecentCache::get_num_entries(), 0);
     }
 
     #[test]
@@ -239,7 +251,7 @@ mod tests {
         // Pretend that the first free transaction number is 899
         RecentCache::parse_ledger_block(899, &ledger_block);
 
-        assert_eq!(RecentCache::cache_size(), 103);
+        assert_eq!(RecentCache::get_num_entries(), 103);
         assert_eq!(RecentCache::get_min_tx_num(), Some(899));
         assert!(RecentCache::get_transaction(898).is_none());
         assert!(RecentCache::get_transaction(899).is_some());

common/src/ledger_refresh.rs

@@ -6,10 +6,10 @@ use crate::{
     contracts_cache_open_remove, dcc_identity, error, reputations_apply_aging,
     reputations_apply_changes, reputations_clear, set_num_providers, set_num_users,
     set_offering_num_per_provider, AHashMap, ContractSignRequest, ContractSignRequestPayload,
-    ReputationAge, ReputationChange, UpdateOfferingPayload, CACHE_TXS_NUM_COMMITTED,
-    LABEL_CONTRACT_SIGN_REPLY, LABEL_CONTRACT_SIGN_REQUEST, LABEL_DC_TOKEN_APPROVAL,
-    LABEL_DC_TOKEN_TRANSFER, LABEL_NP_OFFERING, LABEL_NP_REGISTER, LABEL_REPUTATION_AGE,
-    LABEL_REPUTATION_CHANGE, LABEL_USER_REGISTER, PRINCIPAL_MAP,
+    ReputationAge, ReputationChange, UpdateOfferingPayload, LABEL_CONTRACT_SIGN_REPLY,
+    LABEL_CONTRACT_SIGN_REQUEST, LABEL_DC_TOKEN_APPROVAL, LABEL_DC_TOKEN_TRANSFER,
+    LABEL_NP_OFFERING, LABEL_NP_REGISTER, LABEL_REPUTATION_AGE, LABEL_REPUTATION_CHANGE,
+    LABEL_USER_REGISTER, PRINCIPAL_MAP,
 };
 use borsh::BorshDeserialize;
 use candid::Principal;
@@ -26,7 +26,6 @@ pub fn refresh_caches_from_ledger(ledger: &LedgerMap) -> anyhow::Result<()> {
     let mut replayed_blocks = 0;
     account_balances_clear();
     reputations_clear();
-    let mut num_txs = 0u64;
     let mut num_providers = 0u64;
     let mut num_users = 0u64;
     let mut principals: AHashMap<Principal, Vec<u8>> = HashMap::default();
@@ -73,8 +72,7 @@ pub fn refresh_caches_from_ledger(ledger: &LedgerMap) -> anyhow::Result<()> {
                         account_balance_add(transfer.to(), transfer.amount())?;
                     }
 
-                    RecentCache::add_entry(num_txs, transfer.into());
-                    num_txs += 1;
+                    RecentCache::append_entry(transfer.into());
                 }
                 LABEL_DC_TOKEN_APPROVAL => {
                     let approval =
@@ -151,10 +149,13 @@ pub fn refresh_caches_from_ledger(ledger: &LedgerMap) -> anyhow::Result<()> {
         }
         replayed_blocks += 1;
     }
-    CACHE_TXS_NUM_COMMITTED.with(|n| *n.borrow_mut() = num_txs);
     PRINCIPAL_MAP.with(|p| *p.borrow_mut() = principals);
     set_num_providers(num_providers);
     set_num_users(num_users);
-    debug!("Refreshed caches from {} ledger blocks", replayed_blocks);
+    debug!(
+        "Refreshed caches from {} ledger blocks, found {} transactions",
+        replayed_blocks,
+        RecentCache::get_max_tx_num().unwrap_or_default()
+    );
     Ok(())
 }

ic-canister/src/canister_backend/generic.rs

@@ -1,15 +1,12 @@
 use super::pre_icrc3::ledger_construct_hash_tree;
-use borsh::BorshDeserialize;
 use candid::Principal;
-use dcc_common::cache_transactions::RecentCache;
 use dcc_common::{
     account_balance_get, account_registration_fee_e9s, blocks_until_next_halving, cursor_from_data,
     get_account_from_pubkey, get_num_offerings, get_num_providers, get_pubkey_from_principal,
     refresh_caches_from_ledger, reputation_get, reward_e9s_per_block_recalculate,
     rewards_current_block_checked_in, rewards_distribute, rewards_pending_e9s, set_test_config,
-    ContractId, ContractReqSerialized, FundsTransfer, LedgerCursor, TokenAmountE9s,
-    BLOCK_INTERVAL_SECS, CACHE_TXS_NUM_COMMITTED, DATA_PULL_BYTES_BEFORE_LEN,
-    LABEL_CONTRACT_SIGN_REQUEST, LABEL_DC_TOKEN_TRANSFER, LABEL_NP_CHECK_IN, LABEL_NP_OFFERING,
+    ContractId, ContractReqSerialized, LedgerCursor, TokenAmountE9s, BLOCK_INTERVAL_SECS,
+    DATA_PULL_BYTES_BEFORE_LEN, LABEL_CONTRACT_SIGN_REQUEST, LABEL_NP_CHECK_IN, LABEL_NP_OFFERING,
     LABEL_NP_PROFILE, LABEL_NP_REGISTER, LABEL_REWARD_DISTRIBUTION, LABEL_USER_REGISTER,
     MAX_RESPONSE_BYTES_NON_REPLICATED,
 };
@@ -21,7 +18,6 @@ use ledger_map::{error, info, warn, LedgerMap};
 use serde::Serialize;
 use std::cell::RefCell;
 use std::io::prelude::*;
-use std::ops::AddAssign;
 use std::time::Duration;
 
 thread_local! {
@@ -71,25 +67,7 @@ fn ledger_periodic_task() {
             // Intentionally don't panic. If needed, transactions can be replayed and corrected.
         }
 
-        let mut tx_num = CACHE_TXS_NUM_COMMITTED.with(|n| *n.borrow());
-        for entry in ledger.next_block_iter(Some(LABEL_DC_TOKEN_TRANSFER)) {
-            let transfer: FundsTransfer = BorshDeserialize::try_from_slice(entry.value())
-                .unwrap_or_else(|e| {
-                    ic_cdk::api::trap(&format!(
-                        "Failed to deserialize transfer {:?} ==> {:?}",
-                        entry, e
-                    ));
-                });
-            RecentCache::add_entry(tx_num, transfer.into());
-            tx_num += 1;
-        }
-
-        // Uncommitted transactions now get committed -- adjust the (cache) count of total committed transactions
-        let count_total_txs_uncommitted =
-            ledger.get_next_block_entries_count(Some(LABEL_DC_TOKEN_TRANSFER)) as u64;
-
-        CACHE_TXS_NUM_COMMITTED.with(|n| n.borrow_mut().add_assign(count_total_txs_uncommitted));
-
+        // Commit the block
         ledger.commit_block().unwrap_or_else(|e| {
             error!("Failed to commit ledger: {}", e);
         });
@@ -98,6 +76,9 @@ fn ledger_periodic_task() {
         // Borrowed from https://github.com/ldclabs/ic-sft/blob/4825d760811731476ffbbb1705295a6ad4aae58f/src/ic_sft_canister/src/store.rs#L193-L210
         let root_hash = ledger_construct_hash_tree(ledger).digest();
         ic_cdk::api::set_certified_data(&root_hash);
+
+        // Cleanup old transactions that are used for deduplication
+        crate::canister_backend::icrc1::cleanup_old_transactions();
     });
 }
 

ic-canister/src/canister_backend/icrc1.rs

@@ -9,31 +9,28 @@ use ic_cdk::println;
 
 use crate::canister_backend::generic::LEDGER_MAP;
 use crate::DC_TOKEN_LOGO;
+use crate::{
+    DC_TOKEN_DECIMALS, DC_TOKEN_NAME, DC_TOKEN_SYMBOL, DC_TOKEN_TOTAL_SUPPLY,
+    DC_TOKEN_TRANSFER_FEE_E9S, MEMO_BYTES_MAX, MINTING_ACCOUNT, MINTING_ACCOUNT_ICRC1,
+};
 use candid::types::number::Nat;
 use candid::{CandidType, Principal};
 use dcc_common::{
-    account_balance_get, fees_sink_accounts, get_timestamp_ns, ledger_funds_transfer,
-    nat_to_balance, FundsTransfer, IcrcCompatibleAccount, TokenAmountE9s, PERMITTED_DRIFT,
-    TX_WINDOW,
+    account_balance_get, cache_transactions::RecentCache, fees_sink_accounts, get_timestamp_ns,
+    ledger_funds_transfer, nat_to_balance, AHashMap, FundsTransfer, IcrcCompatibleAccount,
+    TokenAmountE9s, PERMITTED_DRIFT, TX_WINDOW,
 };
 use ic_cdk::caller;
 use icrc_ledger_types::icrc::generic_metadata_value::MetadataValue;
 use icrc_ledger_types::icrc1::account::Account as Icrc1Account;
 use icrc_ledger_types::icrc1::transfer::{Memo as Icrc1Memo, TransferError as Icrc1TransferError};
 use serde::{Deserialize, Serialize};
 use sha2::Digest;
-
-use crate::{
-    DC_TOKEN_DECIMALS, DC_TOKEN_NAME, DC_TOKEN_SYMBOL, DC_TOKEN_TOTAL_SUPPLY,
-    DC_TOKEN_TRANSFER_FEE_E9S, MEMO_BYTES_MAX, MINTING_ACCOUNT, MINTING_ACCOUNT_ICRC1,
-};
-
 use std::cell::RefCell;
-use std::collections::HashMap;
 
 // Store transactions with their timestamps for cleanup
 thread_local! {
-    static RECENT_TRANSACTIONS: RefCell<HashMap<Vec<u8>, u64>> = RefCell::new(HashMap::new());
+    static RECENT_TRANSACTIONS: RefCell<AHashMap<Vec<u8>, u64>> = RefCell::new(AHashMap::default());
 }
 
 fn compute_tx_hash(caller: Principal, arg: &TransferArg) -> Vec<u8> {
@@ -55,8 +52,9 @@ fn compute_tx_hash(caller: Principal, arg: &TransferArg) -> Vec<u8> {
     hasher.finalize().to_vec()
 }
 
-fn cleanup_old_transactions(now: u64) {
+pub fn cleanup_old_transactions() {
     RECENT_TRANSACTIONS.with(|transactions| {
+        let now = get_timestamp_ns();
         let mut txs = transactions.borrow_mut();
         txs.retain(|_, timestamp| *timestamp > now.saturating_sub(TX_WINDOW));
     });
@@ -72,9 +70,6 @@ fn check_duplicate_transaction(
 
     let tx_hash = compute_tx_hash(caller, arg);
 
-    // First cleanup old transactions
-    cleanup_old_transactions(now);
-
     RECENT_TRANSACTIONS.with(|transactions| {
         let mut txs = transactions.borrow_mut();
 
@@ -221,24 +216,27 @@ pub fn _icrc1_transfer(arg: TransferArg) -> Result<Nat, Icrc1TransferError> {
         // It's safe to subtract here because we checked above that the balance will not be negative
         let balance_from_after = balance_from_after.saturating_sub(fee);
         let mut ledger_ref = ledger.borrow_mut();
-        ledger_funds_transfer(
-            &mut ledger_ref,
-            FundsTransfer::new(
-                from,
-                to,
-                Some(fee),
-                Some(fees_sink_accounts()),
-                Some(arg.created_at_time.unwrap_or(get_timestamp_ns())),
-                arg.memo.unwrap_or_default().0.into_vec(),
-                amount,
-                balance_from_after,
-                balance_to_after,
-            ),
-        )
-        .unwrap_or_else(|err| ic_cdk::trap(&err.to_string()));
-
-        let block_count = ledger_ref.get_blocks_count();
-        Ok(block_count.into())
+        let transfer = FundsTransfer::new(
+            from,
+            to,
+            Some(fee),
+            Some(fees_sink_accounts()),
+            Some(arg.created_at_time.unwrap_or(get_timestamp_ns())),
+            arg.memo.unwrap_or_default().0.into_vec(),
+            amount,
+            balance_from_after,
+            balance_to_after,
+        );
+        ledger_funds_transfer(&mut ledger_ref, transfer.clone())
+            .unwrap_or_else(|err| ic_cdk::trap(&err.to_string()));
+
+        RecentCache::append_entry(transfer.into());
+        RecentCache::get_max_tx_num().map(Nat::from).ok_or_else(|| {
+            Icrc1TransferError::GenericError {
+                error_code: Nat::from(10000u32),
+                message: "Failed to get max transaction number".to_string(),
+            }
+        })
     })
 }
 

ic-canister/src/canister_backend/pre_icrc3.rs

@@ -1,10 +1,7 @@
 use super::generic::LEDGER_MAP;
 use crate::canister_backend::generic::encode_to_cbor_bytes;
 use borsh::BorshDeserialize;
-use dcc_common::{
-    cache_transactions::RecentCache, get_ledger_txs_num_committed, FundsTransfer,
-    LABEL_DC_TOKEN_TRANSFER,
-};
+use dcc_common::{cache_transactions::RecentCache, FundsTransfer, LABEL_DC_TOKEN_TRANSFER};
 use ic_certification::{HashTreeNode, Label};
 use icrc_ledger_types::icrc3::blocks::DataCertificate as DataCertificatePreIcrc3;
 use icrc_ledger_types::icrc3::transactions::{
@@ -23,12 +20,6 @@ pub fn _get_transactions(req: GetTransactionsRequest) -> GetTransactionsResponse
         .as_start_and_length()
         .unwrap_or_else(|msg| ic_cdk::api::trap(&msg));
 
-    let count_total_txs_committed = get_ledger_txs_num_committed();
-    let count_total_txs_uncommitted = LEDGER_MAP.with(|ledger| {
-        ledger
-            .borrow()
-            .get_next_block_entries_count(Some(LABEL_DC_TOKEN_TRANSFER))
-    }) as u64;
     let mut txs = _get_committed_transactions(txs_from, txs_length);
     let txs_missing = txs_length.saturating_sub(txs.len() as u64);
     if txs_missing > 0 {
@@ -38,7 +29,7 @@ pub fn _get_transactions(req: GetTransactionsRequest) -> GetTransactionsResponse
     GetTransactionsResponse {
         // We don't have archived transactions in this implementation, so the first_index is always the requested tx number
         first_index: txs_from.into(),
-        log_length: (count_total_txs_committed + count_total_txs_uncommitted).into(),
+        log_length: RecentCache::get_max_tx_num().unwrap_or_default().into(),
         transactions: txs,
         archived_transactions: vec![],
     }