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

Poll mempool entries interatively #410

Merged
merged 3 commits into from
Nov 23, 2024

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Nov 22, 2024

Previously, we'd utilized getrawmempool's verbose flag to retrieve additional information about mempool entries. However, depending on the mempool size this could lead to errors as lightning-block-sync's HTTP
client limits responses to a maximum of ~8MB.

Here, we switch to only query the non-verbose entries (i.e., txid only), and retrieve the needed information iteratively via follow-up calls to getmempoolentry, which shouldn't suffer from the same issue.

Previously, we'd utilized `getrawmempool`'s `verbose` flag to retrieve
additional information about mempool entries. However, when the mempool
is full this could lead to errors as `lightning-block-sync`'s HTTP
client limits responses to a maximum of ~8MB.

Here, we switch to only query the non-verbose entries (i.e., txid only),
and retrieve the needed information iteratively via follow-up calls to
`getmempoolentry`, which shouldn't suffer from the same issue.
@tnull tnull requested a review from jkczyz November 22, 2024 14:43
jkczyz
jkczyz previously approved these changes Nov 22, 2024
Comment on lines 124 to 141
pub(crate) async fn get_mempool_entry(&self, txid: &Txid) -> std::io::Result<MempoolEntry> {
let txid_hex = bitcoin::consensus::encode::serialize_hex(txid);
let txid_json = serde_json::json!(txid_hex);
self.rpc_client
.call_method::<GetMempoolEntryResponse>("getmempoolentry", &vec![txid_json])
.await
.map(|resp| MempoolEntry { txid: txid.clone(), height: resp.height, time: resp.time })
}

pub(crate) async fn get_mempool_entries(&self) -> std::io::Result<Vec<MempoolEntry>> {
let mempool_txids = self.get_raw_mempool().await?;
let mut mempool_entries = Vec::with_capacity(mempool_txids.len());
for txid in &mempool_txids {
let entry = self.get_mempool_entry(txid).await?;
mempool_entries.push(entry);
}
Ok(mempool_entries)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A little cleaner as:

diff --git a/src/chain/bitcoind_rpc.rs b/src/chain/bitcoind_rpc.rs
index c0b4cf9..871dd8a 100644
--- a/src/chain/bitcoind_rpc.rs
+++ b/src/chain/bitcoind_rpc.rs
@@ -121,19 +121,19 @@ impl BitcoindRpcClient {
                        .map(|resp| resp.0)
        }
 
-       pub(crate) async fn get_mempool_entry(&self, txid: &Txid) -> std::io::Result<MempoolEntry> {
-               let txid_hex = bitcoin::consensus::encode::serialize_hex(txid);
+       pub(crate) async fn get_mempool_entry(&self, txid: Txid) -> std::io::Result<MempoolEntry> {
+               let txid_hex = bitcoin::consensus::encode::serialize_hex(&txid);
                let txid_json = serde_json::json!(txid_hex);
                self.rpc_client
                        .call_method::<GetMempoolEntryResponse>("getmempoolentry", &vec![txid_json])
                        .await
-                       .map(|resp| MempoolEntry { txid: txid.clone(), height: resp.height, time: resp.time })
+                       .map(|resp| MempoolEntry { txid, height: resp.height, time: resp.time })
        }
 
        pub(crate) async fn get_mempool_entries(&self) -> std::io::Result<Vec<MempoolEntry>> {
                let mempool_txids = self.get_raw_mempool().await?;
                let mut mempool_entries = Vec::with_capacity(mempool_txids.len());
-               for txid in &mempool_txids {
+               for txid in mempool_txids {
                        let entry = self.get_mempool_entry(txid).await?;
                        mempool_entries.push(entry);
                }

Copy link
Collaborator Author

@tnull tnull Nov 22, 2024

Choose a reason for hiding this comment

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

Now added as a fixup (let me know if I can squash) and also included a commit dropping all the vec! allocations in the call_method calls which turned out to be unnecessary.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash.

Polling every second may be overly aggressive, especially when we're
polling the mempool. Here, we relax the chain polling interval a bit.
@tnull tnull force-pushed the 2024-11-iterative-mempool-retrieval branch from a79f6f0 to 002c402 Compare November 22, 2024 23:48
@tnull
Copy link
Collaborator Author

tnull commented Nov 22, 2024

LGTM. Please squash.

Squashed without further changes.

@tnull tnull merged commit 2753ac4 into lightningdevkit:main Nov 23, 2024
6 of 13 checks passed
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