Skip to content

feat: implement JSON rpc client and extractor for bitcoin#1980

Merged
DSharifi merged 33 commits intomainfrom
1941-bitcoin-blockhash-extractor
Feb 5, 2026
Merged

feat: implement JSON rpc client and extractor for bitcoin#1980
DSharifi merged 33 commits intomainfrom
1941-bitcoin-blockhash-extractor

Conversation

@DSharifi
Copy link
Contributor

@DSharifi DSharifi commented Feb 5, 2026

closes #1941

@DSharifi DSharifi linked an issue Feb 5, 2026 that may be closed by this pull request
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

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

I have two (small) blockers related to the network requests:

  • We are supporting only POST, but RPCs might also use GET I assume
  • In the headers we only allow a single header. To have full generality, we should accept any number of them

netrome
netrome previously approved these changes Feb 5, 2026
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Good stuff. Some thoughts from my end. My main concern is if it's too limiting to restrict us to one RPC call per inspector. Secondly I think we should use associated types instead of generics in the traits, that would be cleaner. But none of these are blocking, and both can be tackled separately or not at all.

@DSharifi
Copy link
Contributor Author

DSharifi commented Feb 5, 2026

I have two (small) blockers related to the network requests:

* We are supporting only POST, but RPCs might also use GET I assume

This first revision is only supporting the bitcoin core API which uses JSON-RPC. Json-rpc requests sent over HTTP are always POST per the spec.

* In the headers we only allow a single header. To have full generality, we should accept any number of them

This ask of multiple header is relevant to you, @netrome , since your PR is creating the config spec for authentication. I see your design also went with supporting only one header I currently don't see why we would need more than one header for authentication, but it's a simple change.

@gilcu3 Do you have a specific use case in mind for setting multiple authentication headers?

gilcu3
gilcu3 previously approved these changes Feb 5, 2026
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

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

Approving as both blockers I had are not relevant, more context in thread

@DSharifi DSharifi dismissed stale reviews from gilcu3 and netrome via 2239b7c February 5, 2026 13:34
netrome
netrome previously approved these changes Feb 5, 2026
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Thanks for updating to use associated types 🙏

@DSharifi DSharifi added this pull request to the merge queue Feb 5, 2026
Merged via the queue into main with commit 27cd9e8 Feb 5, 2026
14 of 15 checks passed
@DSharifi DSharifi deleted the 1941-bitcoin-blockhash-extractor branch February 5, 2026 16:23
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.

Bitcoin BlockHash extractor

3 participants