Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::borrow::Borrow;
use std::convert::TryInto;

use bitcoin::consensus::encode::{deserialize, serialize};
use bitcoin::{block, Script, Transaction, Txid};
use bitcoin::{block, FeeRate, Script, Transaction, Txid};

use crate::batch::Batch;
use crate::types::*;
Expand Down Expand Up @@ -94,11 +94,11 @@ pub trait ElectrumApi {
/// Tries to fetch `count` block headers starting from `start_height`.
fn block_headers(&self, start_height: usize, count: usize) -> Result<GetHeadersRes, Error>;

/// 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**.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`].

fn relay_fee(&self) -> Result<f64, Error>;
fn relay_fee(&self) -> Result<FeeRate, Error>;

/// Subscribes to notifications for activity on a specific *scriptPubKey*.
///
Expand Down Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn batch_estimate_fee<I>(&self, numbers: I) -> Result<Vec<f64>, Error>
fn batch_estimate_fee<I>(&self, numbers: I) -> Result<Vec<FeeRate>, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow<usize>;
Expand Down
8 changes: 4 additions & 4 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{borrow::Borrow, sync::RwLock};

use log::{info, warn};

use bitcoin::{Script, Txid};
use bitcoin::{FeeRate, Script, Txid};

use crate::api::ElectrumApi;
use crate::batch::Batch;
Expand Down Expand Up @@ -207,12 +207,12 @@ impl ElectrumApi for Client {
}

#[inline]
fn estimate_fee(&self, number: usize) -> Result<f64, Error> {
fn estimate_fee(&self, number: usize) -> Result<FeeRate, Error> {
impl_inner_call!(self, estimate_fee, number)
}

#[inline]
fn relay_fee(&self) -> Result<f64, Error> {
fn relay_fee(&self) -> Result<FeeRate, Error> {
impl_inner_call!(self, relay_fee)
}

Expand Down Expand Up @@ -309,7 +309,7 @@ impl ElectrumApi for Client {
}

#[inline]
fn batch_estimate_fee<'s, I>(&self, numbers: I) -> Result<Vec<f64>, Error>
fn batch_estimate_fee<'s, I>(&self, numbers: I) -> Result<Vec<FeeRate>, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow<usize>,
Expand Down
48 changes: 35 additions & 13 deletions src/raw_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use log::{debug, error, info, trace, warn};

use bitcoin::consensus::encode::deserialize;
use bitcoin::hex::{DisplayHex, FromHex};
use bitcoin::{Script, Txid};
use bitcoin::{FeeRate, Script, Txid};

#[cfg(feature = "use-openssl")]
use openssl::ssl::{SslConnector, SslMethod, SslStream, SslVerifyMode};
Expand All @@ -35,9 +35,11 @@ use rustls::{
pki_types::{Der, TrustAnchor},
ClientConfig, ClientConnection, RootCertStore, StreamOwned,
};
use serde_json::Value;

#[cfg(any(feature = "default", feature = "proxy"))]
use crate::socks::{Socks5Stream, TargetAddr, ToTargetAddr};
use crate::utils::convert_fee_rate;

use crate::stream::ClonableStream;

Expand Down Expand Up @@ -69,6 +71,23 @@ macro_rules! impl_batch_call {

Ok(answer)
}};

( $self:expr, $data:expr, $aux_call:expr, $call:ident, $($apply_deref:tt)? ) => {{
let mut batch = Batch::default();
for i in $data {
batch.$call($($apply_deref)* i.borrow());
}

let resp = $self.batch_call(&batch)?;
let mut answer = Vec::new();

for x in resp {
$aux_call(x);
answer.push(serde_json::from_value(x)?);
}

Ok(answer)
}};
}

/// A trait for [`ToSocketAddrs`](https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html) that
Expand Down Expand Up @@ -857,7 +876,7 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {
Ok(deserialized)
}

fn estimate_fee(&self, number: usize) -> Result<f64, Error> {
fn estimate_fee(&self, number: usize) -> Result<FeeRate, Error> {
let req = Request::new_id(
self.last_id.fetch_add(1, Ordering::SeqCst),
"blockchain.estimatefee",
Expand All @@ -867,10 +886,11 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {

result
.as_f64()
.ok_or_else(|| Error::InvalidResponse(result.clone()))
.map(|val| convert_fee_rate(Value::from(val)))
.expect("Invalid response")
}

fn relay_fee(&self) -> Result<f64, Error> {
fn relay_fee(&self) -> Result<FeeRate, Error> {
let req = Request::new_id(
self.last_id.fetch_add(1, Ordering::SeqCst),
"blockchain.relayfee",
Expand All @@ -880,7 +900,8 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {

result
.as_f64()
.ok_or_else(|| Error::InvalidResponse(result.clone()))
.map(|val| convert_fee_rate(Value::from(val)))
.expect("Invalid response")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

fn script_subscribe(&self, script: &Script) -> Result<Option<ScriptStatus>, Error> {
Expand Down Expand Up @@ -1061,12 +1082,12 @@ impl<T: Read + Write> ElectrumApi for RawClient<T> {
.collect()
}

fn batch_estimate_fee<'s, I>(&self, numbers: I) -> Result<Vec<f64>, Error>
fn batch_estimate_fee<'s, I>(&self, numbers: I) -> Result<Vec<FeeRate>, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow<usize>,
{
impl_batch_call!(self, numbers, estimate_fee, apply_deref)
impl_batch_call!(self, numbers, convert_fee_rate, estimate_fee, apply_deref)
}

fn transaction_broadcast_raw(&self, raw_tx: &[u8]) -> Result<Txid, Error> {
Expand Down Expand Up @@ -1149,20 +1170,21 @@ mod test {
assert_eq!(resp.hash_function, Some("sha256".into()));
assert_eq!(resp.pruning, None);
}

#[test]
fn test_relay_fee() {
let client = RawClient::new(get_test_server(), None).unwrap();

let resp = client.relay_fee().unwrap();
assert_eq!(resp, 0.00001);
let resp = client.relay_fee().unwrap().to_sat_per_vb_ceil();
assert!(resp > 0);
Comment on lines +1178 to +1179
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

}

#[test]
fn test_estimate_fee() {
let client = RawClient::new(get_test_server(), None).unwrap();

let resp = client.estimate_fee(10).unwrap();
assert!(resp > 0.0);
let resp = client.estimate_fee(10).unwrap().to_sat_per_vb_ceil();
assert!(resp > 0);
}

#[test]
Expand Down Expand Up @@ -1288,8 +1310,8 @@ mod test {

let resp = client.batch_estimate_fee(vec![10, 20]).unwrap();
assert_eq!(resp.len(), 2);
assert!(resp[0] > 0.0);
assert!(resp[1] > 0.0);
assert!(resp[0].to_sat_per_vb_ceil() > 0);
assert!(resp[1].to_sat_per_vb_ceil() > 0);
}

#[test]
Expand Down
4 changes: 3 additions & 1 deletion src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,8 @@ pub enum Error {
AllAttemptsErrored(Vec<Error>),
/// There was an io error reading the socket, to be shared between threads
SharedIOError(Arc<std::io::Error>),

/// There was an error parsing a fee rate
FeeRate(String),
Comment on lines +312 to +313
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// Couldn't take a lock on the reader mutex. This means that there's already another reader
/// thread running
CouldntLockReader,
Expand Down Expand Up @@ -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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Expand Down
20 changes: 19 additions & 1 deletion src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//! Utilities helping to handle Electrum-related data.

use crate::types::GetMerkleRes;
use crate::Error;
use bitcoin::hash_types::TxMerkleNode;
use bitcoin::hashes::sha256d::Hash as Sha256d;
use bitcoin::hashes::{Hash, HashEngine};
use bitcoin::Txid;
use bitcoin::{Amount, FeeRate, Txid};
use serde_json::Value;

/// Verifies a Merkle inclusion proof as retrieved via [`transaction_get_merkle`] for a transaction with the
/// given `txid` and `merkle_root` as included in the [`BlockHeader`].
Expand Down Expand Up @@ -41,3 +43,19 @@ pub fn validate_merkle_proof(

cur == merkle_root.to_raw_hash()
}

/// Converts a fee rate in BTC/kB to sats/vbyte.
pub(crate) fn convert_fee_rate(fee_rate_kvb: Value) -> Result<FeeRate, Error> {
let fee_rate_kvb = match fee_rate_kvb.as_f64() {
Some(fee_rate_kvb) => fee_rate_kvb,
None => {
return Err(Error::FeeRate("Fee rate conversion failed".to_string()));
}
};
let fee_rate_sat_vb = (Amount::ONE_BTC.to_sat() as f64) * fee_rate_kvb;
let fee_rate = FeeRate::from_sat_per_vb(fee_rate_sat_vb as u64);
match fee_rate {
Some(fee_rate) => Ok(fee_rate),
None => Err(Error::FeeRate("Fee rate conversion failed".to_string())),
}
}