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: make dyn bitcoin rpc available in ServerModuleInitArgs #6456

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
3 changes: 2 additions & 1 deletion fedimint-bitcoind/src/bitcoincore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use anyhow::{anyhow as format_err, bail};
use bitcoin::{BlockHash, Network, ScriptBuf, Transaction, Txid};
use bitcoincore_rpc::bitcoincore_rpc_json::EstimateMode;
use bitcoincore_rpc::{Auth, RpcApi};
use fedimint_core::bitcoin_rpc::{DynBitcoindRpc, IBitcoindRpc};
use fedimint_core::encoding::Decodable;
use fedimint_core::envs::{BitcoinRpcConfig, FM_BITCOIND_COOKIE_FILE_ENV};
use fedimint_core::module::registry::ModuleDecoderRegistry;
Expand All @@ -17,7 +18,7 @@ use fedimint_core::{apply, async_trait_maybe_send, Feerate};
use fedimint_logging::LOG_CORE;
use tracing::{info, warn};

use crate::{DynBitcoindRpc, IBitcoindRpc, IBitcoindRpcFactory, RetryClient};
use crate::{IBitcoindRpcFactory, RetryClient};

#[derive(Debug)]
pub struct BitcoindFactory;
Expand Down
3 changes: 2 additions & 1 deletion fedimint-bitcoind/src/electrum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use anyhow::{anyhow as format_err, bail};
use bitcoin::{BlockHash, Network, ScriptBuf, Transaction, Txid};
use electrum_client::ElectrumApi;
use electrum_client::Error::Protocol;
use fedimint_core::bitcoin_rpc::{DynBitcoindRpc, IBitcoindRpc};
use fedimint_core::envs::BitcoinRpcConfig;
use fedimint_core::runtime::block_in_place;
use fedimint_core::task::TaskHandle;
Expand All @@ -14,7 +15,7 @@ use hex::ToHex;
use serde_json::{Map, Value};
use tracing::info;

use crate::{DynBitcoindRpc, IBitcoindRpc, IBitcoindRpcFactory, RetryClient};
use crate::{IBitcoindRpcFactory, RetryClient};

#[derive(Debug)]
pub struct ElectrumFactory;
Expand Down
3 changes: 2 additions & 1 deletion fedimint-bitcoind/src/esplora.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ use std::collections::HashMap;

use anyhow::{bail, format_err};
use bitcoin::{BlockHash, Network, ScriptBuf, Transaction, Txid};
use fedimint_core::bitcoin_rpc::{DynBitcoindRpc, IBitcoindRpc};
use fedimint_core::envs::BitcoinRpcConfig;
use fedimint_core::task::TaskHandle;
use fedimint_core::txoproof::TxOutProof;
use fedimint_core::util::SafeUrl;
use fedimint_core::{apply, async_trait_maybe_send, Feerate};
use tracing::info;

use crate::{DynBitcoindRpc, IBitcoindRpc, IBitcoindRpcFactory, RetryClient};
use crate::{IBitcoindRpcFactory, RetryClient};

#[derive(Debug)]
pub struct EsploraFactory;
Expand Down
86 changes: 1 addition & 85 deletions fedimint-bitcoind/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::time::Duration;
use anyhow::Context;
pub use anyhow::Result;
use bitcoin::{BlockHash, Network, ScriptBuf, Transaction, Txid};
use fedimint_core::bitcoin_rpc::{DynBitcoindRpc, IBitcoindRpc};
use fedimint_core::envs::{
BitcoinRpcConfig, FM_FORCE_BITCOIN_RPC_KIND_ENV, FM_FORCE_BITCOIN_RPC_URL_ENV,
};
Expand Down Expand Up @@ -104,91 +105,6 @@ dyn_newtype_define! {
pub DynBitcoindRpcFactory(Arc<IBitcoindRpcFactory>)
}

