-
Notifications
You must be signed in to change notification settings - Fork 404
Add populate_anchor_cache method to bdk #2005
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: master
Are you sure you want to change the base?
Conversation
Thank you for approaching this. The ticket was very brief and was not specific enough - sorry about that. Let me clarify what we are looking for.
In other words, |
c26661f
to
9b22afe
Compare
Thanks for the contribution! A couple of things to take care of:
|
9b22afe
to
dacdcd0
Compare
Box::new(self.full_txs().flat_map(move |tx_node| { | ||
tx_node.anchors.iter().map(move |anchor| { | ||
let block_id = anchor.block_id; | ||
((tx_node.txid, block_id.hash), *anchor) |
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.
You are getting the block hash from the anchor, so the API doesn't need to take in the block hash separately.
@@ -94,6 +94,26 @@ pub struct ScanOptions { | |||
pub batch_size: usize, | |||
} | |||
|
|||
/// Extension trait to expose anchors in a public-friendly way | |||
trait TxGraphExt { |
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.
Only introduce extension traits out of necessity. Is there another method on TxGraph
that you can use? If not, you can introduce a method on TxGraph
directly.
dacdcd0
to
030a4e0
Compare
/// Iterate over all tx outputs known by [`TxGraph`]. | ||
/// | ||
/// This returns an iterator over all anchors in the graph as (txid, anchor) pairs | ||
pub fn anchors(&self) -> impl Iterator<Item = (Txid, &A)> + '_ { | ||
self.anchors | ||
.iter() | ||
.flat_map(|(txid, anchors)| anchors.iter().map(move |a| (*txid, a))) | ||
} | ||
|
||
/// This includes txouts of both full transactions as well as floating transactions. |
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.
I think you inserted anchors
in the middle of the doc comments of all_txouts
.
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.
Please double check and try running the code you have written (or the code written for you).
Comments need to capture non-obvious intent or constraints, not restate what is obvious by looking at the code.
@@ -126,6 +126,16 @@ fn main() -> anyhow::Result<()> { | |||
|
|||
let client = BdkElectrumClient::new(electrum_cmd.electrum_args().client(network)?); | |||
|
|||
// This avoids re-fetching known anchor confirmations from Electrum. | |||
// Hold the lock for the entire anchor extraction | |||
let graph_guard = graph.lock().unwrap(); |
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.
Have you tried running this? Since we lock graph
again on line 143, wouldn't this result in a deadlock?
// This avoids re-fetching known anchor confirmations from Electrum. | ||
// Hold the lock for the entire anchor extraction | ||
let graph_guard = graph.lock().unwrap(); | ||
// Collect anchors to release the lock quickly | ||
let mut anchors = Vec::new(); | ||
for (txid, anchor) in graph_guard.graph().anchors() { | ||
anchors.push((txid, *anchor)); // Dereference instead of clone for Copy types | ||
} | ||
client.populate_anchor_cache(anchors); | ||
|
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.
Some of these comments are explaining very obvious things in a way that doesn’t add much beyond reading the code itself.
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.
Also, how about we populate the anchor & tx cache under a single lock of graph
?
/// This returns an iterator over all anchors in the graph as (txid, anchor) pairs | ||
pub fn anchors(&self) -> impl Iterator<Item = (Txid, &A)> + '_ { | ||
self.anchors | ||
.iter() | ||
.flat_map(|(txid, anchors)| anchors.iter().map(move |a| (*txid, a))) | ||
} |
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.
Instead of introducing this anchors
method, how about we make use of the all_anchors
method we have already?
I propose changing BdkElectrumClient::populate_anchor_cache
to the following:
/// Insert anchors into the anchor cache so that they don’t have to be fetched again from
/// Electrum. Typically used to pre-populate the cache from an existing `TxGraph`.
pub fn populate_anchor_cache(
&self,
tx_anchors: impl IntoIterator<Item = (Txid, impl IntoIterator<Item = ConfirmationBlockTime>)>,
) {
let mut cache = self.anchor_cache.lock().unwrap();
for (txid, anchors) in tx_anchors {
for anchor in anchors {
cache.insert((txid, anchor.block_id.hash), anchor);
}
}
}
Using the API will look like this:
let graph = graph.lock().unwrap();
client.populate_anchor_cache(graph.graph().all_anchors().clone());
@Her-Code Thanks for taking this on! It would be really helpful to have this in soon. Do you think you’d be able to complete it by the end of the week, or would you need more time? |
This PR addresses #1982
It introduces functionality to populate the anchor cache, improving how BDK handles stateful or performance-critical anchor data.
Changes
populate_anchor_cache
method toBdkElectrumClient
inbdk-core
Follow-up work will extend this functionality to
bdk-wallet
.