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
Bitcoin core RPC backend #348
Conversation
oh hi, deja vu! I need this for a project, trying it now... |
ImportMultiRequestScriptPubkey, ImportMultiRescanSince, | ||
}; | ||
use bitcoincore_rpc::jsonrpc::serde_json::Value; | ||
use bitcoincore_rpc::{Auth, Client, RpcApi}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should re-export bitcoincore_rpc
in src/lib.rs
? RpcConfig
depends on Auth
so you can't construct it otherwise.
diff --git a/src/lib.rs b/src/lib.rs
index f5323b1..477f023 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -216,6 +216,9 @@ extern crate bdk_macros;
#[cfg(feature = "compact_filters")]
extern crate lazy_static;
+#[cfg(feature = "rpc")]
+pub extern crate bitcoincore_rpc;
+
#[cfg(feature = "electrum")]
pub extern crate electrum_client;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, adding...
Before final merge maybe we can assess if only Auth is needed we may want to import only that struct
src/blockchain/rpc.rs
Outdated
|
||
//TODO if current_height == node_synced should check only the mempool | ||
|
||
//TODO split the interval in chunk so that we can give progress update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of start/stop params which involves state, we could just periodically call getwalletinfo
for the progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reconsidered again, see #348 (comment)
|
||
//TODO if it's a big rescan, spawn a thread that checks getwalletinfo and update progress | ||
self.client | ||
.rescan_blockchain(Some(node_synced as usize), Some(current_height as usize))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing I noticed is that if this call is ever interrupted during a long rescan from 0, set_node_synced_height
is never called, and then the next time sync happens it will rescan from 0 again. not sure what the best way to handle this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right, maybe it's better to return to the initially proposed logic of doing the rescan in chunks, for example, every 10_000 blocks, this would solve both the progress (without a separated thread for the getwalletinfo
calss) and make the restart from crash less bad
} | ||
|
||
impl RpcBlockchain { | ||
fn get_node_synced_height(&self) -> Result<u32, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about making these methods public? I was able to implement a "quick sync" for new wallets by setting node_synced_height to the current tip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be a config parameter?
in compact_filters is passed at creation time https://docs.rs/bdk/0.7.0/bdk/blockchain/compact_filters/struct.CompactFiltersBlockchain.html#method.new
Yeah, right, maybe it's better to return to the initially proposed
logic of doing the rescan in chunks, for example, every 10_000 blocks,
this would solve both the progress (without a separated thread for the
`getwalletinfo` calss) and make the restart from crash less bad
yup I came to the same conclusion after thinking about it some more 👍
|
Maybe it should be a config parameter?
in compact_filters is passed at creation time https://docs.rs/bdk/0.7.0/bdk/blockchain/compact_filters/struct.CompactFiltersBlockchain.html#method.new
either way works for me! having a similar interface to the other backends sounds like a good thing.
|
rebased on top of #347 |
we should export this under the blockchain module like the others: diff --git a/src/blockchain/mod.rs b/src/blockchain/mod.rs
index 95069ec..b9f5e3c 100644
--- a/src/blockchain/mod.rs
+++ b/src/blockchain/mod.rs
@@ -43,6 +43,11 @@ pub use self::electrum::ElectrumBlockchain;
#[cfg(feature = "electrum")]
pub use self::electrum::ElectrumBlockchainConfig;
+#[cfg(feature = "rpc")]
+pub use self::rpc::RpcBlockchain;
+#[cfg(feature = "rpc")]
+pub use self::rpc::RpcConfig;
+
#[cfg(feature = "esplora")]
#[cfg_attr(docsrs, doc(cfg(feature = "esplora")))]
pub mod esplora;
|
Rebased on master and squashed |
5dd8833
to
c111e5c
Compare
#[cfg(feature = "rpc")] | ||
pub mod rpc; | ||
#[cfg(feature = "rpc")] | ||
pub use self::rpc::RpcBlockchain; | ||
#[cfg(feature = "rpc")] | ||
pub use self::rpc::RpcConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new RPC variant should be added to src/blockchain/any.rs
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, right, but I think we need to defer this (maybe following PR) until rust-bitcoin/rust-bitcoincore-rpc#181 is merged and released (initial work for reference d13b925)
for tx_result in list_txs.iter().filter(|t| { | ||
// list_txs returns all conflicting tx we want to | ||
// filter out replaced tx => unconfirmed and not in the mempool | ||
t.info.confirmations > 0 || self.client.get_mempool_entry(&t.info.txid).is_ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here we should differentiate between the tx not being in the mempool and other random errors like networking issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's not too hard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we can discriminate between random errors and not being in the mempool...
updated version of #22
closes #79 and #86
needsrebased on top of #347 for common blockchain testsDescription
Implements Blockchain trait for a backend using an RPC node
Notes to the reviewers
Note we are obliged to use importmulti because bitcoin core doesn't support all descriptor https://bitcoindevkit.org/descriptors/#compatibility-matrix
use bitcoind crate as dev-dependency, requires env var pointing to a bitcoind executable, see for example https://github.com/RCasatta/bitcoind/blob/master/.github/workflows/test.yml
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
CHANGELOG.md