Skip to content

Extract shared Protobuf conversion helpers; standardize prost Option/default handling #638

Description

@emlautarom1

Summary

proto3 presence semantics as prost surfaces them (every message field is Option, scalars silently default) are handled ad hoc per crate:

  • Duty ↔ pbcore::Duty is implemented three times with three failure semantics: fallible TryFrom in core/src/types.rs#L282-L304; a lossy infallible duty_from_proto in consensus qbft/msg.rs#L370-L382 (NoneDuty::new(0, Unknown)); and another infallible pair in priority/src/prioritiser.rs#L73-L88. Two share the name duty_from_proto with different tolerance.
  • ~11 .ok_or(...) unwrap sites and 15+ Some(...) wrapping sites for message fields; duplicated BTreeMap<String,_> ↔ HashMap<PubKey,_> transcode loops and empty-map-means-invalid checks between core/types.rs and core/unsigneddata.rs.
  • Zero-value conventions are hand-rolled per site: "absent vs zero hash" (msg.rs#L330-L337), "empty bytes means missing signature" (#L315-L317).
  • cluster/build.rs inlines its own prost_build::Config instead of using pluto-build-proto, silently skipping enable_type_names() and the lint-header injection every other crate gets.

Message-field presence is proto3, not prost — any generator carries the same absent-vs-default ambiguity, so the leverage is in centralizing the handling, not switching generators.

One default-value problem is prost-specific and needs a different fix. prost omits a map entry's key or value field when it equals the type default, where protoc and Go always emit both. Reference bytes from protoc 32.1, byte-identical to Go:

value protoc / Go prost
UnsignedDataSet{"0xaa": b""} 0a080a04307861611200 0a060a0430786161
UnsignedDataSet{"": b""} 0a040a001200 0a00
map<string,int32>{"a": 0} 0a050a01611000 0a030a0161

hash_proto SSZ-hashes those bytes into the QBFT consensus value hash, so a mixed pluto/charon cluster derives different hashes for an identical value and fails to agree with nothing erroring. It is settled upstream — prost#99 closed as working-as-intended, the omission being spec-permitted even though no Google implementation does it — so the correction has to live on pluto's side. ParSignedDataSet has the same exposure, latent only because it never reaches hash_proto.

Proposed change

  • One conversion module in core (relates Centralize helper at core #127): a single fallible Duty conversion (callers that want lossy behavior make it explicit), a fn required<T>(field: Option<T>, name: &'static str) -> Result<T, ProtoFieldError> helper replacing the ok_or boilerplate, and the shared map-transcode helpers.
  • Document the zero-value conventions (zero hash = absent, empty bytes = absent) once, next to the helpers.
  • Correct map-entry encoding once for every message rather than per hashing call site, and guard it with a descriptor-driven test: walk the FileDescriptorSet (prost_build emits one via file_descriptor_set_path) and assert every map field in the workspace emits both entry fields for a zero-valued key and value. Without that guard, adding a map to a hashed message is a silent consensus divergence.
  • Route cluster/build.rs through pluto-build-proto.

Metadata

Metadata

Assignees

No one assigned

    Labels

    rustPull requests that update rust code

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions