bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
872 stars 312 forks source link

Add recommended_fees to BDK electrum client #1519

Open matthiasdebernardini opened 4 months ago

matthiasdebernardini commented 4 months ago

Describe the enhancement
Add a new function to BdkElectrumClient that returns a vector of FeeRates, so you can easily get the value you need to use with a transaction.

Use case
Using the estimate_fee route from the electrum API is unituitive because it uses units. There should be a method that returns a vector or FeeRates that you can then use to make a transaction with.

Additional context Something like the mempool recommended fees could be good example but if you try to do it this way, a user would pull in the reqwest library and end up having to depend on OpenSSL. While they should know better than to avoid this, it would be great if they could get an easy way to know which feerate to use.

I'm trying this for now, but something simpler would be better I think.

pub fn estimate_electrum_fee(network: Network) -> anyhow::Result<MempoolSpaceFeeEstimation> {
    let electrum_url = match network {
        // todo point to mempool
        Bitcoin =>
        // "ssl://mempool.space:50002",
            "ssl://electrum.blockstream.info:50002",
        _ => "ssl://mempool.space:60602",
    };

    let client = BdkElectrumClient::new(electrum_client::Client::new(electrum_url).unwrap());
    let frs: Vec<_> = (1..=6).map(|n| {
        match client.inner.estimate_fee(n) {
            Ok(fee) => {
                // Convert BTC/kB to satoshis/kb, truncating
                let sats_per_kb = Amount::from_btc((fee* 100_000_000.0).round() / 100_000_000.0).unwrap().to_sat();

                // Convert kB to weight units (1 kB = 4000 weight units)
                let weight_kb = Weight::from_vb(250).unwrap();

                // Calculate satoshis per 1000 weight units
                let sats_per_kwu = sats_per_kb / weight_kb.to_wu();

                FeeRate::from_sat_per_kwu(sats_per_kwu)
            }
            Err(e) => {FeeRate::ZERO}
        }

    }).collect();

    let fastest_fee = frs.first().cloned().unwrap();
    let half_hour_fee = frs[3];
    let hour_fee = frs.last().cloned().unwrap();
    let msfe = MempoolSpaceFeeEstimation{
        fastest_fee,
        half_hour_fee,
        hour_fee,
    };
    Ok(msfe)
}
matthiasdebernardini commented 4 months ago

Thinking through this, perhaps a better idea would be to add more methods to FeeRate so it can accept the values from electrum and provide a fee rate to users that they can use.

storopoli commented 4 months ago

We probably have the machinery already at rust-electrum see https://github.com/bitcoindevkit/rust-electrum-client/blob/64c77ee1bc90c5e19ec0d401b97e420d495d7c6d/src/batch.rs#L64-L69

    /// Add one `blockchain.estimatefee` request to the batch queue
    pub fn estimate_fee(&mut self, number: usize) {
        let params = vec![Param::Usize(number)];
        self.calls
            .push((String::from("blockchain.estimatefee"), params));
    }

So we just need to double check the math and implement something very similar to rust-esplora-client: https://github.com/bitcoindevkit/rust-esplora-client/blob/6002aeaeea220ab3aa80f88b7d8a1f9306a292f6/src/lib.rs#L89-L102

/// 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)
}

with a set of tests and a docstring. Cc @oleonardolima. Let's try to hack this together this week before Bitcoin conf.

storopoli commented 4 months ago

@notmandatory can you assign me?

storopoli commented 4 months ago

Actually the estimatefee is deprecated in ElectrumX: https://electrumx.readthedocs.io/en/latest/protocol-methods.html#blockchain.estimatefee (same with relayfee, also get_fee_histogram).

So I don't think we should rely in bdk_electrum for anything fee related.

storopoli commented 4 months ago

Maybe we should close this :/

notmandatory commented 4 months ago

@matthiasdebernardini what do you think? since you opened this one I'll let you do the honors of closing it since it seems to be deprecated and support will depend on who's server you connect to.

LLFourn commented 4 months ago

So there's no way at all to do this with electrum?

tnull commented 4 months ago

Actually the estimatefee is deprecated in ElectrumX: https://electrumx.readthedocs.io/en/latest/protocol-methods.html#blockchain.estimatefee (same with relayfee, also get_fee_histogram).

So I don't think we should rely in bdk_electrum for anything fee related.

I think you're looking at the wrong docs (all the Electrum forks are very confusing). IIUC, the relevant fork of ElectrumX that didn't drop Bitcoin is https://github.com/spesmilo/electrumx, so the relevant protocol docs are at https://electrumx-spesmilo.readthedocs.io/en/latest/protocol-methods.html which doesn't mention anything about deprecation.

storopoli commented 4 months ago

Thanks @tnull, so we can implement this in the same way as the rust-esplora-client convert_fee_rate: https://github.com/bitcoindevkit/rust-esplora-client/blob/6002aeaeea220ab3aa80f88b7d8a1f9306a292f6/src/lib.rs#L89-L102

/// 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)
}

Does that sound good you @tnull and @matthiasdebernardini?

I am thinking on something similar to the esplora implementation above, that you pass a target as a parameter.

tnull commented 4 months ago

Does that sound good you @tnull and @matthiasdebernardini?

I am thinking on something similar to the esplora implementation above, that you pass a target as a parameter.

I don't think so? Electrum's estimatefee calls through to Bitcoin Core's estimatesmartfee RPC API which takes a single target and returns the fee estimation for that target. IMO, it makes sense that electrum-client's estimate_fee method could/should return a FeeRate, but logic like the one above doesn't seem necessary as Electrum won't return a HashMap of results anyways?

ValuedMammal commented 3 months ago

@matthiasdebernardini I think this would work

const ELECTRUM_URL: &str = "tcp://electrum.blockstream.info:50001";

fn main() -> Result<(), anyhow::Error> {
    let client = electrum_client::Client::new(ELECTRUM_URL)?;

    for n in 1..=6 {
        let btc_kvb = client.estimate_fee(n)?;
        // 0.00001000 btc/kvb * 1e8 sat/btc * 1vb/4wu = 250 sat/kwu
        let sat_kwu = (btc_kvb * 1e8 * 0.25) as u64;
        let feerate = bitcoin::FeeRate::from_sat_per_kwu(sat_kwu);
        println!("{}.00 sat/vb", feerate.to_sat_per_vb_floor());
    }

    Ok(())
}
notmandatory commented 2 months ago

@matthiasdebernardini I'm removing this from the 1.0.0-beta milestone. Let me know if @ValuedMammal's suggestion above works for you and if so I'll close this one.