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

[QoS][REST][CometBFT] Introduce CometQoS: Quality-of-Service for CometBFT & The Interchain #149

Merged
merged 24 commits into from
Feb 5, 2025
Merged

[QoS][REST][CometBFT] Introduce CometQoS: Quality-of-Service for CometBFT & The Interchain #149

merged 24 commits into from
Feb 5, 2025

Conversation

commoddity
Copy link
Contributor

@commoddity commoddity commented Jan 24, 2025 •
edited
Loading

Summary

Adds a CometBFT QoS implementation.

Spec Reference: https://docs.cometbft.com/v0.38/rpc

QoS Checks

  • /health - Simple node heartbeat. Returns empty response if node healthy or error response if node unhealthy.
  • /status - Returns status including node info, pubkey, latest block hash, app hash, block height and time. Only used to get latest block height.

Screenshots

The following screenshots show PATH functioning for cometbft on Shannon after cometbft service added with Supplier backend URL set to https://rpc.cosmos.directory/cosmoshub

1. /health Request
Screenshot 2025-01-24 at 19 26 33

2. /status Request
Screenshot 2025-01-24 at 19 26 50

3. header?height=24106587 Request
Screenshot 2025-01-24 at 19 27 07

4. /header?height=24106587 Request (Envoy Proxy)
Screenshot 2025-01-24 at 19 44 32

5. Hydrator Enabled for cometbft
Screenshot 2025-01-24 at 19 24 47

Changes:

  • Adds qos/cometbft implementation. Built using same pattern as qos/evm package, then modified.
  • Updates cmd/qos.go and config/service_qos.go files to include cometbft.

Issue

Type of change

Select one or more from the following:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Sanity Checklist

  • I have updated the GitHub Issue assignees, reviewers, labels, project, iteration and milestone
  • For docs, I have run make docusaurus_start
  • For code, I have run make test_all
  • For configurations, I have update the documentation
  • I added TODOs where applicable

@commoddity commoddity added the qos Intended to improve quality of service label Jan 24, 2025
@commoddity commoddity added this to the PATH V1 milestone Jan 24, 2025
@commoddity commoddity requested review from Olshansk and adshmh January 24, 2025 19:53
@commoddity commoddity self-assigned this Jan 24, 2025
@Olshansk Olshansk added the rest REST API Support label Jan 29, 2025
Base automatically changed from rest-implementation to main January 29, 2025 21:30
@commoddity commoddity changed the title [Merge after #145] [QoS][REST] feat: adds a qos implementation for cometbft [QoS][REST] feat: adds a qos implementation for cometbft Jan 29, 2025
adshmh
adshmh reviewed Jan 30, 2025
View reviewed changes
observation/gateway.pb.go Outdated Show resolved Hide resolved
adshmh
adshmh reviewed Jan 30, 2025
View reviewed changes
protocol/shannon/context.go Outdated Show resolved Hide resolved
adshmh
adshmh reviewed Jan 30, 2025
View reviewed changes
protocol/shannon/context.go Outdated Show resolved Hide resolved
adshmh
adshmh reviewed Jan 30, 2025
View reviewed changes
qos/cometbft/context.go Outdated Show resolved Hide resolved
adshmh
adshmh reviewed Jan 30, 2025
View reviewed changes
qos/cometbft/routes.go Outdated Show resolved Hide resolved
adshmh
adshmh previously requested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@adshmh adshmh left a comment

Choose a reason for hiding this comment

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

Great PR, thank you.
Only a few blockers, mostly re TODOs.

commoddity
commoddity commented Jan 30, 2025
View reviewed changes
Copy link
Contributor Author

@commoddity commoddity left a comment

Choose a reason for hiding this comment

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

Have addressed all review comments. PTAL and take special note of the change to support both JSON-RPC and REST-like requests in CometBFT QoS.

protocol/shannon/context.go Outdated Show resolved Hide resolved
protocol/shannon/context.go Outdated Show resolved Hide resolved
qos/cometbft/context.go Outdated Show resolved Hide resolved
qos/cometbft/context.go Show resolved Hide resolved
qos/cometbft/routes.go Outdated Show resolved Hide resolved
@commoddity commoddity requested a review from adshmh January 30, 2025 21:17
@Olshansk Olshansk changed the title [QoS][REST] feat: adds a qos implementation for cometbft [QoS][REST][CometBFT] Introduce CometQoS: Quality-of-Service for CometBFT & The Interchain Feb 3, 2025
@Olshansk Olshansk mentioned this pull request Feb 5, 2025
Merged
Olshansk
Olshansk requested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@commoddity PTAL at my review in #155

@commoddity
Copy link
Contributor Author

@commoddity PTAL at my review in #155

Approved. Feel free to merge.

Olshansk and others added 2 commits February 5, 2025 16:33
@Olshansk
[Review] CometQoS Review (#155)
e53ee0c 
Review of #149

Some notes:
- No real structural changes; just naming, nits, comments, etc...
- I tried to wrap my head around the whole thing and went too deep and
out of scope
- Approach is good, but not amazing. We are setting the foundation for
non-technical additions thought
- I understand the need for a refactor now
- We can likely use CometBFT as the "starting point" for the refactor,
but it'll also require farther cleanup, even with this PR

Suggested next steps;
1. Review this PR
2. Merge it into #149 assuming it looks good
3. Add some makefile helpers in #149 specific to CometBFT
4. Merge the whole thing into main
5. Kickoff the refactor
@commoddity
chore: add make target example cometbft request
c924b5c
@commoddity commoddity requested a review from Olshansk February 5, 2025 17:42
Olshansk
Olshansk approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@Olshansk Olshansk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's get this in and keep iterating!

Please make sure the E2E tests in CI pass befor emerging though.

@commoddity commoddity merged commit dad7813 into main Feb 5, 2025
11 checks passed
@commoddity commoddity deleted the cometbft-qos branch February 5, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk approved these changes

@adshmh adshmh Awaiting requested review from adshmh

Assignees

@commoddity commoddity

Labels
qos Intended to improve quality of service rest REST API Support
Projects
Path Roadmap
Status: ✅ Done
Milestone
PATH V1
Development

Successfully merging this pull request may close these issues.
3 participants
@commoddity @Olshansk @adshmh