Bitcoin RPC Blockchain #22

justinmoon · 2020-06-28T09:57:21Z

No description provided.

justinmoon · 2020-06-28T10:01:08Z

src/blockchain/bitcoin_rpc.rs

+                }
+                false
+            })
+            .any(|x| x);


I'm trying to determine if Bitcoin Core knows about this script.

The only method of determining this that I know of is getaddressinfo, which requires we actually turn the script into an address. Since this module has not notion of "network", I try each one.

src/blockchain/bitcoin_rpc.rs

justinmoon · 2020-06-28T10:05:40Z

src/blockchain/bitcoin_rpc.rs

+                })
+                .collect::<Vec<_>>(),
+            Some(&ImportMultiOptions { rescan }),
+        )?;


Here I export all our scripts. It would be better if we could, say, export our last 100 scripts. Then check if the previous 100 scripts need exporting, etc.

This still isn't ideal, but I think we can iterate on this.

src/blockchain/bitcoin_rpc.rs

justinmoon · 2020-07-11T02:55:57Z

src/blockchain/bitcoin_rpc.rs

+                    .0
+                    .as_mut()
+                    .unwrap()
+                    .get_block_info(&ltx.info.blockhash.unwrap()) // FIXME: don't unwrap


I'm doing this because until now listtransactions doesn't include block heights.

Well Bitcoin 20.0 added this field. So we only need to call getblock if their node version is lower than 20.0.

Once this is merged we can skip this if we find the height (it won't be there for pre-20.0 nodes)

I'm using a rust-bitcoincore-rpc fork until ^^ gets added. This fork hacks around this issue which breaks getaddressinfo on 0.20.0 :/

I should probably swap out the fork before we merge this.

justinmoon · 2020-07-26T23:59:11Z

src/blockchain/bitcoin_rpc.rs

+        vec![].into_iter().collect()
+    }
+
+    async fn setup<D: BatchDatabase + DatabaseUtils, P: Progress>(


It would also be nice if this could create watch-only wallets in Bitcoin Core. Otherwise the caller needs it's own "setup" logic.

To do this Magical would need to know the watch-only wallet name:

pub struct BitcoinRpcBlockchain(Option<Client>, Option<String>);

Where the string is the watch-only wallet name. None would just use default wallet. Or we could not use an Option to force people to use a watch-only wallet, which seems like a better idea. Then the BitcoinRpcBlockchain.setup could:

Call loadwallet

If it succeeds, check if the node recognizes our addresses.

If yes, we're done.

If no, export addresses.

If it fails, call createwallet to create a new watch-only wallet.

If this succeeds, export addresses

I implemented this, but didn't use loadwallet because this RPC call isn't supported yet

- Create feature - Make sync algorithm more batch-friendly - Improve error handling - Add more fields to the RpcBlockchain struct: `wallet_name`, `network`, and `rescan_since` - Create and load watch-only wallets

justinmoon · 2020-08-02T03:18:05Z

This is ready for review.

setup now checks that a valid bitcoin core version is being used and creates / loads watch-only wallets.

sync has been made more batching-friendly (waiting for library support) and attempts reorg protection. I take the highest height associated with any transaction in sled and assume that any transaction 100 blocks or more before this height is considered settled. Our recursive listtransactions calls terminate once such a transaction is observed on the assumption that all older transactions will be "even more settled"!

In normal usage once we add batching, sync will make 3-4 RPC calls depending on node version:

Batched getaddressinfo to see if Bitcoin Core recognizes our addresses. Perhaps this could exist on a separate .import method on the OnlineBlockchain trait?
One listtransactinons call. It will be rare that user has added more "wallet transactions" (inputs and outputs related to the wallet each count separately here) between two sync calls.
Batched gettransaction to fetch the full transaction for each new txid from ^^.
If we're using 0.20.0 we're done. If using earlier node version, then blockheight is not included in either the listtransactions or gettransaction. Thus, we must make a final batched call to getblock to get the block height for each transaction. Perhaps this could be done in parallel with gettranaction calls?

I think this is pretty good. I wonder if that will be fast enough over Tor? In my limited experience doing RPC over Tor, it seems to take a few seconds sometimes. This is where it would be nice to move the import logic to a separate and paralleliz-able call ...

justinmoon · 2020-08-02T03:22:10Z

Cargo.toml

@@ -19,6 +19,7 @@ reqwest = { version = "0.10", optional = true, features = ["json"] }
 futures = { version = "0.3", optional = true }
 clap = { version = "2.33", optional = true }
 base64 = { version = "^0.11", optional = true }
+bitcoincore-rpc = { git = "https://github.com/justinmoon/rust-bitcoincore-rpc", branch = "magical", optional = true }

 # Platform-specific dependencies
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]


