fix(electrum): bdk_electrum coinbase merkle and populate_with_txids exploit fixes

[LagginTimes]

Fixes #1698, bitcoindevkit/bdk_wallet#57.

Description

The aim of this PR is to fix a few exploits that were found within bdk_electrum. Most notably:

• Transaction Merkle proof verification does not check the Merkle proof for the coinbase. This enables an attack by which an attacker can fake a deeply-confirmed payment to a BDK wallet. (https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710#merkle-tree-attacks-using-64-bytes-transactions-8)
• An Electrum server can trigger a panic in populate_with_txids by returning a transaction with no output.
• fetch_prev_txout should not try to query the prevouts of coinbase transactions. This will query a transaction to the Electrum server which will return an error and will make the overall sync fail.

Notes to the reviewers

• The "validate coinbase merkle test" commit needs significant review. My initial thoughts were to put MockElectrumClient into bdk_testenv, but because bdk_testenv's electrum-client is re-exported from electrsd, a separate dependency would have to be added just for electrum-client 0.22 support. However, since we are doing in-line tests to access private functions, there is a strong argument for leaving the test structures where they are currently at. Or perhaps there is a better way to specifically test validate_merkle_for_anchor that does not involve creating a MockElectrumClient, similar to what @evanlinjin is doing here: https://github.com/evanlinjin/experiments/blob/main/electrum_c/src/state.rs.

Changelog notice

• fetch_prev_txout no longer queries coinbase transactions.
• Coinbase tx merkle path is also validated during tx merkle path verification.
• populate_with_txids will no longer panic on transactions with no outputs.

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]

Isn't this one part of 1.0 milestone? I guess it's missing it and not appearing on the project board.

[notmandatory]

For the audit issue #1698 you fixed it with #1756. Can you update this to just be checking Merkle proofs? And if so then I'd say this is a new feature that can be put off to a post 1.0.0 release.

[oleonardolima]

Concept ACK

I'm planning giving this one a test throughout the weekend, and think more about the approach with MockElectrumClient.

For the audit issue #1698 you fixed it with #1756. Can you update this to just be checking Merkle proofs? And if so then I'd say this is a new feature that can be put off to a post 1.0.0 release.

Agree, the commit with the fix on coinbase prevout should be dropped. The remaining two fixes (I guess) can either be together in this PR or separate, it'd depend on the priority of each one on 1.0.0-beta milestone.

[evanlinjin]

Thank you for working on this. However, I'm not sure how useful this is here, since we aren't even verifying the PoW of the chain in bdk_electrum. bdk_electrum assumes that we can trust the server that they are connected to.

In the future, we should have a version of bdk_electrum which checks PoW and addresses this exploit.