bdk + esplora not seeing an old transaction as confirmed #1151

danielabrozzoni · 2023-10-06T14:28:50Z

Describe the bug
When syncing using bdk and esplora, we might not see that a certain tx is confirmed, and consider it unconfirmed forever

To Reproduce
See https://github.com/sebastianmontero/bdk-esplora-test

Expected behavior
bdk correctly seeing a transaction as confirmed when it is

Build environment

BDK tag/commit: 4ee11aa

The text was updated successfully, but these errors were encountered:

...before using missing_heights Fixes bitcoindevkit#1151. When wallet_esplora_* was used to sync a wallet containing a transaction confirmed some time ago (more than 10-15 blocks ago), the transaction would be stuck in an "unconfirmed" state forever. At the first scan time, `update_local_chain` would just fetch the last 10 to 15 blocks (depending on the server used), and `tx_graph.missing_heights` wouldn't return the tx's confirmation block as it was called on the original, non-updated tx_graph. So, after the first scan, we would have a transaction in memory with an anchor that doesn't exist in our local_chain, and try_get_chain_position would return unconfirmed. When scanning again, missing_heights would find the missing anchor, but `update_local_chain` wouldn't include it as it's older than ASSUME_FINAL_DEPTH. The missing block would be downloaded every time, but never included in the local_chain, and the transaction would remain unconfirmed forever. Here we update the tx_graph immediately after getting the graph update, so that `missing_heights` can correctly return the anchor height, and `update_local_chain` fetch it and include it in the chain.

LLFourn · 2023-10-09T02:31:14Z

The bug is in the above code and in the wallet example. You're meant to ask the update what heights are missing from the current chain not the wallet. Though I'm not sure why it would be unconfirmed forever. The transaction should be added with its anchors anyway so it should be missing the next time I would have thought. Can we confirm why the anchor is not returned from missing_heights in the future? Is it never inserted? If it's there and not returned it's probably a bug in missing_heights.

danielabrozzoni · 2023-10-09T08:29:52Z

@LLFourn missing_heights would return the anchor, but update_local_chain wouldn't insert it as it's older than ASSUME_FINAL_DEPTH:

bdk/crates/esplora/src/blocking_ext.rs

