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

storopoli · 2024-07-23T17:10:58Z

tnull

Hmm, see my comment over on the issue, but why not have estimate_fee/batch_estimate_fee return bitcoin::FeeRates?

Otherwise this would amount to a silent change of the API semantics that could have severe consequences for users that expect to get a fee rate in a certain denomination and now suddenly we'd return a different one.

storopoli · 2024-07-24T10:54:19Z

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 · 2024-07-24T10:59:25Z

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 · 2024-07-24T11:00:33Z

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

matthiasdebernardini · 2024-07-24T20:11:27Z

I was unable to get a correct fee estimation

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

returns

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.

tnull · 2024-07-25T08:41:05Z

src/utils.rs

@@ -41,3 +45,10 @@ pub fn validate_merkle_proof(

    cur == merkle_root.to_raw_hash()
 }
+
+/// Converts a fee rate in BTC/kB to sats/vbyte.
+pub fn convert_fee_rate(fee_rate: f64) -> FeeRate {


I don't think this should be pub. Possibly, we should also just inline the require calculation as it's trivial.

I do agree, you can just do this inline.

Ok moved to pub(crate) because it's being called in other files.

tnull · 2024-07-25T09:21:23Z

src/utils.rs

+pub fn convert_fee_rate(fee_rate: f64) -> FeeRate {
+    // 1 kB = 1_000 bytes
+    // 1 vbyte = 4 weight units
+    FeeRate::from_sat_per_kwu((fee_rate.ceil() as u64 * SATOSHIS_IN_BTC) / 4_000)


This seems wrong. How about?:

pub fn convert_fee_rate(fee_rate_btc_kvb: f64) -> Option<FeeRate> { /// Convert a fee rate in BTC/kvB to sats/vB let fee_rate_sat_vb = (fee_rate_btc_kvb * 100_000.0) as u64; FeeRate::from_sat_per_vb(fee_rate_sat_vb) }

I think same applies here, you probably want to also use Weight too (if needed for the conversion).

for thought: Is this conversion something we could upstream to rust-bitcoin units module as a new FeeRate module ?

src/api.rs

oleonardolima · 2024-07-27T01:03:13Z

src/utils.rs

@@ -41,3 +45,10 @@ pub fn validate_merkle_proof(

    cur == merkle_root.to_raw_hash()
 }
+
+/// Converts a fee rate in BTC/kB to sats/vbyte.
+pub fn convert_fee_rate(fee_rate: f64) -> FeeRate {


I do agree, you can just do this inline.

oleonardolima · 2024-07-27T01:03:49Z

src/utils.rs

+pub fn convert_fee_rate(fee_rate: f64) -> FeeRate {
+    // 1 kB = 1_000 bytes
+    // 1 vbyte = 4 weight units
+    FeeRate::from_sat_per_kwu((fee_rate.ceil() as u64 * SATOSHIS_IN_BTC) / 4_000)


I think same applies here, you probably want to also use Weight too (if needed for the conversion).

for thought: Is this conversion something we could upstream to rust-bitcoin units module as a new FeeRate module ?

storopoli · 2024-07-28T02:05:37Z

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

tnull · 2024-08-06T07:49:43Z

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 · 2024-08-07T21:32:03Z

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 · 2024-08-08T06:52:42Z

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 · 2024-08-08T08:46:32Z

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

tnull · 2024-08-08T08:49:46Z

src/raw_client.rs

-        impl_batch_call!(self, numbers, estimate_fee, apply_deref)
+        let fee_rate_num: Result<Vec<f64>, Error> =
+            impl_batch_call!(self, numbers, estimate_fee, apply_deref);
+        let fee_rate: Vec<FeeRate> = fee_rate_num?.into_iter().map(convert_fee_rate).collect();


Can we avoid the additional allocation here by just collecting a Vec once?

Yeah, I modified the macro impl_batch_call!. Any ideas on how to fix it? Very little experience with macros here...

tnull · 2024-08-08T08:50:01Z

src/utils.rs

+/// Converts a fee rate in BTC/kB to sats/vbyte.
+///
+/// # Note
+///


nit: Superfluous blank line?

It is not superfluous. In rustdocs it is common to leave a blank line before and after headers, just like markdown.

Fair, excuse the noise.

But I've removed since we are returning an error.

tnull · 2024-08-08T08:51:25Z

src/utils.rs

+/// 1 sat/vbyte.
+pub(crate) fn convert_fee_rate(fee_rate_kvb: f64) -> FeeRate {
+    let fee_rate_sat_vb = (Amount::ONE_BTC.to_sat() as f64) * fee_rate_kvb;
+    FeeRate::from_sat_per_vb(fee_rate_sat_vb as u64).unwrap_or(FeeRate::from_sat_per_vb(1).unwrap())


Can we avoid the default fallback here? I think we're just about to drop this over at rust-esplora-client (cf bitcoindevkit/rust-esplora-client#80), would make sense to also avoid it here?

Now returns an error.

Thanks, though see #136 (comment)

tnull · 2024-08-08T09:47:28Z

src/raw_client.rs

@@ -880,7 +882,8 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {

        result
            .as_f64()
-            .ok_or_else(|| Error::InvalidResponse(result.clone()))
+            .map(convert_fee_rate)
+            .expect("Invalid response")


Ugh, please never panic on input we receive via network from a third party. Any calls to convert_fee_rate also need to be fallible and the user needs to handle the error.

I agree, and you can probably add a new error variant to Error such as: CouldNotConvertFeeRate, or reuse some of the existing ones.

Also changes all fee rates from f64 to proper FeeRate

oleonardolima · 2024-08-09T12:42:26Z

src/api.rs

@@ -189,7 +189,7 @@ pub trait ElectrumApi {
    ///
    /// Takes a list of `numbers` of blocks and returns a list of fee required in
    /// **Satoshis per kilobyte** to confirm a transaction in the given number of blocks.


Suggested change

/// **Satoshis per kilobyte** to confirm a transaction in the given number of blocks.

/// [`FeeRate`] to confirm a transaction in the given number of blocks.

oleonardolima · 2024-08-09T12:43:04Z

src/api.rs

-    /// Estimates the fee required in **Bitcoin per kilobyte** to confirm a transaction in `number` blocks.
-    fn estimate_fee(&self, number: usize) -> Result<f64, Error>;
+    /// Estimates the fee required in [`FeeRate`] to confirm a transaction in `number` blocks.
+    fn estimate_fee(&self, number: usize) -> Result<FeeRate, Error>;

    /// Returns the minimum accepted fee by the server's node in **Bitcoin, not Satoshi**.


Suggested change

/// Returns the minimum accepted fee by the server's node in **Bitcoin, not Satoshi**.

/// Returns the minimum accepted fee by the server's node in [`FeeRate`].

oleonardolima · 2024-08-09T12:48:45Z

src/raw_client.rs

@@ -880,7 +882,8 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {

        result
            .as_f64()
-            .ok_or_else(|| Error::InvalidResponse(result.clone()))
+            .map(convert_fee_rate)
+            .expect("Invalid response")


I agree, and you can probably add a new error variant to Error such as: CouldNotConvertFeeRate, or reuse some of the existing ones.

oleonardolima · 2024-08-09T12:50:47Z

src/raw_client.rs

+        let resp = client.relay_fee().unwrap().to_sat_per_vb_ceil();
+        assert!(resp > 0);


I probably need some time to take a better look at the tests to wrap my mind around it, I'm afraid we can be missing something due to the current state of testing at crate level 🤔

oleonardolima · 2024-08-09T12:52:42Z

src/types.rs

+    /// There was an error parsing a fee rate
+    FeeRate(String),


Oh, now I see that you already added a new variant here.

oleonardolima · 2024-08-09T12:53:29Z

src/types.rs

@@ -365,6 +366,7 @@ impl Display for Error {
            Error::MissingDomain => f.write_str("Missing domain while it was explicitly asked to validate it"),
            Error::CouldntLockReader => f.write_str("Couldn't take a lock on the reader mutex. This means that there's already another reader thread is running"),
            Error::Mpsc => f.write_str("Broken IPC communication channel: the other thread probably has exited"),
+            Error::FeeRate(e) => f.write_str(e),


nit: You can probably leave the error message here, as it becomes a single point of change in the future.

tnull reviewed Jul 24, 2024

View reviewed changes

storopoli force-pushed the fee-conversion branch from b20aa40 to 6dbda08 Compare July 24, 2024 17:03

storopoli requested a review from tnull July 24, 2024 17:09

tnull reviewed Jul 25, 2024

View reviewed changes

oleonardolima reviewed Jul 27, 2024

View reviewed changes

storopoli force-pushed the fee-conversion branch from 6dbda08 to 696ad72 Compare July 28, 2024 01:43

storopoli force-pushed the fee-conversion branch from 696ad72 to 29f5e9f Compare August 7, 2024 21:30

storopoli requested review from tnull and oleonardolima August 7, 2024 21:32

storopoli force-pushed the fee-conversion branch from 29f5e9f to 496479c Compare August 7, 2024 21:33

storopoli mentioned this pull request Aug 8, 2024

Fix unreproducible tests #142

Open

tnull reviewed Aug 8, 2024

View reviewed changes

storopoli force-pushed the fee-conversion branch from 496479c to 3b93638 Compare August 8, 2024 09:37

tnull reviewed Aug 8, 2024

View reviewed changes

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

d9bd4aa

Also changes all fee rates from f64 to proper FeeRate

storopoli force-pushed the fee-conversion branch from 3b93638 to d9bd4aa Compare August 8, 2024 09:59

oleonardolima reviewed Aug 9, 2024

View reviewed changes

	/// Satoshis per kilobyte to confirm a transaction in the given number of blocks.
	/// [`FeeRate`] to confirm a transaction in the given number of blocks.

	/// Returns the minimum accepted fee by the server's node in Bitcoin, not Satoshi.
	/// Returns the minimum accepted fee by the server's node in [`FeeRate`].

		let resp = client.relay_fee().unwrap().to_sat_per_vb_ceil();
		assert!(resp > 0);

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

Are you sure you want to change the base?

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

Conversation

storopoli commented Jul 23, 2024

tnull left a comment

Choose a reason for hiding this comment

storopoli commented Jul 24, 2024

tnull commented Jul 24, 2024

storopoli commented Jul 24, 2024

matthiasdebernardini commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

oleonardolima Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

storopoli commented Jul 28, 2024

tnull commented Aug 6, 2024

storopoli commented Aug 7, 2024

tnull commented Aug 8, 2024

storopoli commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jul 25, 2024 •

edited

Loading

oleonardolima Jul 27, 2024 •

edited

Loading

oleonardolima Jul 27, 2024 •

edited

Loading