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.

Sign up for GitHub

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

Jump to bottom

Unify RPC connection creation for chain backends #7951

Open
3 tasks
yyforyongyu opened this issue Sep 4, 2023 · 3 comments · May be fixed by #9435
Open
3 tasks

Unify RPC connection creation for chain backends #7951

yyforyongyu opened this issue Sep 4, 2023 · 3 comments · May be fixed by #9435
Labels
bitcoind Bitcoin Core backend btcd chain handling intermediate Issues suitable for developers moderately familiar with the codebase and LN P2 should be fixed if one has time

Comments

@yyforyongyu
Copy link
Member

When using bitcoind as the chain backend, lnd creates three RPC connections,

  1. using chain.NewBitcoindConn in chainreg.
  2. using rpcclient.New in chainreg.
  3. using rpcclient.New in chainfee.

This could cause issues as lndmay make concurrent RPC requests to bitcoind which may cause failures. This also breaks the abstraction provided by chain.BitcoindConn as we are now accessing the connection directly. To fix it,

  • replace the rpcclient.New used in 2 and 3 with the chain.BitcoindConn created in 1, to make sure we only ever create one RPC connection to bitcoind.
  • replace other occurrences of rpcclient.New with chain.NewRPCClient for btcd backend.
  • rename chain.NewRPCClient to chain.NewBtcdClient in btcwallet for clarity.
@yyforyongyu yyforyongyu added intermediate Issues suitable for developers moderately familiar with the codebase and LN bitcoind Bitcoin Core backend P2 should be fixed if one has time chain handling btcd labels Sep 4, 2023
@yyforyongyu yyforyongyu mentioned this issue Jan 3, 2024
Merged
@mohamedawnallah
Copy link
Contributor

Hi @yyforyongyu,

I'm interested in working on this issue. After taking a closer look, it seems we need to submit two PRs in a sequential order:

1️⃣ btcsuite/btcwallet Repo

  • Add GetBlockChainInfo and RawRequest APIs to Interface.
  • Implement these APIs for BitcoindClient, and return a "not implemented" error for NeutrinoClient.
  • Update mockChainClient to mock the implementation of these APIs.
  • Rename the RPCClient struct to BtcdClient and change the NewRPCClient method to NewBtcdClient.

2️⃣ lightningnetwork/lnd Repo

  • Modify the backendSupportsTaproot function to depend on the chain.Interface abstraction instead of the concrete rpcclient.Client type.
  • In the BitcoindEstimator struct, use *chain.BitcoindClient instead of *rpcclient.Client.
  • In the BtcdEstimator struct, use *chain.BtcdClient instead of *rpcclient.Client.
  • In the BtcdFilteredChainView struct, use *chain.BtcdClient instead of *rpcclient.Client.

What do you think? 🙏

@mohamedawnallah
Copy link
Contributor

It’s probably easier done than said. I’m going to submit the PR today, and it might be easier to see the diff then. 👍

@yyforyongyu
Copy link
Member Author

yyforyongyu commented Jan 20, 2025 •
edited
Loading

@mohamedawnallah yeah would be nice to see some draft PRs - I have tried locally and realized there are many places need to be changed so that's why the issue was created to track it. In the end, it'd be nice to have the chain backend initialized once and once only here,

lnd/server.go

Lines 2111 to 2118 in baa3b0d

// startLowLevelServices starts the low-level services of the server. These
// services must be started successfully before running the main server. The
// services are,
// 1. the chain notifier.
//
// TODO(yy): identify and add more low-level services here.
func (s *server) startLowLevelServices() error {
var startErr error

This was referenced Jan 22, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bitcoind Bitcoin Core backend btcd chain handling intermediate Issues suitable for developers moderately familiar with the codebase and LN P2 should be fixed if one has time
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

multi: unify RPC connection creations for chain backends mohamedawnallah/lnd
2 participants
@yyforyongyu @mohamedawnallah