/// Trait that allows interacting with the Bitcoin blockchain
///
/// Functions may panic if the bitcoind node is not reachable.
#[apply(async_trait_maybe_send!)]
pub trait IBitcoindRpc: Debug {
/// Returns the Bitcoin network the node is connected to
async fn get_network(&self) -> Result<bitcoin::Network>;

/// Returns the current block count
async fn get_block_count(&self) -> Result<u64>;

/// Returns the block hash at a given height
///
/// # Panics
/// If the node does not know a block for that height. Make sure to only
/// query blocks of a height less to the one returned by
/// `Self::get_block_count`.
///
/// While there is a corner case that the blockchain shrinks between these
/// two calls (through on average heavier blocks on a fork) this is
/// prevented by only querying hashes for blocks tailing the chain tip
/// by a certain number of blocks.
async fn get_block_hash(&self, height: u64) -> Result<BlockHash>;

/// Estimates the fee rate for a given confirmation target. Make sure that
/// all federation members use the same algorithm to avoid widely
/// diverging results. If the node is not ready yet to return a fee rate
/// estimation this function returns `None`.
async fn get_fee_rate(&self, confirmation_target: u16) -> Result<Option<Feerate>>;

/// Submits a transaction to the Bitcoin network
///
/// This operation does not return anything as it never OK to consider its
/// success as final anyway. The caller should be retrying
/// broadcast periodically until it confirms the transaction was actually
/// via other means or decides that is no longer relevant.
///
/// Also - most backends considers brodcasting a tx that is already included
/// in the blockchain as an error, which breaks idempotency and requires
/// brittle workarounds just to reliably ignore... just to retry on the
/// higher level anyway.
///
/// Implementations of this error should log errors for debugging purposes
/// when it makes sense.
async fn submit_transaction(&self, transaction: Transaction);

/// If a transaction is included in a block, returns the block height.
/// Note: calling this method with bitcoind as a backend must first call
/// `watch_script_history` or run bitcoind with txindex enabled.
async fn get_tx_block_height(&self, txid: &Txid) -> Result<Option<u64>>;

/// Check if a transaction is included in a block
async fn is_tx_in_block(
&self,
txid: &Txid,
block_hash: &BlockHash,
block_height: u64,
) -> Result<bool>;

/// Watches for a script and returns any transactions associated with it
///
/// Should be called at least prior to transactions being submitted or
/// watching may not occur on backends that need it
/// TODO: bitcoind backend is broken
/// `<https://github.com/fedimint/fedimint/issues/5329>`
async fn watch_script_history(&self, script: &ScriptBuf) -> Result<()>;

/// Get script transaction history
///
/// Note: should call `watch_script_history` at least once, before calling
/// this.
async fn get_script_history(&self, script: &ScriptBuf) -> Result<Vec<Transaction>>;

/// Returns a proof that a tx is included in the bitcoin blockchain
async fn get_txout_proof(&self, txid: Txid) -> Result<TxOutProof>;

/// Returns the Bitcoin RPC config
fn get_bitcoin_rpc_config(&self) -> BitcoinRpcConfig;
}

dyn_newtype_define! {
#[derive(Clone)]
pub DynBitcoindRpc(Arc<IBitcoindRpc>)
}

const RETRY_SLEEP_MIN_MS: Duration = Duration::from_millis(10);
const RETRY_SLEEP_MAX_MS: Duration = Duration::from_millis(5000);

Expand Down
95 changes: 95 additions & 0 deletions fedimint-core/src/bitcoin_rpc.rs
Copy link
Member

Choose a reason for hiding this comment

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

Are we moving IBitcoindRpc to fedimint-core to avoid a circular dependency between fedimint-bitcoind and fedimint-core? Or is there another reason? It's strange to separate IBitcoindRpc from IBitcoindRpcFactory, but it sounds like we might be removing IBitcoindRpcFactory anyway.

If we can't keep IBitcoindRpc in fedimint-bitcoind, it might make sense to just merge all of fedimint-bitcoind into fedimint-core. We can see how further refactors play out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I had to move the IBitcoinRpc trait into fedimint core into fedimint core so I can use it in the ServerModuleInitArgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to turn this into a bigger refactor where at the end we can remove the factory.

Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use std::fmt::Debug;
use std::sync::Arc;

use anyhow::Result;
use bitcoin::{BlockHash, ScriptBuf, Transaction, Txid};
use macro_rules_attribute::apply;

use crate::envs::BitcoinRpcConfig;
use crate::txoproof::TxOutProof;
use crate::{async_trait_maybe_send, dyn_newtype_define, Feerate};

