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

EXP-189: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types #251

Merged

Conversation

storopoli
Copy link
Member

@storopoli storopoli commented Aug 23, 2024

Description

Replacing our homegrown solution with an off-the-shelf crate,
bitcoind-json-rpc-types(https://crates.io/crates/bitcoind-json-rpc-types).

Also tried to improve all the docstrings of the functions that I had to touch.

I gave up going full rust-bitcoin and replacing all u64s with proper bitcoin::Amount and bitcoin::absolute::Height.
This PR is huge as it is. I'll open a new ticket.

We have 4 kinds of traits for clients that somehow interface with the bitcoin blockchain/node:

  1. BitcoinReader: these only consume stuff from the blockchain.
  2. BitcoinBroadcaster: can broadcast things to the blockchain (for now only has the send_raw_transaction).
  3. BitcoinSigner: the one that signs, i.e. has the private keys.
  4. BitcoinWallet: could be a watch-only wallet that only has Xpubs.

Right now I don't see a huge advantage in that, hence I'll move that to a ticket.

I also got rid of a bunch of custom types and tried to use the ones bitcoind-json-rpc-types provided, and if not possible I've create some new types following the convention of bitcoind-json-rpc-types. These might be upstreamed later as a PR to it. These are:

  • ListUnspent: the result of the call listunspent
  • ListTransactions: the result of call listtransactions
  • SignRawTransactionWithWallet: the result of call signtransactionwithwallet

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

EXP-189
Closes #81.

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 72.00000% with 175 lines in your changes missing coverage. Please review.

Project coverage is 58.20%. Comparing base (cedf7a9) to head (51cc522).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
crates/btcio/src/rpc/types.rs 54.76% 57 Missing ⚠️
crates/btcio/src/test_utils.rs 55.23% 47 Missing ⚠️
crates/btcio/src/rpc/error.rs 0.00% 32 Missing ⚠️
crates/btcio/src/rpc/client.rs 92.00% 18 Missing ⚠️
crates/btcio/src/reader/query.rs 0.00% 7 Missing ⚠️
sequencer/src/main.rs 0.00% 4 Missing ⚠️
crates/btcio/src/broadcaster/task.rs 92.85% 2 Missing ⚠️
crates/btcio/src/writer/config.rs 0.00% 2 Missing ⚠️
crates/btcio/src/writer/task.rs 0.00% 2 Missing ⚠️
crates/btcio/src/broadcaster/handle.rs 0.00% 1 Missing ⚠️
... and 3 more
@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   56.53%   58.20%   +1.67%     
==========================================
  Files         132      133       +1     
  Lines       14223    14404     +181     
==========================================
+ Hits         8041     8384     +343     
+ Misses       6182     6020     -162     
Files with missing lines Coverage Δ
crates/btcio/src/broadcaster/state.rs 97.26% <100.00%> (+1.09%) ⬆️
crates/db/src/types.rs 95.45% <100.00%> (-4.55%) ⬇️
crates/rocksdb-store/src/broadcaster/db.rs 99.46% <100.00%> (ø)
crates/storage/src/ops/l1tx_broadcast.rs 87.93% <100.00%> (+0.43%) ⬆️
crates/btcio/src/broadcaster/handle.rs 45.45% <0.00%> (ø)
crates/btcio/src/writer/builder.rs 97.98% <98.43%> (+0.13%) ⬆️
crates/btcio/src/writer/signer.rs 98.27% <90.00%> (-1.73%) ⬇️
sequencer/src/l1_reader.rs 0.00% <0.00%> (ø)
crates/btcio/src/broadcaster/task.rs 88.17% <92.85%> (+0.23%) ⬆️
crates/btcio/src/writer/config.rs 0.00% <0.00%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

@storopoli storopoli changed the title refactor: replace homegrown Bitcoin RPC for rust-bitcoincore-rpc official crate refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc Aug 23, 2024
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 3 times, most recently from f2da3f7 to 198cc2d Compare August 23, 2024 20:12
@storopoli storopoli changed the title refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc-client Aug 23, 2024
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 3 times, most recently from d3fc8fd to 171f4e8 Compare August 26, 2024 21:25
@storopoli storopoli changed the title refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc-client refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc-types and jsonrpsee Aug 26, 2024
@storopoli storopoli changed the title refactor: replace homegrown Bitcoin RPC for rust-bitcoind-json-rpc-types and jsonrpsee refactor: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee Aug 26, 2024
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 3 times, most recently from ef450e4 to 5feee87 Compare August 27, 2024 19:29
@storopoli storopoli marked this pull request as ready for review August 27, 2024 19:45
@storopoli
Copy link
Member Author

@bewakes can you help me debug what's going with the functional tests?

@storopoli storopoli requested a review from bewakes August 27, 2024 19:46
@storopoli storopoli self-assigned this Aug 27, 2024
@storopoli storopoli changed the title refactor: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee EXP-101: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee Aug 27, 2024
@storopoli storopoli changed the title EXP-101: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee EXP-189: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee Aug 27, 2024
@storopoli storopoli mentioned this pull request Aug 28, 2024
11 tasks
@storopoli
Copy link
Member Author

Ok we are hitting paritytech/jsonrpsee#888 which exactly mentions bitcoind rpc server.
Let's go back to reqwest. Wasted a lot of time thinking that "I" was doing something stupid but in fact it was a mess of jsonrpsee not wanting to deviate from the spec and bitcoind not adhering to the spec.

Theoretically this is fixed in bitcoin/bitcoin#27101 but I cannot see from the release notes which version this was included.
It's better to be safe with reqwest since we don't know which bitcoin core version the user will eventually want to use.

@storopoli storopoli changed the title EXP-189: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types and jsonrpsee EXP-189: replace homegrown Bitcoin RPC for bitcoind-json-rpc-types Aug 29, 2024
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 6 times, most recently from 0c57eda to f2579b8 Compare August 30, 2024 18:08
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 2 times, most recently from f008d22 to 4373844 Compare August 30, 2024 18:18
@storopoli
Copy link
Member Author

Ok this is ready for the reviews.

I have no idea why some functional tests are failing.
I made the API WAY MORE STRICTER with the types that it gets from the underlying bitcoind RPC with proper deserialization into rust-bitcoin types that do some checking under the hood for correctness.

@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch from 4373844 to 1480c85 Compare September 3, 2024 13:35
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Generally looks good. Possibly could be using the storage ops interface in one place but I'm not completely sure.

crates/btcio/src/writer/utils.rs Outdated Show resolved Hide resolved
crates/db/src/types.rs Outdated Show resolved Hide resolved
crates/storage/src/ops/l1tx_broadcast.rs Outdated Show resolved Hide resolved
crates/btcio/src/writer/broadcast.rs Outdated Show resolved Hide resolved
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 4 times, most recently from 7eac515 to f6194c5 Compare September 4, 2024 14:47
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch from f6194c5 to 10b8f69 Compare September 4, 2024 14:50
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Looks good, minor comments.

crates/btcio/src/broadcaster/handle.rs Show resolved Hide resolved
crates/btcio/src/broadcaster/task.rs Outdated Show resolved Hide resolved
crates/btcio/src/broadcaster/task.rs Outdated Show resolved Hide resolved
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch 2 times, most recently from fe4dbb6 to 643d7d3 Compare September 5, 2024 10:47
@storopoli storopoli force-pushed the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch from 8ec5d4f to 51cc522 Compare September 5, 2024 14:14
@storopoli
Copy link
Member Author

@delbonis all tests are now passing (thanks to @bewakes).
I've addressed all comments from last review round.

@delbonis delbonis merged commit 61363b5 into master Sep 5, 2024
14 checks passed
@storopoli storopoli deleted the EXP-101-migrate-btcio-rpc-to-rust-bitcoincore-rpc-v3 branch September 5, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace homegrown Bitcoin RPC client with an off-the-shelf one
2 participants