fix: potential cache issues #7623
Conversation
WalkthroughA single-file optimization to the sync UTXOs handler that conditionally adapts caching duration based on the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
applications/minotari_node/src/http/handler/sync_utxos_by_block.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: SWvheerden
Repo: tari-project/tari PR: 7300
File: base_layer/wallet/src/transaction_service/service.rs:1663-1663
Timestamp: 2025-07-10T13:21:46.666Z
Learning: In the Tari wallet codebase, scanned height persistence is handled by the UTXO scanner service, not individual services like TransactionService. Services that need current scanned height should read it via `get_last_scanned_height()` rather than persisting it themselves. This follows separation of concerns architecture pattern.
📚 Learning: 2025-08-11T15:37:17.354Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7387
File: base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs:2896-2927
Timestamp: 2025-08-11T15:37:17.354Z
Learning: The `update_accumulated_difficulty` method in `base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs` is migration-specific code that only runs during a once-off migration when existing ChainHeader data is present in LMDB. The consistency guarantees are upheld by the calling function `process_accumulated_data_for_height`, so additional validation within this method is redundant.
Applied to files:
applications/minotari_node/src/http/handler/sync_utxos_by_block.rs
📚 Learning: 2025-08-18T14:27:34.527Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7421
File: base_layer/core/src/base_node/sync/header_sync/synchronizer.rs:774-781
Timestamp: 2025-08-18T14:27:34.527Z
Learning: In the header sync synchronizer (base_layer/core/src/base_node/sync/header_sync/synchronizer.rs), non-banable errors from the sync process are handled by the calling function, not within the sync methods themselves. This means that internal state errors like `current_valid_chain_tip_header()` returning `None` will be properly categorized and handled upstream without incorrectly banning peers.
Applied to files:
applications/minotari_node/src/http/handler/sync_utxos_by_block.rs
🧬 Code graph analysis (1)
applications/minotari_node/src/http/handler/sync_utxos_by_block.rs (1)
applications/minotari_merge_mining_proxy/src/block_template_manager.rs (1)
height(339-341)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
- GitHub Check: ci
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: cargo check with msrv
- GitHub Check: wasm build tests
- GitHub Check: ledger build tests
🔇 Additional comments (2)
applications/minotari_node/src/http/handler/sync_utxos_by_block.rs (2)
4-4: LGTM: Import added for new min comparison.The
std::cmpimport is necessary for thecmp::mincall on line 115.
90-90: LGTM: Variable extraction improves readability.Extracting
exclude_spentinto a local variable is reasonable for code clarity and reuse in the subsequent conditional logic.
| let last_height = if let true = exclude_spent { | ||
| // if we're excluding spent outputs we cannot cache for longer than a day, so we force the "height" so that the | ||
| // caching will only be a max of 1 day. Wallets will sent full without excluding spends the last 1000 blocks so | ||
| // this leaves a margin for wallets to get data if it changed. | ||
| cmp::min(height.unwrap_or(0), tip_height.saturating_add(2500)) | ||
| } else { | ||
| height.unwrap_or(0) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the apply_cache_control function and surrounding context
fd -e rs -x grep -l "apply_cache_control" {}Repository: tari-project/tari
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Search for apply_cache_control implementation
rg -n "fn apply_cache_control" -A 25 -B 2 --type rustRepository: tari-project/tari
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Search for how tip_height and last_height are used in cache calculations
rg -n "cache.*control|max-age|Cache-Control" -A 5 -B 5 --type rustRepository: tari-project/tari
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Get the full context of the sync_utxos_by_block.rs file around the flagged section
wc -l applications/minotari_node/src/http/handler/sync_utxos_by_block.rsRepository: tari-project/tari
Length of output: 178
Fix inverted clamping logic in cache control calculation.
The current code clamps last_height to at most tip_height + 2500:
cmp::min(height.unwrap_or(0), tip_height.saturating_add(2500))In typical operation where height <= tip_height, this returns height unchanged—providing no cache duration limiting effect.
To limit cache to approximately 1 day when exclude_spent is true, clamp last_height to at least tip_height - 2500:
cmp::max(height.unwrap_or(0), tip_height.saturating_sub(2500))This ensures old blocks are treated as closer to the tip, reducing cache TTL as intended.
Also simplify the condition: replace if let true = exclude_spent with if exclude_spent.
🤖 Prompt for AI Agents
In applications/minotari_node/src/http/handler/sync_utxos_by_block.rs around
lines 111 to 118, the cache clamping logic is inverted and the condition is
verbose: change the branch to use `if exclude_spent` (remove `if let true =
...`) and compute last_height by clamping upwards with
cmp::max(height.unwrap_or(0), tip_height.saturating_sub(2500)) so that when
excluding spent outputs old heights are treated as at least tip_height - 2500
(reducing cache TTL as intended).
| // if we're excluding spent outputs we cannot cache for longer than a day, so we force the "height" so that the | ||
| // caching will only be a max of 1 day. Wallets will sent full without excluding spends the last 1000 blocks so | ||
| // this leaves a margin for wallets to get data if it changed. |
There was a problem hiding this comment.
Fix comment typo and clarify block count.
The comment has a grammatical error and an inconsistency:
- "Wallets will sent full" should be "Wallets will send full"
- The comment mentions "the last 1000 blocks" but the code uses 2500
Please clarify whether 2500 blocks is correct (approximately 1 day) and update the comment accordingly.
🤖 Prompt for AI Agents
In applications/minotari_node/src/http/handler/sync_utxos_by_block.rs around
lines 112 to 114, fix the typo and clarify the block count in the comment:
change "Wallets will sent full" to "Wallets will send full" and update "the last
1000 blocks" to "the last 2500 blocks (~1 day)" to match the code using 2500 and
explain that 2500 is approximately one day for clarity.
1 participant
Description
Excluding spends, can cause an issue with spends as those older blocks are cached for long periods, where as the ouputs can be spent any block.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.