[WIP] JUDGE QoS framework v0 #253

adshmh · 2025-05-09T18:58:45Z

Summary

JUDGE: new framework for building QoS systems on PATH.

Primary Changes:

Introduce the core concepts of JUDGE: 4 Contexts of operation
Define and implement key components for implementing the 4 defined key contexts.
Define new proto files as data containers for JUDGE
Separate Data Container and logic functionality in components

Secondary Changes:

Automate observations
Automate Request processing
Automate Response processing
Automate Edge cases processing (e.g. protocol-level errors)

Issue

Repetitive/boiler plate code in qos package, which masked the actual QoS logic.

Issue or PR: #{ISSUE_OR_PR_NUMBER}

Type of change

Select one or more from the following:

QoS Checklist

E2E Validation & Tests

make path_up
make test_e2e_evm_shannon
make test_e2e_evm_morse

Observability

1. make path_up
2. Run one of the following:
- For Shannon with anvil: make test_request__shannon_relay_util_100
- For Morse with F00C: make test_request__morse_relay_util_100
3. Visit PATH Relay Grafana Dashboard to view results

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 updated the documentation
I added TODOs where applicable

adshmh · 2025-05-09T19:00:21Z

Please note the UPNEXT items below:

Architecture.md doc.
toolkit package implementation (JUDGE works without it, but this will make QoS implementation even easier).
Resolving conflicts
Remove the Draft status.

commoddity

I'm signing off for the day and leaving this as a partial review that I will pick up tomorrow.

@adshmh just want to say that I love the design and direction of this. IMO this is a major improvements and I can really see how it's incorporating all of the learnings we've made in developing the QoS system from 0 to where it's at now.

This is definitely the right direction and I'm excited to see the finished PR. 🚀

commoddity · 2025-05-14T14:42:51Z

proto/path/qos/framework/request.proto

+}
+
+// RequestError contains details about a request error
+message RequestError {


Should this file be called request_error?

commoddity · 2025-05-14T14:53:36Z

qos/example_evm/check.go

@@ -0,0 +1,3 @@
+package evm
+
+// TODO_IN_THIS_PR: use the framework's EndpointQualityCheckContext for adding QoS checks.


@adshmh I'm confused about the purpose of the example_evm folder. Is this meant to be an in-progress development folder or is this something that will be included in the final version of JUDGE?

commoddity · 2025-05-14T15:02:59Z

qos/judge/README.md

+
+```go
+// Create a QoS specification with probes for each method
+spec := toolkit.QoSSpec{


So far this design looks amazing; very intuitive.

Is the final idea for configuring PATH that we would setup a series of toolkit.QoSSpec to replace the current service configurations in config/service_qos_config.go?

commoddity · 2025-05-14T18:09:46Z

qos/judge/client_http_response.go

+// and provides a standardized way to return HTTP responses to clients.
+type ClientHTTPResponse struct {
+	StatusCode int
+	Headers    map[string]string


Any reason we can't use http.Header as the type here?

commoddity · 2025-05-14T18:12:03Z

qos/judge/context_endpoint_checks.go

+	endpointChecksToPerform []*jsonrpc.Request
+}
+
+func (ctx *EndpointQualityChecksContext) buildEndpointQualityCheckContexts() []gateway.RequestQoSContext {


#PUC for this method.

commoddity · 2025-05-14T18:59:42Z

qos/judge/context_endpoint_selection.go

+	"github.com/buildwithgrove/path/protocol"
+)
+
+// TODO_FUTURE(@adshmh): Rank qualified endpoints, e.g. based on latency, for selection.


Suggested change

// TODO_FUTURE(@adshmh): Rank qualified endpoints, e.g. based on latency, for selection.

// TODO_FUTURE(@adshmh,@commoddity): Rank qualified endpoints, e.g. based on latency, for selection.

I have a functional PR that does this already so I'm happy to take on this work once JUDGE is merged to main.

commoddity · 2025-05-14T19:00:27Z

qos/judge/context_endpoint_selection.go

+
+	// request's journal.
+	// Provide read-only access to the request, e.g. the JSONRPC method.
+	*requestJournal


I like the name 'journal` for this logic.

commoddity · 2025-05-14T19:00:53Z

qos/judge/context_endpoint_selection.go

+	customSelector EndpointSelector
+
+	// Endpoints disqualified from current selection context.
+	disqualifiedEndpoints map[protocol.EndpointAddr]struct{}


Very nice to provide this alongside valid endpoints.

commoddity · 2025-05-14T19:03:12Z

qos/judge/context_endpoint_selection.go

+		}
+
+		// Call the endpoint filters on the endpoint.
+		// Endpoint will be disqualified if any filter reutrns an error.


Suggested change

// Endpoint will be disqualified if any filter reutrns an error.

// Endpoint will be disqualified if any filter returns an error.

commoddity · 2025-05-14T19:06:15Z

qos/judge/context_request.go

+
+// TODO_IN_THIS_PR: sort out the scope of fields and methods: private/public on private structs.
+//
+// requestQoSContext holds the context for a request through its lifecycl.


