Check recent blocks are not empty

[rustaceanrob]

Closes #1696

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[oleonardolima]

It looks good, but it seems to partially address the problem.

I'm trying to wrap my mind on the behavior when the latest_blocks is empty. I wonder if it shouldn't be handled on the upstream fns too?

Otherwise, the same rogue Esplora server would still be able to crash the application as we expect to have a valid point of agreement here:

bdk/crates/esplora/src/blocking_ext.rs

Line 225 in 3bc45b5

let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");

[rustaceanrob]

That's a good point. I think these is_empty checks should simply be done in the first line of chain_update and this would be sufficient.

[oleonardolima]

Concept ACK f3efbda

[notmandatory]

utACK f3efbda

[notmandatory]

Can you add a simple test to exercise this condition in case we change the fix implementation later we make sure this remains fixed.

I also had some discussion with @oleonardolima and @ValuedMammal and it would be good to have the test first and make sure without any changes this does in fact gets to and fails an at expect.

[rustaceanrob]

I can try to work on that tomorrow. Heads up I will not be able to work on much next week

[ValuedMammal]

Since the checkpoint is an optional field of the sync/scan response it may also make sense to have chain_update return a Result<Option<CheckPoint>, Error>.

[rustaceanrob]

I was thinking that this should be an error variant in the Esplora client, as it should not ever respond with empty blocks when requested. I am unable to do much this week but I can review any new PRs that replace this one

[evanlinjin]

Please include docs explaining why we are doing this.

[evanlinjin]

I'm thinking, if the server does something unexpected, maybe it is best to error out? Otherwise there is no way the caller will figure it out.

[rustaceanrob]

I'm thinking, if the server does something unexpected, maybe it is best to error out? Otherwise there is no way the caller will figure it out.

Right, I agree, but that would push the error variant into rust_esplora_client and would no longer be a bdk_esplora patch correct?

[rustaceanrob]

Would be resolved by bitcoindevkit/rust-esplora-client#113

[rustaceanrob]

Closed by bitcoindevkit/rust-esplora-client#113