fix(electrum): prevent fetch_prev_txout from querying coinbase transactions

[LagginTimes]

Fixes #1698.

Description

fetch_prev_txout should not try to query the prevouts of coinbase transactions, as it may result in the Electrum server returning an error which would cause the overall sync/full_scan to fail. Without this critical bug fix, bdk_electrum will crash when someone tracks an address being mined to.

Notes to the reviewers

Changelog notice

• fetch_prev_txout no longer queries coinbase transactions.

Checklists

All Submissions:

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

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

[notmandatory]

Needs a test then this looks good to go for 1.0.0.

[ValuedMammal]

For the test can't we also build a SyncRequest with the same address that receives the block reward? which would cause electrum to throw an error without the fix in this PR.

[jirijakes]

tACK ccf13ec. Test breaks when coinbase TX queried.

Failed test

running 1 test
test bdk_electrum_client::test::test_fetch_prev_txout_with_coinbase ... FAILED
failures:
failures:

bdk_electrum_client::test::test_fetch_prev_txout_with_coinbase
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.20s
--- STDERR:              bdk_electrum bdk_electrum_client::test::test_fetch_prev_txout_with_coinbase ---

thread 'bdk_electrum_client::test::test_fetch_prev_txout_with_coinbase' panicked at crates/electrum/src/bdk_electrum_client.rs:570:9:

assertion failed: client.fetch_prev_txout(&mut tx_update).is_ok()

[LagginTimes]

For the test can't we also build a SyncRequest with the same address that receives the block reward? which would cause electrum to throw an error without the fix in this PR.

Yes, something like this would work as well:

#[test]
fn test_sync_with_coinbase() -> anyhow::Result<()> {
    let env = TestEnv::new()?;
    let electrum_client = electrum_client::Client::new(env.electrsd.electrum_url.as_str())?;
    let client = BdkElectrumClient::new(electrum_client);

    // Setup address.
    let spk_to_track = ScriptBuf::new_p2wsh(&WScriptHash::all_zeros());
    let addr_to_track = Address::from_script(&spk_to_track, bdk_chain::bitcoin::Network::Regtest)?;

    // Setup receiver.
    let (mut recv_chain, _) = LocalChain::from_genesis_hash(env.bitcoind.client.get_block_hash(0)?);
    let mut recv_graph = IndexedTxGraph::<ConfirmationBlockTime, _>::new({
        let mut recv_index = SpkTxOutIndex::default();
        recv_index.insert_spk((), spk_to_track.clone());
        recv_index
    });

    // Mine some blocks.
    env.mine_blocks(101, Some(addr_to_track))?;
    env.wait_until_electrum_sees_block(Duration::from_secs(6))?;

    // Check to see if electrum syncs properly.
    assert!(sync_with_electrum(
        &client,
        [spk_to_track.clone()],
        &mut recv_chain,
        &mut recv_graph,
    )
    .is_ok());

    Ok(())
}

I believe the current implementation in the PR is more straightforward and easier to understand. However, I would appreciate hearing others' thoughts on this.

[ValuedMammal]

ACK ccf13ec

[evanlinjin]

@LagginTimes let's include both tests. The test_sync_with_coinbase is excellent.

[evanlinjin]

ACK ccf13ec

[notmandatory]

ACK ccf13ec

[notmandatory]

Looks like this is ready to go once test_sync_with_coinbase() is added. I'm also happy to have it merged as is.

[LagginTimes]

test_sync_with_coinbase() has been added.

[evanlinjin]

ACK d4ef266