feat!: convert fees from BTC/kB to sats/vB

[storopoli]

Fix for https://github.com/bitcoindevkit/bdk/issues/1519

[storopoli]

I was just emulating what we already do in rust-esplora-client:

/// Get a fee value in sats/vbytes from the estimates
/// that matches the confirmation target set as parameter.
pub fn convert_fee_rate(target: usize, estimates: HashMap<u16, f64>) -> Result<f32, Error> {
    let fee_val = {
        let mut pairs = estimates.into_iter().collect::<Vec<(u16, f64)>>();
        pairs.sort_unstable_by_key(|(k, _)| std::cmp::Reverse(*k));
        pairs
            .into_iter()
            .find(|(k, _)| *k as usize <= target)
            .map(|(_, v)| v)
            .unwrap_or(1.0)
    };
    Ok(fee_val as f32)
}

Shall we use FeeRate instead?

[tnull]

I was just emulating what we already do in rust-esplora-client:

/// Get a fee value in sats/vbytes from the estimates
/// that matches the confirmation target set as parameter.
pub fn convert_fee_rate(target: usize, estimates: HashMap<u16, f64>) -> Result<f32, Error> {
    let fee_val = {
        let mut pairs = estimates.into_iter().collect::<Vec<(u16, f64)>>();
        pairs.sort_unstable_by_key(|(k, _)| std::cmp::Reverse(*k));
        pairs
            .into_iter()
            .find(|(k, _)| *k as usize <= target)
            .map(|(_, v)| v)
            .unwrap_or(1.0)
    };
    Ok(fee_val as f32)
}

Shall we use FeeRate instead?

I think it would be preferable, yes. At least a lot less error-prone and unambiguous than using a f32 or similar.

[storopoli]

I agree and like the idea. Will update the PR soon. Thanks!

[matthiasdebernardini]

I was unable to get a correct fee estimation

    let res = client.estimate_fee(1).unwrap().to_sat_per_vb_ceil();
    println!("{:#?}", res);

returns

100

when the current recommended fee from mempool.space (I'd expect a similar, if not exact value) is

{"fastestFee":5,"halfHourFee":5,"hourFee":5,"economyFee":4,"minimumFee":2}

I think the convert fee rate function needs work.

[storopoli]

I think the math is right, but the existing test might need to be updated. What you think?

[tnull]

I think the math is right, but the existing test might need to be updated. What you think?

As discussed offline, I think the tests need fixing.

[storopoli]

The tests are CRAZY! They are even not reproducible. They pretty much ping a live server electrum.blockstream.info:50001. So I added the tests for the fees to make sure that the fee is > 0.

[tnull]

The tests are CRAZY! They are even not reproducible. They pretty much ping a live server electrum.blockstream.info:50001. So I added the tests for the fees to make sure that the fee is > 0.

Maybe we should switch to use a local regtest electrs that we pre-populate with a certain amount of transactions and blocks to make them ~reproducible? Or, we could consider building a mock Electrum server, which shouldn't be hard to do? Especially given the confusion around fee rate conversion it would be good to test it properly in CI?

[storopoli]

Moved to #142 so that we can move with this PR into review/merge mode.