/// Trait that allows interacting with the Bitcoin blockchain
///
/// Functions may panic if the bitcoind node is not reachable.
#[apply(async_trait_maybe_send!)]
pub trait IBitcoindRpc: Debug {
/// Returns the Bitcoin network the node is connected to
async fn get_network(&self) -> Result<bitcoin::Network>;

/// Returns the current block count
async fn get_block_count(&self) -> Result<u64>;

/// Returns the block hash at a given height
///
/// # Panics
/// If the node does not know a block for that height. Make sure to only
/// query blocks of a height less to the one returned by
/// `Self::get_block_count`.
///
/// While there is a corner case that the blockchain shrinks between these
/// two calls (through on average heavier blocks on a fork) this is
/// prevented by only querying hashes for blocks tailing the chain tip
/// by a certain number of blocks.
async fn get_block_hash(&self, height: u64) -> Result<BlockHash>;

/// Estimates the fee rate for a given confirmation target. Make sure that
/// all federation members use the same algorithm to avoid widely
/// diverging results. If the node is not ready yet to return a fee rate
/// estimation this function returns `None`.
async fn get_fee_rate(&self, confirmation_target: u16) -> Result<Option<Feerate>>;

/// Submits a transaction to the Bitcoin network
///
/// This operation does not return anything as it never OK to consider its
/// success as final anyway. The caller should be retrying
/// broadcast periodically until it confirms the transaction was actually
/// via other means or decides that is no longer relevant.
///
/// Also - most backends considers brodcasting a tx that is already included
/// in the blockchain as an error, which breaks idempotency and requires
/// brittle workarounds just to reliably ignore... just to retry on the
/// higher level anyway.
///
/// Implementations of this error should log errors for debugging purposes
/// when it makes sense.
async fn submit_transaction(&self, transaction: Transaction);

/// If a transaction is included in a block, returns the block height.
/// Note: calling this method with bitcoind as a backend must first call
/// `watch_script_history` or run bitcoind with txindex enabled.
async fn get_tx_block_height(&self, txid: &Txid) -> Result<Option<u64>>;

/// Check if a transaction is included in a block
async fn is_tx_in_block(
&self,
txid: &Txid,
block_hash: &BlockHash,
block_height: u64,
) -> Result<bool>;

/// Watches for a script and returns any transactions associated with it
///
/// Should be called at least prior to transactions being submitted or
/// watching may not occur on backends that need it
/// TODO: bitcoind backend is broken
/// `<https://github.com/fedimint/fedimint/issues/5329>`
async fn watch_script_history(&self, script: &ScriptBuf) -> Result<()>;

/// Get script transaction history
///
/// Note: should call `watch_script_history` at least once, before calling
/// this.
async fn get_script_history(&self, script: &ScriptBuf) -> Result<Vec<Transaction>>;

/// Returns a proof that a tx is included in the bitcoin blockchain
async fn get_txout_proof(&self, txid: Txid) -> Result<TxOutProof>;

/// Returns the Bitcoin RPC config
fn get_bitcoin_rpc_config(&self) -> BitcoinRpcConfig;
}