Lines 134 to 140 in 4a65a12

    
           let updated_hash = match fetched_blocks.entry(local_block.height) { 
        
               btree_map::Entry::Occupied(entry) => *entry.get(), 
        
               btree_map::Entry::Vacant(entry) => *entry.insert( 
        
                   if local_tip_height - local_block.height >= ASSUME_FINAL_DEPTH { 
        
                       local_block.hash 
        
                   } else { 
        
                       self.get_block_hash(local_block.height)?

...graph update Fixes bitcoindevkit#1151. When wallet_esplora_* was used to sync a wallet containing a transaction confirmed some time ago (more than 10-15 blocks ago), the transaction would be stuck in an "unconfirmed" state forever. At the first scan time, `update_local_chain` would just fetch the last 10 to 15 blocks (depending on the server used), and `tx_graph.missing_heights` wouldn't return the tx's confirmation block as it was called on the original, non-updated tx_graph. So, after the first scan, we would have a transaction in memory with an anchor that doesn't exist in our local_chain, and try_get_chain_position would return unconfirmed. When scanning again, missing_heights would find the missing anchor, but `update_local_chain` wouldn't include it as it's older than ASSUME_FINAL_DEPTH. The missing block would be downloaded every time, but never included in the local_chain, and the transaction would remain unconfirmed forever. Here we call missing_heights on the updated graph, so that it can correctly return the anchor height, and `update_local_chain` can fetch it and include it in the chain.

b1461f0 fix(wallet_esplora): missing_heights uses the... ...graph update (Daniela Brozzoni) Pull request description: Fixes #1151. When wallet_esplora_* was used to sync a wallet containing a transaction confirmed some time ago (more than 10-15 blocks ago), the transaction would be stuck in an "unconfirmed" state forever. At the first scan time, `update_local_chain` would just fetch the last 10 to 15 blocks (depending on the server used), and `tx_graph.missing_heights` wouldn't return the tx's confirmation block as it was called on the original, non-updated tx_graph. So, after the first scan, we would have a transaction in memory with an anchor that doesn't exist in our local_chain, and try_get_chain_position would return unconfirmed. When scanning again, missing_heights would find the missing anchor, but `update_local_chain` wouldn't include it as it's older than ASSUME_FINAL_DEPTH. The missing block would be downloaded every time, but never included in the local_chain, and the transaction would remain unconfirmed forever. Here we call missing_heights on the updated graph, so that it can correctly return the anchor height, and `update_local_chain` can fetch it and include it in the chain. ### Notes to the reviewers I'm not sure if this is the right approach, so I'm opening this PR to gather feedback. I still need to add tests, I'll do so once we decide if this is the right way to go or not. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Bugfixes: * [x] This pull request breaks the existing API * [ ] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: evanlinjin: ACK b1461f0 Tree-SHA512: ba0cf85929644ee737dbc77e6afec662845532de0f120917aa6000ca8f5db79d0cb3971bd92285b5c5b5d26042b60b6c8536f50c9bd49615e31f5da28e80a509

LLFourn · 2023-10-10T00:01:56Z

@LLFourn missing_heights would return the anchor, but update_local_chain wouldn't insert it as it's older than ASSUME_FINAL_DEPTH:

bdk/crates/esplora/src/blocking_ext.rs

Lines 134 to 140 in 4a65a12

let updated_hash = match fetched_blocks.entry(local_block.height) {

btree_map::Entry::Occupied(entry) => *entry.get(),

btree_map::Entry::Vacant(entry) => *entry.insert(

if local_tip_height - local_block.height >= ASSUME_FINAL_DEPTH {

local_block.hash

} else {

self.get_block_hash(local_block.height)?

But it should be part of request_heights is what I mean. The things in request_heights are always fetched:

bdk/crates/esplora/src/blocking_ext.rs

Lines 107 to 117 in 4a65a12

    
           for height in request_heights { 
        
               // do not fetch blocks higher than remote tip 
        
               if height > new_tip_height { 
        
                   continue; 
        
               } 
        
               // only fetch what is missing 
        
               if let btree_map::Entry::Vacant(entry) = fetched_blocks.entry(height) { 
        
                   let hash = self.get_block_hash(height)?; 
        
                   entry.insert(hash); 
        
               } 
        
           }

So more succinct question why isn't the things in missing_heights going into request_heights and then being returned.
Maybe something going wrong here:

bdk/crates/esplora/src/blocking_ext.rs

Lines 178 to 186 in 4a65a12

    
           fetched_blocks 
        
               // we exclude anything at or below the first cp of the update chain otherwise 
        
               // building the chain will fail 
        
               .split_off(&(first_cp.height() + 1)) 
        
               .into_iter() 
        
               .map(|(height, hash)| BlockId { height, hash }) 
        
               .fold(first_cp, |prev_cp, block| { 
        
                   prev_cp.push(block).expect("must extend checkpoint") 
        
               })

danielabrozzoni · 2023-10-10T00:15:00Z

Yeah, I can look better into it tomorrow, but as far as I can tell the block is fetched as part of the request heights, but then not added to the chain because the earliest agreement checkpoint is not early enough. I initially thought it was because of ASSUME_FINAL_DEPTH, but I might be wrong. -------- Original Message --------

…

On Oct 10, 2023, 02:02, Lloyd Fournier wrote: > ***@***.***(https://github.com/LLFourn) missing_heights would return the anchor, but update_local_chain wouldn't insert it as it's older than ASSUME_FINAL_DEPTH: > > https://github.com/bitcoindevkit/bdk/blob/4a65a12c4fe9bb419906d5c921bbff8b8b4e5379/crates/esplora/src/blocking_ext.rs#L134-L140 But it should be part of request_heights is what I mean. The things in request_heights are always fetched: https://github.com/bitcoindevkit/bdk/blob/4a65a12c4fe9bb419906d5c921bbff8b8b4e5379/crates/esplora/src/blocking_ext.rs#L107-L117 So more succinct question why isn't the things in missing_heights going into request_heights and then being returned. Maybe something going wrong here: https://github.com/bitcoindevkit/bdk/blob/4a65a12c4fe9bb419906d5c921bbff8b8b4e5379/crates/esplora/src/blocking_ext.rs#L178-L186 — Reply to this email directly, [view it on GitHub](#1151 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AF7B4KP2RUA23JKPRJQHRCLX6SF77AVCNFSM6AAAAAA5V5GK5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJUGA3TOMRWGE). You are receiving this because you were assigned.Message ID: ***@***.***>

LLFourn · 2023-10-10T01:10:03Z

This seems like a bug if so. Thanks a lot for looking into it. I'll just reopen this because I think although this is not the main problem it should have fixed itself. FWIW I don't understand why we are doing a split_off which seems like it might be the culprit.

cc @evanlinjin

evanlinjin · 2023-10-10T07:39:40Z

https://github.com/sebastianmontero/bdk-esplora-test/blob/master/src/main.rs#L34

Would this line be the culprit?

Edit: okay this is already bought up, I didn't read the above conversation.

nondiremanuel · 2023-11-05T09:09:35Z

I close this issue since it was fixed by #1152

danielabrozzoni · 2023-11-08T12:41:51Z

Fair, I opened #1199 to keep track of the bug we discovered while investigating this one

danielabrozzoni added the bug Something isn't working label Oct 6, 2023

danielabrozzoni self-assigned this Oct 6, 2023

danielabrozzoni added this to BDK Oct 6, 2023

danielabrozzoni moved this to In Progress in BDK Oct 6, 2023

danielabrozzoni mentioned this issue Oct 6, 2023

fix(wallet_esplora): missing_heights uses the graph update #1152

Merged

6 tasks

evanlinjin closed this as completed in b1461f0 Oct 9, 2023

github-project-automation bot moved this from In Progress to Done in BDK Oct 9, 2023

LLFourn reopened this Oct 10, 2023

danielabrozzoni mentioned this issue Oct 11, 2023

Add integration tests for electrum, esplora, bitcoind_rpc crates #1094

Closed

notmandatory mentioned this issue Nov 1, 2023

Not seeing confirmed TX with example_esplora CLI tool #1192

Closed

nondiremanuel closed this as completed Nov 5, 2023

danielabrozzoni mentioned this issue Nov 8, 2023

bdk + esplora update_local_chain logic might be off #1199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bdk + esplora not seeing an old transaction as confirmed #1151

bdk + esplora not seeing an old transaction as confirmed #1151

danielabrozzoni commented Oct 6, 2023 •

edited

Loading

LLFourn commented Oct 9, 2023 •

edited

Loading

danielabrozzoni commented Oct 9, 2023

LLFourn commented Oct 10, 2023

danielabrozzoni commented Oct 10, 2023 via email

LLFourn commented Oct 10, 2023

evanlinjin commented Oct 10, 2023 •

edited

Loading

nondiremanuel commented Nov 5, 2023

danielabrozzoni commented Nov 8, 2023

bdk + esplora not seeing an old transaction as confirmed #1151

bdk + esplora not seeing an old transaction as confirmed #1151

Comments

danielabrozzoni commented Oct 6, 2023 • edited Loading

LLFourn commented Oct 9, 2023 • edited Loading

danielabrozzoni commented Oct 9, 2023

LLFourn commented Oct 10, 2023

danielabrozzoni commented Oct 10, 2023 via email

LLFourn commented Oct 10, 2023

evanlinjin commented Oct 10, 2023 • edited Loading

nondiremanuel commented Nov 5, 2023

danielabrozzoni commented Nov 8, 2023

danielabrozzoni commented Oct 6, 2023 •

edited

Loading

LLFourn commented Oct 9, 2023 •

edited

Loading

evanlinjin commented Oct 10, 2023 •

edited

Loading