Suggested change

// requestQoSContext holds the context for a request through its lifecycl.

// requestQoSContext holds the context for a request through its lifecycle.

commoddity

Finished my first round of review.

First of, want to reiterate that I really love the direction of this PR. IT's a great improvement all around over the repetitive code we currently have. Great work and I'm looking forward to reviewing the finished PR.

Not requesting changes since I know it's a WIP so just leaving some observations and comments on the current state. 🔥

commoddity · 2025-05-16T13:38:38Z

qos/judge/context_state_update.go

+	paramsToUpdate := ctx.stateUpdater(ctx)
+
+	// Update the state parameters through the service state.
+	return ctx.ServiceState.updateParameters(paramsToUpdate)


Given the above comment re: ServiceState being read-only, it's a little confusing that there is an updateParameters method here. Is it possible to clarify this in comments?

commoddity · 2025-05-16T13:39:52Z

qos/judge/endpoint.go

+	// queryResults maps keys to query results for this endpoint.
+	// The map key is the method of the JSONRPC request for which the query result was built.
+	// Examples:
+	// - "eth_blockNumber": &EndpointQueryResult{IntValues: {"blockNumber": 0x1234}}


Great comment here; makes it nice and clear what the data actually is.

commoddity · 2025-05-16T13:47:38Z

qos/judge/endpoint_query_result.go

+	parsedJSONRPCResponse *jsonrpc.Response
+
+	// The set of values/attributes extracted from the endpoint query and the endpoint's parsed JSONRPC response.
+	// e.g. for a Solana `getEpochInfo` request, the custom service could derive two endpoint attributes as follows:


Once again great comment here.

commoddity · 2025-05-16T13:49:00Z

qos/judge/endpoint_query_result.go

+		ErrorKind:   EndpointErrKindInvalidResult,
+		Description: description,
+		RecommendedSanction: &Sanction{
+			Type:       SanctionTypeTemporary,


#PUC on why this is always SanctionTypeTemporary.

EDIT - nm just saw PermanentSanction below. Should we maybe call this TemporarilySanctionEndpoint or something similar?

commoddity · 2025-05-16T13:54:16Z

qos/judge/endpoint_sanction.go

+	SanctionTypePermanent                       // Permanent exclusion
+)
+
+// Sanction represents a recommendation to limit endpoint usage.


Not a strong opinion but perhaps this file would benefit from more detail for the reader on what exactly a "sanction" is in the context of JUDGE? ie. making this comment more explicit that a sanction means not selecting an endpoint.

commoddity · 2025-05-16T14:21:44Z

qos/judge/observations_endpoint_result.go

+	if len(eqr.StrValues) > 0 {
+		obs.StringValues = make(map[string]string)
+	}
+	for key, value := range eqr.StrValues {
+		obs.StringValues[key] = value
+	}


Suggested change

if len(eqr.StrValues) > 0 {

obs.StringValues = make(map[string]string)

}

for key, value := range eqr.StrValues {

obs.StringValues[key] = value

}

if len(eqr.StrValues) > 0 {

obs.StringValues = make(map[string]string,)

maps.Copy(obs.StringValues, eqr.StrValues)

}

commoddity · 2025-05-16T14:23:44Z

qos/judge/observations_endpoint_result.go

+	if len(eqr.IntValues) > 0 {
+		obs.IntValues = make(map[string]int64)
+	}
+	for key, value := range eqr.IntValues {
+		obs.IntValues[key] = int64(value)
+	}


Any reason that eqr.IntValues must be map[string]int but obs.IntValues is map[string]int64?

If it's not possible to change feel free to ignore the below suggestion.

Suggested change

if len(eqr.IntValues) > 0 {

obs.IntValues = make(map[string]int64)

}

for key, value := range eqr.IntValues {

obs.IntValues[key] = int64(value)

}

if len(eqr.IntValues) > 0 {

obs.IntValues = make(map[string]int64)

maps.Copy(obs.IntValues, eqr.IntValues)

}

commoddity · 2025-05-16T14:25:37Z

qos/judge/observations_endpoint_result.go

+	// Copy string values
+	if len(observation.StringValues) > 0 {
+		result.StrValues = make(map[string]string)
+	}
+	for key, value := range observation.StringValues {
+		result.StrValues[key] = value
+	}
+
+	// Copy int values
+	if len(observation.IntValues) > 0 {
+		result.IntValues = make(map[string]int)
+	}
+	for key, value := range observation.IntValues {
+		result.IntValues[key] = int(value)
+	}


Ditto the above. Feel free to ignore if result.IntValue cannot be map[string]int64.

Suggested change

// Copy string values

if len(observation.StringValues) > 0 {

result.StrValues = make(map[string]string)

}

for key, value := range observation.StringValues {

result.StrValues[key] = value

}

// Copy int values

if len(observation.IntValues) > 0 {

result.IntValues = make(map[string]int)

}

for key, value := range observation.IntValues {

result.IntValues[key] = int(value)

}

// Copy string values

