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

refactor: make bitcoin implementation flexible #1092

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Abdulkbk
Copy link
Contributor

@Abdulkbk Abdulkbk commented Dec 30, 2024

Description

This PR refactors the bitcoin implementation.

Currently, the implementation only supports bitcoind, making it less flexible. This PR allows for the easy integration of other bitcoin implementations, such as BTCD, in the future.

Approach:
The PR refactors the Bitcoin implementation to utilize BitcoinFactory, similar to the implementation for the Lightning network, which allows for various Lightning implementations in Polar. This change will also improve the flexibility of the Bitcoin implementation.

Steps to Test

Since this is a refactor PR and does not change any behavior in Polar, you can test it normally by starting Polar and creating a network that involves bitcoind.

Here are a few pointers to test:

  • Conduct transactions between two or more bitcoind nodes.
  • Utilize bitcoind backends for some Lightning nodes and perform various operations.

@Abdulkbk Abdulkbk changed the title Btcd support Feat: btcd support Dec 30, 2024
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4e4192f) to head (7cfbed7).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1092   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          152       154    +2     
  Lines         5547      5567   +20     
  Branches      1082      1128   +46     
=========================================
+ Hits          5547      5567   +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamaljsr
Copy link
Owner

jamaljsr commented Jan 3, 2025

Hey @Abdulkbk thanks for opening this draft PR to show what you've implemented so far. I skimmed through the diff and so far it's looking fantastic! I'm pleasantly surprised to see the test coverage still at 100%. 👌

Please let me know if you have any questions or need any assistance moving forward with this.

@Abdulkbk
Copy link
Contributor Author

Abdulkbk commented Jan 6, 2025

Hey @jamaljsr thanks for taking the time to skim through this.

I'm pleasantly surprised to see the test coverage still at 100%.

Yes, I try to add test to all new code as I add them👌.

Please let me know if you have any questions or need any assistance moving forward with this.

Of course I will. I've been thinking of the best way I should add thebtcdservice but I'm inclined towards using REST as I couldn't find a standard library similar to the one used for bitcoindService.

@Abdulkbk
Copy link
Contributor Author

Hey @jamaljsr, I am currently facing two blockers. I figured out a way for the first but I am still figuring out the second.

The first is that BTCD has wallet functionalities decoupled from chain functionalities, unlike bitcoind. However, there are workarounds. For example, I can run BTCD alongside btcwallet, and then use btcwallet for all non-CLI calls, as it supports both chain and wallet-related RPCs.

The second blocker is that btcwallet does not support regtest currently, meanwhile all the other node implementations in Polar use regtest. I think this is the real challenge.

I would love to hear your thoughts as I continue to figure a way out. Thanks.

@jamaljsr
Copy link
Owner

Hi @Abdulkbk,

The second blocker is that btcwallet does not support regtest currently, meanwhile all the other node implementations in Polar use regtest. I think this is the real challenge.

I would love to hear your thoughts as I continue to figure a way out. Thanks.

Oh wow, you are correct! btcd uses a different network for local testing called simnet. I totally forgot about this. While lnd does have support for simnet, the other implementations do not AFAIK. So if we move forward, we'd have to prevent adding CLN/Eclair nodes when there is no bitcoind present in the network. We'd also have to prevent mixing btcd and bitcoind in the same network since they won't be able to communicate with each other. This problem is going to really increase the effort required for this PR substantially due to the added compatibility complexity.

I honestly don't have all the answers right now on how to implement this best, but I'm willing to work with you if you are still interested in pursuing this. My hunch is that there will be more challenges along the way. We'll likely need to split this up into multiple PRs so reviewing the updates won't be too difficult. Let me know if you'd like to continue with this. I'd totally understand if this feels like too large of a project to tackle at this time.

@Abdulkbk
Copy link
Contributor Author

This problem is going to really increase the effort required for this PR substantially due to the added compatibility complexity.

I agree this will require much more effort.

We'll likely need to split this up into multiple PRs so reviewing the updates won't be too difficult. Let me know if you'd like to continue with this. I'd totally understand if this feels like too large of a project to tackle at this time.

I would like to take this further. I will perhaps start by modifying this PR to focus on refactoring the implementation of the Bitcoin nodes to use a factory. Once I’ve completed that, I will request a review.

In this commit, we refactor the designer components to make it
possible for us to introduce another bitcoin node implementation
later.
In this commit, we refactor how bitcoind is implemented and used,
and moved to using injected factory so that other node implementa
tions can be added later.
In this commit, we refactor the test files to use the new bitcoin
factory implementation. We also added a test for the newly created
notImplemented service file for bitcoin nodes.
@Abdulkbk Abdulkbk changed the title Feat: btcd support refactor: make bitcoin implementation flexible Jan 16, 2025
@Abdulkbk Abdulkbk marked this pull request as ready for review January 20, 2025 21:24
@Abdulkbk
Copy link
Contributor Author

Abdulkbk commented Jan 20, 2025

Hey @jamaljsr, I think this PR is ready for review. Thanks

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.

2 participants