Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize UTXO handling with in-memory caching #417

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ab-oggm
Copy link

@ab-oggm ab-oggm commented Feb 16, 2025

  • Added utxo_cache in WalletStore to store UTXOs from listunspent RPC calls, reducing redundant network requests.
  • Implemented update_utxo_cache to update and maintain the cache efficiently using RwLock.
  • Overrode PartialEq for WalletStore since RwLock does not implement it, ensuring correct equality checks.
  • Updated get_all_utxo to leverage caching, reducing unnecessary RPC calls while keeping UTXO data fresh.

- Added `utxo_cache` in `WalletStore` to store UTXOs from `listunspent` RPC calls, reducing redundant network requests.
- Implemented `update_utxo_cache` to update and maintain the cache efficiently using `RwLock`.
- Overrode `PartialEq` for `WalletStore` since `RwLock` does not implement it, ensuring correct equality checks.
- Updated `get_all_utxo` to leverage caching, reducing unnecessary RPC calls while keeping UTXO data fresh.

This improves wallet performance and reduces RPC overhead. 🚀
@ab-oggm
Copy link
Author

ab-oggm commented Feb 16, 2025

Fixes #143

@ab-oggm
Copy link
Author

ab-oggm commented Feb 16, 2025

Currently, the UTXO state is stored in WalletStore, but there is no periodic refresh to call list_unspent. If makerd runs continuously, new transactions won't be reflected in the cache, which could impact calculations.

Should we add a periodic refresh mechanism to ensure the UTXO cache stays updated?

@mojoX911
Copy link

Should we add a periodic refresh mechanism to ensure the UTXO cache stays updated?

The utxo cache should update with every sync call. The makerd syncs in loop every 15 mins. Also after each spend tx, and coinswap it syncs. So it should be updated as long as we update the cache at syncs.

Add a sync call at maker/rpc/server.rs for callbacks of SendToAddress and ReedemFidelity.

Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good. Some things needs to be changed.

  • Don't use RwLock, not needed.
  • Update the cache at wallet::sync calls.
  • The cache should include <ListUnspentResultEntry, UTXOSpendInfo>. Checkj the list_all_utxo_spend_info function for how to generate the spend infos from the utxos.
  • Add a sync call at maker's SendToAddres and RedeemFidelity rpc handlers.

Comment on lines 623 to 628
let cache = self.store.utxo_cache.read().unwrap();
if !cache.is_empty() {
log::info!("Using cached UTXO data.");
return Ok(cache.values().cloned().collect());
}
} // Read lock is dropped here to allow a write lock later

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check, if its empty it will return empty vec, which is fine.

Comment on lines 630 to 637
self.rpc.unlock_unspent_all()?;
let all_utxos = self
.rpc
.list_unspent(Some(0), Some(9999999), None, None, None)?;

// Acquire write lock to update the cache
self.store.update_utxo_cache(all_utxos.clone());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the store in wallet::sync() function, not here.

@@ -40,6 +42,9 @@ pub(crate) struct WalletStore {
pub(super) last_synced_height: Option<u64>,

pub(super) wallet_birthday: Option<u64>,

#[serde(default)] // Ensures deserialization works if `utxo_cache` is missing
pub(super) utxo_cache: RwLock<HashMap<OutPoint, ListUnspentResultEntry>>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a RwLock here, as the wallet is already wrapped in it at maker's multithreaded code. So just simple hashmap will work.

Copy link
Author

@ab-oggm ab-oggm Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense, since only one makerd can run from a single wallet.

- Added `utxo_cache` in `WalletStore` as HashMap<OutPoint, (ListUnspentResultEntry, UTXOSpendInfo)>
- `update_utxo_cache_internal` fn for updating the cache
- `build_utxo_list_from_cache` to build original list of utxo from cache(mathches the output format for rpc call `list_unspent`)
- `refresh_utxo_cache` added inside `wallet/api.rs` to refresh the cache with wallet's internal spend info
- added wallet sync in `SendToAddress` and `ReedemFidelity`.
- Added `utxo_cache` in `WalletStore` as HashMap<OutPoint, (ListUnspentResultEntry, UTXOSpendInfo)>
- `update_utxo_cache_internal` fn for updating the cache
- `build_utxo_list_from_cache` to build original list of utxo from cache(mathches the output format for rpc call `list_unspent`)
- `refresh_utxo_cache` added inside `wallet/api.rs` to refresh the cache with wallet's internal spend info
- added wallet sync in `SendToAddress` and `ReedemFidelity`.
@ab-oggm
Copy link
Author

ab-oggm commented Feb 19, 2025

The utxo cache should update with every sync call. The makerd syncs in loop every 15 mins. Also after each spend tx, and coinswap it syncs. So it should be updated as long as we update the cache at syncs.

yeah having the cache updates inside the wallet sync call keeps the utxo list updated

@ab-oggm
Copy link
Author

ab-oggm commented Feb 19, 2025

  • One more issue I observed was updating utxo_cache to HashMap<OutPoint, (ListUnspentResultEntry, UTXOSpendInfo)>, raised error when trying to run makerd from a already known wallet.
  • Mainly beacuse while loading wallet from path we use serde_cbor::from_slice::<Self>(&reader) to deserialize . Since the structure was changed, the new deserialzed doesn't match.
  • Either run from a new wallet or rename utxo_cache to new name like utxo_cache_1 with #[serde(default)]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants