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

Support JSON-RPC verifier #67

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johncantrell97
Copy link
Contributor

  • Updates the verifier to rely on both BlockSource and UtxoSource traits which the RestClient and RpcClient both implement. Removing the unnecessary manual implementation of getblockbyhash.
  • Introduces an enum AnyChainSource that also implements BlockSource and UtxoSource that the verifier can use.
  • Add support for bitcoind JSON-RPC api using the RpcClient.

The default behavior of using RestClient is unchanged. This allows a user to switch to the RpcClient by setting USE_BITCOIN_RPC_API to "true" and providing the various BITCOIN_RPC_* vars (host, port, path, user, password).

@TheBlueMatt
Copy link
Contributor

Oh, can we actually replace most of the ChainVerifier code with the GossipVerifier in lightning-block-sync? It was added here and then mostly upstreamed but never removed here in favor of the upstream version.

@johncantrell97
Copy link
Contributor Author

Oh, can we actually replace most of the ChainVerifier code with the GossipVerifier in lightning-block-sync? It was added here and then mostly upstreamed but never removed here in favor of the upstream version.

Ah whoops, I thought it was strange this wasn't using that. I assumed there was some important difference. Will give it a shot.

@johncantrell97
Copy link
Contributor Author

johncantrell97 commented Dec 5, 2023

It's appears to be impossible to use GossipVerifier here.

The GossipVerifier assumes your PeerManager will use a P2PGossipSync as your RoutingMessageHandler but RGS PeerManager does not. It uses two of them wrapped in a GossipRouter.

Unless there's some simple way to get around this maybe we can push this change to later?

I could see relaxing the PeerManager constraint on GossipVerifier to be APeerManager or even completely generic RMH but I don't see an easy way to get rid of the P2PGossipSync requirement since in order to resolve the UtxoFuture it calls a method implemented directly on P2PGossipSync and not some method on a trait RGS's router can implement.

edit

Actually, I guess we can give the outbound gossiper to GossipVerifier as the gossiper but still need to a way to relax the PeerManager constraints

edit again

I just opened this to fix this (I think): lightningdevkit/rust-lightning#2773

Comment on lines +21 to +24
enum AnyChainSource {
Rest(RestClient),
Rpc(RpcClient),
}
Copy link

Choose a reason for hiding this comment

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

Could we use a type parameter instead of introducing an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it bleeds into the entire application and felt unnecessary when RGS is a binary and not really a lib. But I can go back and do this if you think it's better once we resolve the GossipVerifier issues.

@TheBlueMatt
Copy link
Contributor

LGTM. Needs a trivial rebase, however.

@TheBlueMatt
Copy link
Contributor

Any desire to rebase this?

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.

3 participants