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

Tx cache #110

Draft
wants to merge 10 commits into
base: new-index
Choose a base branch
from
Draft

Tx cache #110

wants to merge 10 commits into from

Conversation

RCasatta
Copy link
Collaborator

Introduce a tx cache for the tx cache lookup.

The same crate for the cache has been introduced by romanz electrs, it's main advantages are that is a contiguous memory region capped in size.

The hypothesis is that with a big enough cache it's possible to run a public instance in lightmode, without duplicating tx data which is already present in the bitcoin node.

it is less interesting to see how many rows are written in the db and
more interesting knowing the last height indexed
Copy link
Collaborator

@shesek shesek left a comment

Choose a reason for hiding this comment

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

The hypothesis is that with a big enough cache it's possible to run a public instance in lightmode, without duplicating tx data which is already present in the bitcoin node.

When lightmode is disabled, is there still an advantage to using a cache in addition to having the txs available in rocksdb?

@@ -424,7 +424,7 @@ impl Daemon {
loop {
match self.handle_request_batch(method, params_list) {
Err(Error(ErrorKind::Connection(msg), _)) => {
warn!("reconnecting to bitcoind: {}", msg);
warn!("reconnecting to bitcoind: {msg}\nmethod was:{method}\nwith_params:{params_list:?}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to include this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, just a draft...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And by the way i was trying to undersand why was failing in my test run but the log, even changed, wasn't helping, in the end the error was "rpc queue limit reached"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Were you running with #89? If so, note that it requires adjusting bitcoind's rpcworkqueue and rpcthreads options upwards.

@@ -838,7 +851,16 @@ impl ChainQuery {
pub fn lookup_raw_txn(&self, txid: &Txid, blockhash: Option<&BlockHash>) -> Option<Bytes> {
let _timer = self.start_timer("lookup_raw_txn");

if self.light_mode {
if let Ok(cache) = self.txs_cache.lock() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it expected that locking may sometime fail and that the failure should be ignored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think is expected to fail, but since here everything would work normally in case of failure (just perf degradation) I preferred this way

Copy link
Collaborator

Choose a reason for hiding this comment

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

A poisoned mutex is likely to indicate some underlying coding issue, wouldn't it be better to let it error visibly?

I would at least log a warn-level message instead of silently ignoring it, but generally my preference is the "fail fast" approach for unanticipated errors. Failing fast (and restarting) would also re-enable the tx cache, instead of having it continue with degraded performance (until the process eventually restarts for another reason).

@@ -309,6 +309,18 @@ impl Mempool {
txids.push(txid);
self.txstore.insert(txid, tx);
}

// Populate tx cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an advantage to populating the cache with mempool transactions, which already are stored entirely in memory?

Also, it seems that the cache is only used when looking up on-chain txs (via ChainQuery), so the mempool transactions populated into the cache won't actually be used until they get confirmed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well when the mempool is evicted they can still be looked up from the tx cache

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but why store them while they're still in the mempool and not wait until they get confirmed (and cached naturally when they're used)?

Also its possible that by the time they confirm, they won't longer be in the FIFO cache

@RCasatta
Copy link
Collaborator Author

When lightmode is disabled, is there still an advantage to using a cache in addition to having the txs available in rocksdb?

yes, avoiding to hit the disk

@RCasatta
Copy link
Collaborator Author

Thanks for the review, I wasn't expecting it while in draft, but it's useful to have early feedback...

@shesek
Copy link
Collaborator

shesek commented Sep 29, 2024

When lightmode is disabled, is there still an advantage to using a cache in addition to having the txs available in rocksdb?

yes, avoiding to hit the disk

I guess my question was, if we have all transactions available in a reasonably-fast in-process database and don't require network roundtrips to fetch them (as in romanz/electrs), would a cache still improve things in the typical case for typical access patterns? How large does the cache has to be for it to be beneficial? I'm not saying that it wouldn't help, just not so sure how to analyze this.

Another difference from romanz/electrs is that it needs to fetch full transactions in order to provide scripthash history, so a single history request can result in processing potentially hundreds or thousands of funding/spending transactions. In our implementation everything needed is already available in the history indexes, so we don't lookup txs internally as much.

Another thing to consider is that in the Esplora setup, we also have caching implemented at the HTTP level by caching proxies. For transactions, once they reach 10 confirmations we set a max-age of 5 years. However this does not apply to the Electrum server or to internal transaction lookup like a tx cache would.

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