if len(observation.StringValues) > 0 {

result.StrValues = make(map[string]string)

maps.Copy(result.StrValues, observation.StringValues)s

}

// Copy int values

if len(observation.IntValues) > 0 {

result.IntValues = make(map[string]int)

maps.Copy(result.IntValues, observation.IntValues)

}

commoddity · 2025-05-16T15:08:24Z

qos/judge/state.go

+	// mu protects the state map from concurrent access
+	mu sync.RWMutex
+
+	// stateParameters


Perhaps examples of these values could be would be helpful in the comment.

commoddity · 2025-05-16T15:08:54Z

qos/judge/state.go

+
+}
+
+func (s *ServiceState) GetConsensusParam(paramName string) (map[string]int, bool) {


#PUC for this since it's a bit more complicated that the simple values. Specifically what it's used for (eg. archival checks).

Olshansk

@adshmh I'm leaving a comment for posterity.

After you tend/reply to @commoddity's comments AND merge with main, I'll aim to do a first round next week.

Thank you in advance for maintaining and upkeeping such a major PR - I know it's not easy!

Olshansk

@adshmh I'm leaving a comment for posterity.

After you tend/reply to @commoddity's comments AND merge with main, I'll aim to do a first round next week.

Thank you in advance for maintaining and upkeeping such a major PR - I know it's not easy!

Olshansk

@adshmh I'm leaving a comment for posterity.

After you tend/reply to @commoddity's comments AND merge with main, I'll aim to do a first round next week.

Thank you in advance for maintaining and upkeeping such a major PR - I know it's not easy!

adshmh added 26 commits April 7, 2025 16:13

TEMP commit on framework/jsonrpc package

5e1c333

introduce a requestJournal to track all data related to a request

0aded17

Refactor: merge endpointAttribute and endpointQueryResult

4169447

Make QoS struct the context builder

6f10e48

Introduce request struct

cf94a95

Remove now-obsolete requestErrorContext

6284daf

Implement request observations.

bcf5c11

Handle NoResponse, i.e. Protocol-level error

99aa007

Update proto definitions to match endpoint structs

80df05f

Implement observation logic

d67cc05

Drop obsolete proto files

09bdbff

Move all files to framework directory

57e84f8

Add ObservationsInterpreter placeholder

19677b0

Update framework initialization in EVM

b57082d

Update package name to framework

f5cc4eb

Move example EVM up one level

e1b37e9

Regenerate observation package

47642d5

Refactor proto files to drop redundant endpoint.proto

ed28bc8

Refactor: consolidate all details/events of a request in requestJournal

ba649eb

Refactor: make requestJournal top level struct in observations

848fe1c

Fix syntax errors

4b2d70d

Fix syntax errors

c3375c7

Fix syntax errors

3c3b500

Refactor observations: requestError

56c0535

Implement the EndpointQualityChecks context

0996e83

WIP commit: adding toolkit to judge

bcef0ad

adshmh added this to the Portal -> PATH Migration milestone May 9, 2025

adshmh self-assigned this May 9, 2025

adshmh added enhancement qos Intended to improve quality of service labels May 9, 2025

adshmh requested review from Olshansk and commoddity May 9, 2025 19:05

commoddity reviewed May 14, 2025

View reviewed changes

commoddity reviewed May 16, 2025

View reviewed changes

Olshansk suggested changes May 16, 2025

View reviewed changes

Olshansk modified the milestones: Portal -> PATH Migration, PATH to 100% Traffic Jun 14, 2025

Olshansk removed the enhancement label Jun 30, 2025

Olshansk changed the title ~~JUDGE QoS framework v0~~ [WIP] JUDGE QoS framework v0 Jul 17, 2025

adshmh mentioned this pull request Aug 15, 2025

[QoS][Solana] Disqualify endpoints with recent non-JSONRPC response #406

Merged

15 tasks

		@@ -0,0 +1,3 @@
		package evm

		// TODO_IN_THIS_PR: use the framework's EndpointQualityCheckContext for adding QoS checks.

	// TODO_FUTURE(@adshmh): Rank qualified endpoints, e.g. based on latency, for selection.
	// TODO_FUTURE(@adshmh,@commoddity): Rank qualified endpoints, e.g. based on latency, for selection.

	// Endpoint will be disqualified if any filter reutrns an error.
	// Endpoint will be disqualified if any filter returns an error.

	// requestQoSContext holds the context for a request through its lifecycl.
	// requestQoSContext holds the context for a request through its lifecycle.


		}

		func (s *ServiceState) GetConsensusParam(paramName string) (map[string]int, bool) {

[WIP] JUDGE QoS framework v0 #253

Are you sure you want to change the base?

[WIP] JUDGE QoS framework v0 #253

Uh oh!

Conversation

adshmh commented May 9, 2025

Summary

Primary Changes:

Secondary Changes:

Issue

Type of change

QoS Checklist

E2E Validation & Tests

Observability

Sanity Checklist

Uh oh!

adshmh commented May 9, 2025

Uh oh!

commoddity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

commoddity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!