Repository navigation
Stop caching every mempool tx for bitcoind #1216
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1270,15 +1270,16 @@ impl BitcoindClient { | |
|
|
||
| let mempool_entries_cache = mempool_entries_cache.lock().await; | ||
| let mut mempool_txs_cache = mempool_txs_cache.lock().await; | ||
| mempool_txs_cache.retain(|txid, _| mempool_entries_cache.contains_key(txid)); | ||
|
|
||
| if let Some(difference) = | ||
| mempool_entries_cache.len().checked_sub(mempool_txs_cache.capacity()) | ||
| { | ||
| mempool_txs_cache.reserve(difference) | ||
| } | ||
| // Only transactions we may have to emit again are cached, i.e., those that entered the | ||
| // mempool above our best processed height. Caching every mempool transaction would keep a | ||
| // full copy of the mempool in memory. | ||
| mempool_txs_cache.retain(|txid, _| { | ||
| mempool_entries_cache | ||
| .get(txid) | ||
| .is_some_and(|entry| entry.height > best_processed_height) | ||
| }); | ||
|
|
||
| let mut txs_to_emit = Vec::with_capacity(mempool_entries_cache.len()); | ||
| let mut txs_to_emit = Vec::new(); | ||
| for (txid, entry) in mempool_entries_cache.iter() { | ||
| if entry.time > latest_time { | ||
| latest_time = entry.time; | ||
|
|
@@ -1302,7 +1303,9 @@ impl BitcoindClient { | |
|
|
||
| match self.get_raw_transaction(&entry.txid).await { | ||
| Ok(Some(tx)) => { | ||
| mempool_txs_cache.insert(entry.txid, (tx.clone(), entry.time)); | ||
| if !ancestor_within_height { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One concern with caching only In a quick model (20k-entry mempool at startup, transient failure per RPC), a 1-in-10,000 failure rate went from ~2.9 polls / ~20k RPCs with the old code to ~6.7 polls / ~58k RPCs with this PR, and the cost grows quickly with mempool size and error rate. What we need to keep is exactly the set that may still be emitted, so the insert could be: if !ancestor_within_height || entry.time > prev_mempool_time {
mempool_txs_cache.insert(entry.txid, (tx.clone(), entry.time));
}Steady-state memory stays small, since a successful poll advances |
||
| mempool_txs_cache.insert(entry.txid, (tx.clone(), entry.time)); | ||
| } | ||
| txs_to_emit.push((tx, entry.time)); | ||
| }, | ||
| Ok(None) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To go with the insert condition suggested above, the
retaincould keep an entry while it may still need emitting, i.e. the same condition as the skip check further down:prev_mempool_timeis already loaded at the top of this function. After a successful polllatest_mempool_timestampadvances, so the nextretaindrops everything that was only kept for the retry, and the steady-state cache stays as small as with the current PR. I haven't compiled this against your branch, so please double-check it.