Skip to content

Fix potentially wrong logic if bdk_electrum is used with other chain sources #1956

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.

Sign up for GitHub

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

Closed
evanlinjin wants to merge 1 commit into from
Closed

Fix potentially wrong logic if bdk_electrum is used with other chain sources #1956

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 27 additions & 44 deletions crates/electrum/src/bdk_electrum_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
let start_time = request.start_time();

let tip_and_latest_blocks = match request.chain_tip() {
Some(chain_tip) => Some(fetch_tip_and_latest_blocks(&self.inner, chain_tip)?),
Some(chain_tip) => Some(fetch_tip(&self.inner, chain_tip)?),
None => None,
};

Expand Down Expand Up @@ -141,9 +141,8 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
}

let chain_update = match tip_and_latest_blocks {
Some((chain_tip, latest_blocks)) => Some(chain_update(
Some(chain_tip) => Some(populate_chain_with_anchor_heights(
chain_tip,
&latest_blocks,
tx_update.anchors.iter().cloned(),
)?),
_ => None,
Expand Down Expand Up @@ -188,7 +187,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
let start_time = request.start_time();

let tip_and_latest_blocks = match request.chain_tip() {
Some(chain_tip) => Some(fetch_tip_and_latest_blocks(&self.inner, chain_tip)?),
Some(chain_tip) => Some(fetch_tip(&self.inner, chain_tip)?),
None => None,
};

Expand Down Expand Up @@ -231,9 +230,8 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
}

let chain_update = match tip_and_latest_blocks {
Some((chain_tip, latest_blocks)) => Some(chain_update(
Some(chain_tip) => Some(populate_chain_with_anchor_heights(
chain_tip,
&latest_blocks,
tx_update.anchors.iter().cloned(),
)?),
None => None,
Expand Down Expand Up @@ -597,25 +595,15 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
}
}

/// Return a [`CheckPoint`] of the latest tip, that connects with `prev_tip`. The latest blocks are
/// fetched to construct checkpoint updates with the proper [`BlockHash`] in case of re-org.
fn fetch_tip_and_latest_blocks(
client: &impl ElectrumApi,
prev_tip: CheckPoint,
) -> Result<(CheckPoint, BTreeMap<u32, BlockHash>), Error> {
/// Return a [`CheckPoint`] of the latest tip that connects with `prev_tip`.
fn fetch_tip(client: &impl ElectrumApi, local_tip: CheckPoint) -> Result<CheckPoint, Error> {
let HeaderNotification { height, .. } = client.block_headers_subscribe()?;
let new_tip_height = height as u32;

// If electrum returns a tip height that is lower than our previous tip, then checkpoints do
// not need updating. We just return the previous tip and use that as the point of agreement.
if new_tip_height < prev_tip.height() {
return Ok((prev_tip, BTreeMap::new()));
}
let remote_tip_height = height as u32;

// Atomically fetch the latest `CHAIN_SUFFIX_LENGTH` count of blocks from Electrum. We use this
// to construct our checkpoint update.
let mut new_blocks = {
let start_height = new_tip_height.saturating_sub(CHAIN_SUFFIX_LENGTH - 1);
let start_height = remote_tip_height.saturating_sub(CHAIN_SUFFIX_LENGTH - 1);
let hashes = client
.block_headers(start_height as _, CHAIN_SUFFIX_LENGTH as _)?
.headers
Expand All @@ -627,13 +615,13 @@ fn fetch_tip_and_latest_blocks(
// Find the "point of agreement" (if any).
let agreement_cp = {
let mut agreement_cp = Option::<CheckPoint>::None;
for cp in prev_tip.iter() {
for cp in local_tip.iter() {
let cp_block = cp.block_id();
let hash = match new_blocks.get(&cp_block.height) {
Some(&hash) => hash,
None => {
assert!(
new_tip_height >= cp_block.height,
remote_tip_height >= cp_block.height,
"already checked that electrum's tip cannot be smaller"
);
let hash = client.block_header(cp_block.height as _)?.block_hash();
Expand All @@ -650,38 +638,33 @@ fn fetch_tip_and_latest_blocks(
.ok_or_else(|| Error::Message("cannot find agreement block with server".to_string()))?
};

let extension = new_blocks
.iter()
.filter({
let agreement_height = agreement_cp.height();
move |(height, _)| **height > agreement_height
})
.map(|(&height, &hash)| BlockId { height, hash });
let new_tip = agreement_cp
.extend(extension)
.expect("extension heights already checked to be greater than agreement height");

Ok((new_tip, new_blocks))
// Contruct the new tip.
let mut new_tip = agreement_cp;
for (height, hash) in new_blocks {
new_tip = new_tip.insert(BlockId { height, hash });
}
Ok(new_tip)
}

// Add a corresponding checkpoint per anchor height if it does not yet exist. Checkpoints should not
// surpass `latest_blocks`.
fn chain_update(
fn populate_chain_with_anchor_heights<A>(
mut tip: CheckPoint,
latest_blocks: &BTreeMap<u32, BlockHash>,
anchors: impl Iterator<Item = (ConfirmationBlockTime, Txid)>,
) -> Result<CheckPoint, Error> {
anchors: A,
) -> Result<CheckPoint, Error>
where
A: Iterator<Item = (ConfirmationBlockTime, Txid)>,
{
let tip_height = tip.height();
let exclude_from = tip_height - CHAIN_SUFFIX_LENGTH;

for (anchor, _txid) in anchors {
let height = anchor.block_id.height;

// Checkpoint uses the `BlockHash` from `latest_blocks` so that the hash will be consistent
// in case of a re-org.
if tip.get(height).is_none() && height <= tip.height() {
let hash = match latest_blocks.get(&height) {
Some(&hash) => hash,
None => anchor.block_id.hash,
};
tip = tip.insert(BlockId { hash, height });
if tip.get(height).is_none() && height < exclude_from {
tip = tip.insert(anchor.block_id);
}
}
Ok(tip)
Expand Down
Loading