dyn_newtype_define! {
#[derive(Clone)]
pub DynBitcoindRpc(Arc<IBitcoindRpc>)
}
2 changes: 2 additions & 0 deletions fedimint-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ pub mod admin_client;
mod amount;
/// Federation-stored client backups
pub mod backup;
/// Trait to abstract over bitcoin backends
pub mod bitcoin_rpc;
/// Legacy serde encoding for `bls12_381`
pub mod bls12_381_serde;
/// Federation configuration
Expand Down
9 changes: 9 additions & 0 deletions fedimint-core/src/module/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use tracing::Instrument;
// TODO: Make this module public and remove the wildcard `pub use` below
mod version;
pub use self::version::*;
use crate::bitcoin_rpc::DynBitcoindRpc;
use crate::config::{
ClientModuleConfig, ConfigGenModuleParams, DkgPeerMsg, ModuleInitParams, ServerModuleConfig,
ServerModuleConsensusConfig,
Expand Down Expand Up @@ -500,6 +501,7 @@ pub trait IServerModuleInit: IDynCommonModuleInit {
db: Database,
task_group: &TaskGroup,
our_peer_id: PeerId,
dyn_bitcoin_rpc: DynBitcoindRpc,
) -> anyhow::Result<DynServerModule>;

fn validate_params(&self, params: &ConfigGenModuleParams) -> anyhow::Result<()>;
Expand Down Expand Up @@ -580,6 +582,7 @@ where
task_group: TaskGroup,
our_peer_id: PeerId,
num_peers: NumPeers,
dyn_bitcoin_rpc: DynBitcoindRpc,
// ClientModuleInitArgs needs a bound because sometimes we need
// to pass associated-types data, so let's just put it here right away
_marker: marker::PhantomData<S>,
Expand Down Expand Up @@ -608,6 +611,10 @@ where
pub fn our_peer_id(&self) -> PeerId {
self.our_peer_id
}

pub fn dyn_bitcoin_rpc(&self) -> DynBitcoindRpc {
self.dyn_bitcoin_rpc.clone()
}
}
/// Module Generation trait with associated types
///
Expand Down Expand Up @@ -694,6 +701,7 @@ where
db: Database,
task_group: &TaskGroup,
our_peer_id: PeerId,
dyn_bitcoin_rpc: DynBitcoindRpc,
) -> anyhow::Result<DynServerModule> {
<Self as ServerModuleInit>::init(
self,
Expand All @@ -703,6 +711,7 @@ where
db,
task_group: task_group.clone(),
our_peer_id,
dyn_bitcoin_rpc,
_marker: PhantomData,
},
)
Expand Down
7 changes: 7 additions & 0 deletions fedimint-server/src/config/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,7 @@ mod tests {
use fedimint_core::config::{ServerModuleConfigGenParamsRegistry, ServerModuleInitRegistry};
use fedimint_core::db::mem_impl::MemDatabase;
use fedimint_core::db::IRawDatabaseExt;
use fedimint_core::envs::BitcoinRpcConfig;
use fedimint_core::module::ApiAuth;
use fedimint_core::runtime::spawn;
use fedimint_core::task::{sleep, TaskGroup};
Expand Down Expand Up @@ -1038,6 +1039,12 @@ mod tests {
"dummyversionhash".to_owned(),
&module_inits,
TaskGroup::new(),
// This is unused and is just here to get this test to work
BitcoinRpcConfig {
kind: "esplora".to_string(),
url: SafeUrl::parse(&format!("http://127.0.0.1:50002"))
.expect("Failed to parse default esplora server"),
},
)
.await
.expect("Failed to run fedimint server");
Expand Down
3 changes: 3 additions & 0 deletions fedimint-server/src/consensus/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use async_channel::Sender;
use db::get_global_database_migrations;
use fedimint_api_client::api::net::Connector;
use fedimint_api_client::api::DynGlobalApi;
use fedimint_core::bitcoin_rpc::DynBitcoindRpc;
use fedimint_core::config::ServerModuleInitRegistry;
use fedimint_core::core::{ModuleInstanceId, ModuleKind};
use fedimint_core::db::{apply_migrations, apply_migrations_server, Database};
Expand Down Expand Up @@ -54,6 +55,7 @@ pub async fn run(
force_api_secrets: ApiSecrets,
data_dir: PathBuf,
code_version_str: String,
dyn_bitcoin_rpc: DynBitcoindRpc,
) -> anyhow::Result<()> {
cfg.validate_config(&cfg.local.identity, &module_init_registry)?;

Expand Down Expand Up @@ -87,6 +89,7 @@ pub async fn run(
db.with_prefix_module_id(*module_id).0,
task_group,
cfg.local.identity,
dyn_bitcoin_rpc.clone(),
)
.await?;

Expand Down
4 changes: 4 additions & 0 deletions fedimint-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ use std::path::{Path, PathBuf};
use config::io::{read_server_config, PLAINTEXT_PASSWORD};
use config::ServerConfig;
use fedimint_aead::random_salt;
use fedimint_bitcoind::create_bitcoind;
use fedimint_core::config::ServerModuleInitRegistry;
use fedimint_core::db::Database;
use fedimint_core::envs::BitcoinRpcConfig;
use fedimint_core::epoch::ConsensusItem;
use fedimint_core::task::TaskGroup;
use fedimint_core::util::write_new;
Expand Down Expand Up @@ -59,6 +61,7 @@ pub async fn run(
code_version_str: String,
module_init_registry: &ServerModuleInitRegistry,
task_group: TaskGroup,
bitcoin_rpc: BitcoinRpcConfig,
) -> anyhow::Result<()> {
let cfg = match get_config(&data_dir)? {
Some(cfg) => cfg,
Expand Down Expand Up @@ -98,6 +101,7 @@ pub async fn run(
force_api_secrets,
data_dir,
code_version_str,
create_bitcoind(&bitcoin_rpc, task_group.make_handle())?,
)
.await?;

Expand Down
6 changes: 2 additions & 4 deletions fedimint-testing/src/btc/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@ use bitcoin::merkle_tree::PartialMerkleTree;
use bitcoin::{
Address, Block, BlockHash, CompactTarget, Network, OutPoint, ScriptBuf, Transaction, TxOut,
};
use fedimint_bitcoind::{
register_bitcoind, DynBitcoindRpc, IBitcoindRpc, IBitcoindRpcFactory,
Result as BitcoinRpcResult,
};
use fedimint_bitcoind::{register_bitcoind, IBitcoindRpcFactory, Result as BitcoinRpcResult};
use fedimint_core::bitcoin_rpc::{DynBitcoindRpc, IBitcoindRpc};
use fedimint_core::envs::BitcoinRpcConfig;
use fedimint_core::task::{sleep_in_test, TaskHandle};
use fedimint_core::txoproof::TxOutProof;
Expand Down
Loading