Skip to content

Conversation

@mohamedawnallah
Copy link
Collaborator

@mohamedawnallah mohamedawnallah commented Jan 22, 2025

Description

In this commit, we add new APIs to chain interface used across btcd, neutrino, and bitcoind chain backends. The new APIs are GetPeerInfo, GetBlockChainInfo, and RawRequest.

Addresses part of lightningnetwork/lnd#7951.
Dependent PR lightningnetwork/lnd#9435.

Motivation and Context

The motivation behind adding those APIs is to easier the process of unifying RPC connection creations across chain backends as part of LND Daemon.

@mohamedawnallah mohamedawnallah changed the title [1/2] wallet+chain: add new APIs GetPeerInfo, GetBlockChainInfo, and RawRequest to chain interface wallet+chain: add new APIs GetPeerInfo, GetBlockChainInfo, and RawRequest to chain interface Jan 22, 2025
@Roasbeef
Copy link
Member

Approved CI.

@mohamedawnallah mohamedawnallah force-pushed the integrateNewChainInterfaceAPIs branch from 1d91cdd to 8a4f7c9 Compare January 29, 2025 01:23
In this commit, we add new APIs to chain interface
used across `btcd`, `neutrino`, and `bitcoind` chain
backends. The new APIs are `GetPeerInfo`,
`GetBlockChainInfo`, and `RawRequest`.

The motivation behind adding those APIs is to easier the
process of unifying RPC connection creations across chain
backends as part of LND Daemon.
In this commit, we decouple `BitcoindClient.GetBlockHeight`
method from btcd library by making it utilize
`BitcoindClient.GetBlockHeaderVerbose` instead.
@mohamedawnallah mohamedawnallah force-pushed the integrateNewChainInterfaceAPIs branch from 8a4f7c9 to 96f4d5a Compare January 29, 2025 01:43
@mohamedawnallah
Copy link
Collaborator Author

mohamedawnallah commented Jan 29, 2025

It looks like approval is required whenever an updated commit is submitted. Additionally, it doesn’t seem to be configured to trigger against the master branch in the forked repository.

I'm also unsure about the linting process when running make lint locally. For example, I'm encountering linting issues for code blocks I haven’t modified, such as FilterBlocks and handler:

chain/btcd.go:316: Function 'FilterBlocks' is too long (77 > 60) (funlen)
func (c *BtcdClient) FilterBlocks(
chain/btcd.go:496: Function 'handler' is too long (72 > 60) (funlen)
func (c *BtcdClient) handler() {
rpc/legacyrpc/methods.go:974: Function 'help' is too long (78 > 60) (funlen)
func help(icmd interface{}, _ *wallet.Wallet,
rpc/legacyrpc/methods.go:1593: Function 'signRawTransaction' has too many statements (86 > 40) (funlen)
...

EDIT:
How am I supposed to move this PR forward? The linter is requiring considerable code refactoring of the FilterBlocks, handler, and help functions after modifying their signatures. However, these refactoring changes are unrelated to the PR's original purpose. What's the best way to handle this situation?

@Roasbeef
Copy link
Member

Roasbeef commented Feb 5, 2025

How am I supposed to move this PR forward? The linter is requiring considerable code refactoring of the FilterBlocks, handler, and help functions after modifying their signatures. However, these refactoring changes are unrelated to the PR's original purpose. What's the best way to handle this situation?

Usually in other projects we're able to specifying a starting commit in the linter. It only attempts to flag linting errors of new changes after that commit. Looks like we're missing that config here.

So I think we should either add that starting commit (here's an example for lnd), or we can move forward wit hthis PR, verifying that no new linter violations are hit.

@Abdulkbk
Copy link
Contributor

Abdulkbk commented Apr 2, 2025

Thanks for the PR @mohamedawnallah. Before diving into the changes, I have a quick question: would renaming RPCClient to BtcdClient here result in backward compatibility issues for projects using btcwallet, or are we assuming that only LND uses it?

@yyforyongyu
Copy link
Collaborator

yyforyongyu commented Apr 7, 2025

Thanks for the PR @mohamedawnallah. Before diving into the changes, I have a quick question: would renaming RPCClient to BtcdClient here result in backward compatibility issues for projects using btcwallet, or are we assuming that only LND uses it?

Good question. I think we should keep it backward compatible. Maybe keep RPCClient and point it to BtcdClient? Many other projects use this package.

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.

4 participants