Should I do something like this? Bitcoin RPC doesn't work in the browser ...

I think it's fine leaving it in the normal dependencies for now, even the electrum-client doesn't work but it's still up there...

justinmoon · 2020-08-02T03:24:32Z

src/blockchain/rpc.rs

+                    load_wallet_err
+                {
+                    if load_wallet_err.message == format!("Wallet file verification failed: Error loading wallet {}. Duplicate -wallet filename specified.", config.wallet_name) {
+                            info!("Watch-only wallet already loaded: \"{}\"", &config.wallet_name);


Once listwallets support is added to rust-bitcoincore-rpc, then we can remove this ugly hack!

Maybe here you could use the raw call() method to make the listwallets call. Still kinda hacky but less than having to try I guess

justinmoon · 2020-08-02T03:25:44Z

src/blockchain/rpc.rs

+                        panic!(
+                            "couldn't create watch-only bitcoin core wallet: {}",
+                            error.to_string()
+                        );


Don't know what to do here.

wallet_name doesn't exist

can't create wallet_name

Maybe return a Error::BitcoinRpcCreateWalletFailed error or something?

Yeah, i think it makes sense to have a specific error for this

afilini

Overall I really like this, thanks!

I would wait a little bit and see if they merge your fix for the getaddressinfo labels so that we can point the dependency to their git repo. If that starts taking a long time, I can just fork it in this org and we can use that fork temporarily.

I will make a little list of things that are kinda suboptimal now and that could be improved by a future pull request, so that we don't forget. Let me know if you think there's more to add:

Implement RPC batching (waiting for library support)
Use of listwallets (again, waiting for library support or alternative methods, see comment below)
Support pruned nodes by just rescanning the utxo set (or offer this as an option for users who don't care about the full history)
Advertise GetAnyTx on nodes that have txindex enabled. (I'm not sure if this can be checked with an RPC call, maybe we could just add an option and if the user knows that the node is configured correctly, he can enable it)

afilini · 2020-08-03T07:47:58Z

src/blockchain/rpc.rs

+        let db_tip_height = database.iter_txs(false)?.iter().fold(0, |acc, tx| {
+            if let Some(height) = tx.height {
+                std::cmp::max(acc, height)
+            } else {
+                acc
+            }
+        });


Just a suggestion based on my own personal preference, feel free to keep it the way it is if you like it more:

let db_top_height = database.iter_txs(false)?.iter().filter_map(|tx| tx.height).max().unwrap_or(0);

I haven't tested it but I think they should be equivalent. I personally like it more this way because I think the methods used on the iterator express what you are trying to do better (find the maximum height). fold is generally used when you want to squash together all the items of an iterator, so it might be a little more confusing to somebody reading it because that's not really what you are doing here.

afilini · 2020-08-03T08:03:35Z

src/blockchain/rpc.rs

+                        panic!(
+                            "couldn't create watch-only bitcoin core wallet: {}",
+                            error.to_string()
+                        );


Yeah, i think it makes sense to have a specific error for this

afilini · 2020-08-03T08:09:25Z

src/blockchain/rpc.rs

+
+impl OnlineBlockchain for RpcBlockchain {
+    fn get_capabilities(&self) -> HashSet<Capability> {
+        vec![].into_iter().collect()


I think the FullHistory capability should be advertised. It basically means that you can get all the historical transactions of the wallet and not just the ones that have outputs in the UTXO set.

Eventually I think it will make sense to add more options (and sometimes just adapt to the node's configuration) and dinamically add/remove capabilities, like:

If the node has txindex enabled, then it also supports the GetAnyTx capability

If somebody wants to speed up the sync of his wallet or if the node is pruned, instead of doing a full rescan it could just rescan the utxo set. In that case FullHistory should be removed.

Anyways, I think it's fine like this for now, all these improvements could be part of a follow-up pull request.

afilini · 2020-08-03T08:43:59Z

src/blockchain/rpc.rs

+                    load_wallet_err
+                {
+                    if load_wallet_err.message == format!("Wallet file verification failed: Error loading wallet {}. Duplicate -wallet filename specified.", config.wallet_name) {
+                            info!("Watch-only wallet already loaded: \"{}\"", &config.wallet_name);


Maybe here you could use the raw call() method to make the listwallets call. Still kinda hacky but less than having to try I guess

justinmoon · 2020-10-06T17:26:48Z

This is outdated. We can just track this as an issue for now: #79.

Removed Ord/PartialOrd derive rules for types.

justinmoon commented Jun 28, 2020

View reviewed changes

src/blockchain/bitcoin_rpc.rs Outdated Show resolved Hide resolved

justinmoon commented Jun 28, 2020

View reviewed changes

src/blockchain/bitcoin_rpc.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

justinmoon commented Jul 11, 2020

View reviewed changes

justinmoon commented Jul 26, 2020

View reviewed changes

[blockchain] Bitcoin RPC

38a65ee

justinmoon changed the title ~~WIP: Bitcoin RPC~~ Bitcoin RPC Blockchain Aug 2, 2020

justinmoon added 2 commits August 1, 2020 21:50

Polish Bitcoin RPC integration

8d7ca6b

- Create feature - Make sync algorithm more batch-friendly - Improve error handling - Add more fields to the RpcBlockchain struct: `wallet_name`, `network`, and `rescan_since` - Create and load watch-only wallets

Check Bitcoin RPC node version

154b2e4

justinmoon commented Aug 2, 2020

View reviewed changes

afilini reviewed Aug 3, 2020

View reviewed changes

This was referenced Sep 22, 2020

Bitcoin wallet tracker (bwt) client #88

Open

Bitcoin core RPC client #79

Closed

justinmoon closed this Oct 6, 2020

RCasatta mentioned this pull request May 17, 2021

Bitcoin core RPC backend #348

Merged

6 tasks

nickfarrow pushed a commit to nickfarrow/bdk that referenced this pull request Feb 21, 2022

Merge pull request bitcoindevkit#22 from sanket1729/ms-derive-rules

b2f690e

Removed Ord/PartialOrd derive rules for types.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitcoin RPC Blockchain #22

Bitcoin RPC Blockchain #22

justinmoon commented Jun 28, 2020 •

edited

Loading

justinmoon Jun 28, 2020

justinmoon Jun 28, 2020

justinmoon Aug 2, 2020

This comment has been minimized.

justinmoon Jul 11, 2020

justinmoon Jul 27, 2020

justinmoon Aug 2, 2020

justinmoon Jul 26, 2020 •

edited

Loading

justinmoon Aug 2, 2020

justinmoon commented Aug 2, 2020

justinmoon Aug 2, 2020

afilini Aug 3, 2020

justinmoon Aug 2, 2020

afilini Aug 3, 2020

justinmoon Aug 2, 2020

afilini Aug 3, 2020

afilini left a comment

afilini Aug 3, 2020 •

edited

Loading

afilini Aug 3, 2020

afilini Aug 3, 2020

afilini Aug 3, 2020

justinmoon commented Oct 6, 2020

Bitcoin RPC Blockchain #22

Bitcoin RPC Blockchain #22

Conversation

justinmoon commented Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmoon Jul 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmoon commented Aug 2, 2020

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

afilini left a comment

Choose a reason for hiding this comment

afilini Aug 3, 2020 • edited Loading

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

justinmoon commented Oct 6, 2020

justinmoon commented Jun 28, 2020 •

edited

Loading

justinmoon Jul 26, 2020 •

edited

Loading

afilini Aug 3, 2020 •

edited

Loading