refactor(proto): replace composite gRPC byte payloads with structured messages - #2471
refactor(proto): replace composite gRPC byte payloads with structured messages#2471kkovaacs wants to merge 8 commits into
Conversation
Add canonical protobuf wrappers for Miden field elements and words. Implement owned and borrowed domain conversions with strict encoded-length and canonical-value validation, plus focused round-trip and malformed-input tests.
Replace serialized note attachment payloads with validated protobuf messages backed by canonical Word wrappers. Update note and RPC conversions, reserve the removed wire fields, add attachment boundary and consistency tests, and lower the GetNotesById limit to keep worst-case responses under 4 MiB.
Assert the public descriptor exposes structured note messages and reserves legacy fields. Document the breaking client regeneration requirement and record the completed migration in grpc.md.
Replace opaque BlockBody, SignedBlock, and BlockProof gRPC payloads with validated protobuf structures. Add shared account patch, output note, and transaction header conversions, migrate all in-repository consumers, retain serialization at persistence boundaries, and add descriptor and round-trip coverage.
Replace opaque serialized transaction and batch payloads with fine-grained protobuf messages across RPC, sequencer, validator, and remote prover APIs. Add structured execution-proof and partial-blockchain envelopes, strict domain conversions and validation, and update clients, services, tests, and migration documentation.
Replace the structured execution-proof envelope in transaction and batch messages with canonical Miden-serialized byte fields. Restore the remote prover's generic proof-type and byte-payload API across its server and clients, remove the VM proof schema and conversions, and update tests and migration documentation.
| // A field element encoded by miden_protocol::utils::serde::Serializable. | ||
| message Felt { | ||
| // Exactly eight bytes containing a canonical field element. | ||
| bytes encoded = 1; | ||
| } | ||
|
|
||
| // WORD | ||
| // ================================================================================================ | ||
|
|
||
| // A word encoded by miden_protocol::utils::serde::Serializable. | ||
| message Word { | ||
| // Exactly 32 bytes containing four canonical field elements. | ||
| bytes encoded = 1; | ||
| } |
There was a problem hiding this comment.
Did you consider a more precise integer/fields approach?
| // A field element encoded by miden_protocol::utils::serde::Serializable. | |
| message Felt { | |
| // Exactly eight bytes containing a canonical field element. | |
| bytes encoded = 1; | |
| } | |
| // WORD | |
| // ================================================================================================ | |
| // A word encoded by miden_protocol::utils::serde::Serializable. | |
| message Word { | |
| // Exactly 32 bytes containing four canonical field elements. | |
| bytes encoded = 1; | |
| } | |
| message Felt { | |
| uint64 a = 1; | |
| uint64 b = 2; | |
| uint64 c = 3; | |
| uint64 d = 4; | |
| } | |
| // WORD | |
| // ================================================================================================ | |
| message Word { | |
| Felt a = 1; | |
| Felt b = 2; | |
| Felt c = 3; | |
| Felt d = 4; | |
| } |
| impl BatchAccountUpdateProjection { | ||
| fn into_domain(self) -> Result<BatchAccountUpdate, ConversionError> { | ||
| if self.details.get_size_hint() > ACCOUNT_UPDATE_MAX_SIZE as usize { | ||
| return Err(ConversionError::message("account update exceeds the size limit")); | ||
| } | ||
|
|
||
| match (&self.details, self.account_id.is_private()) { | ||
| (AccountUpdateDetails::Private, true) => {}, | ||
| (AccountUpdateDetails::Public(_), true) => { | ||
| return Err(ConversionError::message( | ||
| "private account update must not reveal public details", | ||
| )); | ||
| }, | ||
| (AccountUpdateDetails::Private, false) => { | ||
| return Err(ConversionError::message( | ||
| "public account update must include public details", | ||
| )); | ||
| }, | ||
| (AccountUpdateDetails::Public(patch), false) => { | ||
| if patch.id() != self.account_id { | ||
| return Err(ConversionError::message( | ||
| "public account patch ID does not match account ID", | ||
| )); | ||
| } | ||
| if self.initial_state_commitment.is_empty() { | ||
| let account = Account::try_from(patch).map_err(ConversionError::new)?; | ||
| if account.to_commitment() != self.final_state_commitment { | ||
| return Err(ConversionError::message( | ||
| "new public account commitment does not match its full-state patch", | ||
| )); | ||
| } | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| Ok(BatchAccountUpdate::new_unchecked( | ||
| self.account_id, | ||
| self.initial_state_commitment, | ||
| self.final_state_commitment, | ||
| self.details, | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
@PhilippGackstatter FYI: miden-protocol does not expose a validated parts constructor for BatchAccountUpdate. As a workaround we're enabling protocol’s testing feature and calling BatchAccountUpdate::new_unchecked after locally reproducing its validation rules.
The proper fix is an upstream validated constructor so that this hack can be removed.
There was a problem hiding this comment.
Do you need this in 0.16 or is 0.17 sufficient?
There was a problem hiding this comment.
Since the code already exists on our end here, I guess either is fine.
We should perhaps just call out that this is temporary so it doesn't pollute the PR review.
Summary
This PR addresses GitHub issue #1882 by replacing opaque, Miden-serialized byte payloads in the node’s gRPC APIs with fine-grained Protobuf messages for notes, blocks, transactions, batches, sequencer requests, and validator block proposals.
Breaking changes
This is a big-bang wire-format migration. Clients must regenerate bindings from the Protobuf definitions shipped with this node release; compatibility with the former composite bytes fields is intentionally not retained.
Tradeoffs
An important implementation compromise is the
BatchAccountUpdateconstructor workaround.BatchAccountUpdate::new_uncheckedafter locally reproducing its validation rules.Other notable tradeoffs:
proto/proto/types/transaction.proto:87; MAST forests, keys, signatures, ciphertext, primitives, and fullAccountDetails.detailsalso remain opaque. The remote-prover API was deliberately left unchanged.crates/proto/src/domain/batch.rs:211. This preserves the existing internal trust boundary but means Protobuf conversion alone is not proof verification.crates/block-producer/src/block_builder/mod.rs:267.proto/proto/types/blockchain.proto:108. It distinguishes “proof present” from “absent,” but will require schema evolution when block proofs gain content.bin/validator/src/server/validator_service/tests.rs:142.Changelog