feat(rpc): network verification via commitment header by Mirko-von-Leipzig · Pull Request #1084 · 0xMiden/node

Mirko-von-Leipzig · 2025-07-14T10:36:24Z

This PR adds support for verifying the network identity for RPC clients.

This PR also changes the existing format for the version checking:

# Original
application/vnd.miden+grpc.x.y.z

# New
application/vnd.miden; version=x.y.z; genesis=0xABC

The now obsolete format gets rejected now which I think is okay. The new format still allows for an optional +grpc suffix.

The original version checking behavior is retained, though we may want to consider switching to Caret instead of Exact but that's separate from this addition.

This PR also improves the correctness of the accept header verification by:

accepting wildcards */* and application/* media-types
rejecting zero-weighted media-types q=0

Similar to the RPC version check added in #844, we now check the network the client thinks its on. The client can now set the genesis block's commitment in the miden-network header, and the RPC service will reject the request if there is a mismatch.

~~This does add an additional header, and maybe instead we should add it to the accept header? I'm just unsure what this format should look like; and we probably want to do additional parsing instead.~~

Closes #1066.

sergerad · 2025-07-14T23:24:23Z

-        };
-        assert_eq!(AcceptHeaderValue::try_from(input), Ok(expected));
+        let uut = HeaderVerificationLayer::for_tests();
+        uut.verify(&headers).unwrap_err();


Could we test the exact error variants coming out? Given you have created the error enums now.

We can; but I'm not sure that adds any specific value?

For example, in the case where both mismatch, which error should it be? Could be either..

For example, in the case where both mismatch, which error should it be? Could be either..

It would be well defined which error would be returned if both mismatched. Checking the error proves our logic is working precisely as expected. We could erroneously return invalid semver variant for an input with a correct semver for example. Unless I'm missing something.

I had a crack at adding the expected error variants into the cases but I think its blocked on variants like

VersionParsing(#[source] semver::Error, String),

Because semver::Error and its constructors etc are not public so we can't recreate them for the tests.

bobbinth

Thank you! Looks good! Not a review but I did leave a couple of questions/comments inline.

This does add an additional header, and maybe instead we should add it to the accept header? I'm just unsure what this format should look like; and we probably want to do additional parsing instead.

I haven't thought about it too much, but it does feel like maybe handling everything through the accept header is a bit more "natural" - i.e., we have a single header which tells us if the request should be accepted or rejected.

On the other hand, this probably complicates parsing a bit and makes it more difficult to handle partial scenarios (e.g., missing version info but present genesis commitment or vise-versa). But it is also not clear if we should care about partial scenarios - and in that case, maybe channeling everything through the accept header is a better option.

Mirko-von-Leipzig · 2025-07-18T16:58:00Z

I'm most of the way there, but I'm converting to draft until its done properly to spare you looking at it for no reason.

As a short summary of my intentions:

Client can now specify any version(s) using the semver::VersionReq format. This is the same as Rust's Cargo.toml rules. I think this is a great idea because
- Clients can fully specify without the server ever having to recompile e.g. if we accidentally do a semver faux pax the client's can just adjust there headers to be more specific.
- Once/if we ever support multiple RPC versions this will be naturally supported already. We just need to adjust the logic so the server does more proper content type selection.
Genesis support is added.

Mirko-von-Leipzig · 2025-07-23T13:40:37Z

I've refactored this to use an external crate and revert the version requirements to what they were before. I've also updated the description to match the current reality.

If you could re-review please :)

Mirko-von-Leipzig · 2025-07-23T14:00:45Z

+        let supported_versions = VersionReq {
+            comparators: vec![Comparator {
+                op: semver::Op::Exact,
+                major: rpc_version.major,
+                minor: rpc_version.minor.into(),
+                patch: None,
+                pre: semver::Prerelease::default(),
+            }],
+        };


I'm a bit on the fence about this; but I've left the current behavior as is.

The issue is that its a bit back-to-front. The client should be indicating which versions they're compatible with. For example, their RPC version might be patch compatible with our server's but they may require a new feature or addition that we introduced.

Its easy to do by swapping the client and server types around. The downside is that client's need to think a bit more about what they want.

I think this is fine for now, but I would create an issue to revisit this in the future because in principle I agree: the client should be specifying the version that it is compatible with and if the node is within this specifications, the request should be accepted.

bobbinth

Looks good! Thank you! I left a couple of small comments inline.

bobbinth

All looks good! Thank you!

Mirko-von-Leipzig added 3 commits July 11, 2025 17:58

Add genesis commitment check

0d8b075

Add rstest

170facc

Insert genesis header into verifier

96e059f

Mirko-von-Leipzig requested a review from sergerad July 14, 2025 10:36

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch 3 times, most recently from 37be2c3 to 55a9e6a Compare July 14, 2025 11:07

neaten up tests

61fb917

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from 55a9e6a to 61fb917 Compare July 14, 2025 11:11

Move test to integration

b1324ec

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from ce9ba13 to b1324ec Compare July 14, 2025 11:54

sergerad reviewed Jul 14, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs

sergerad reviewed Jul 14, 2025

View reviewed changes

Comment thread crates/rpc/src/server/api.rs

sergerad reviewed Jul 14, 2025

View reviewed changes

Comment thread crates/rpc/src/server/api.rs

sergerad reviewed Jul 14, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs Outdated

bobbinth reviewed Jul 15, 2025

View reviewed changes

Comment thread crates/rpc/src/server/mod.rs

Comment thread crates/rpc/src/server/mod.rs

Mirko-von-Leipzig mentioned this pull request Jul 15, 2025

Include genesis block's commitment in Status #1088

Closed

tomyrd mentioned this pull request Jul 16, 2025

feat: provide genesis commitment via header 0xMiden/miden-client#1045

Merged

tomyrd reviewed Jul 17, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs

tomyrd mentioned this pull request Jul 17, 2025

Improve rpc error messages for the web client 0xMiden/miden-client#1049

Open

Mirko-von-Leipzig added 2 commits July 18, 2025 14:08

only use accept header

c8c8fc5

Update tests and fix parsing

7d2abbd

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch 2 times, most recently from 7325d95 to 1a25c73 Compare July 18, 2025 13:36

Mirko-von-Leipzig marked this pull request as draft July 18, 2025 16:51

merge next

42e59ae

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from 1a25c73 to bee1a0f Compare July 21, 2025 13:07

Fix f32 Ordering

37e67fd

Mirko-von-Leipzig added 4 commits July 23, 2025 13:24

Switch to external media type

92fd682

Move header parsing outward

61f240e

Revert version check to server side

801bf90

merge next

c664cc6

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from 86f569a to bbe039e Compare July 23, 2025 13:40

Mirko-von-Leipzig marked this pull request as ready for review July 23, 2025 13:46

Mirko-von-Leipzig requested review from drahnr, igamigo, sergerad and tomyrd July 23, 2025 13:46

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from bbe039e to e654322 Compare July 23, 2025 13:48

Mirko-von-Leipzig commented Jul 23, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs

drahnr reviewed Jul 23, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs

drahnr approved these changes Jul 23, 2025

View reviewed changes

igamigo approved these changes Jul 23, 2025

View reviewed changes

Comment thread crates/rpc/src/server/accept.rs

Comment thread crates/rpc/src/server/accept.rs

Mirko-von-Leipzig added 2 commits July 23, 2025 17:33

lints

5892c9f

Add quality parsing tests

61aca21

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from e654322 to 61aca21 Compare July 23, 2025 16:00

sergerad approved these changes Jul 23, 2025

View reviewed changes

bobbinth approved these changes Jul 24, 2025

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread crates/rpc/src/server/accept.rs

bobbinth mentioned this pull request Jul 24, 2025

chore: align internal clients #1075

Merged

Mirko-von-Leipzig added 2 commits July 24, 2025 10:34

Replace custom parsing with u16::from_str

8c09b69

merge next

cf11dfd

Mirko-von-Leipzig force-pushed the mirko/rpc-verify-network branch from 5157d20 to cf11dfd Compare July 24, 2025 08:44

bobbinth approved these changes Jul 24, 2025

View reviewed changes

resolve merge conflict

325eaad

Mirko-von-Leipzig merged commit 7dcbf64 into next Jul 24, 2025
8 checks passed

Mirko-von-Leipzig deleted the mirko/rpc-verify-network branch July 24, 2025 16:31

Conversation

Mirko-von-Leipzig commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sergerad Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Mirko-von-Leipzig Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

sergerad Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

sergerad Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mirko-von-Leipzig commented Jul 18, 2025

Uh oh!

Mirko-von-Leipzig commented Jul 23, 2025

Uh oh!

Mirko-von-Leipzig Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bobbinth Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mirko-von-Leipzig commented Jul 14, 2025 •

edited

Loading