Skip to content

fix(consensus): match charon map-entry encoding in hash_proto - #580

Open
varex83agent wants to merge 2 commits into
mainfrom
fix/qbft-map-entry-hash
Open

fix(consensus): match charon map-entry encoding in hash_proto#580
varex83agent wants to merge 2 commits into
mainfrom
fix/qbft-map-entry-hash

Conversation

@varex83agent

Copy link
Copy Markdown
Collaborator

Problem

hash_proto uses prost to encode a message before SSZ-hashing it. prost omits a map entry's key or value field when it equals the protobuf default (empty string / empty bytes), whereas charon's Go marshaler (proto.MarshalOptions{Deterministic: true}) always emits both fields.

As a result, an UnsignedDataSet (map<string, bytes> set = 1) carrying an entry with an empty key or empty value encodes — and therefore hashes — differently in pluto than in charon. In a mixed charon/pluto cluster, the two implementations would derive different value hashes from identical proposed bytes, so QBFT could not reach agreement on that value.

Confirmed by differential execution against charon's hashProto (core/consensus/qbft/msg.go): the empty-key, empty-value, and both-empty cases each produce distinct bytes, while non-empty entries are byte-identical.

Reachability

Empty keys/values never occur in normal operation (keys are 0x-prefixed validator pubkeys, values are serialized duty data). The path is reachable via adversarial or malformed wire input: inbound Any values are decoded and re-hashed in values_by_hash, and prost's decoder accepts the explicit-empty forms. Impact is bounded to interop/consensus-liveness in a mixed cluster; a pluto-only cluster is self-consistent. Not a memory-safety or key-compromise issue.

Fix

UnsignedDataSet is the only map-bearing message hashed in consensus (PriorityResult uses repeated fields; ParSignedDataSet is not hashed). hash_proto now re-encodes it with charon-compatible map-entry bytes — always emitting both key and value fields, key-sorted via BTreeMap to match Go's deterministic map ordering — before hashing. All existing call sites (propose and verify) funnel through hash_proto, so the single chokepoint covers every path.

Tests

  • Byte-exact vectors reproduced from charon's deterministic marshal pin the three empty-field cases.
  • A parity test confirms the charon encoder reproduces prost's bytes exactly for non-empty entries (so the existing golden vectors are unaffected).
  • A hash-level test confirms hash_proto hashes the charon bytes, not prost's, for an empty-field entry.

cargo test -p pluto-consensus (211 tests), clippy, and fmt all pass. The pre-existing charon v1.7.1 golden-vector test still passes.

🤖 Generated with Claude Code

varex83agent and others added 2 commits August 3, 2026 13:06
prost skips a map entry's key or value field when it equals the protobuf
default (empty string / empty bytes), but charon's Go marshaler always
emits both. An `UnsignedDataSet` carrying an entry with an empty key or
empty value therefore encodes — and hashes — differently in pluto than in
charon, so a mixed cluster cannot agree on that value's hash.

Such entries never arise in normal operation (keys are validator pubkeys,
values are serialized duty data) but are reachable via adversarial or
malformed wire input, since inbound `Any` values are decoded and re-hashed
in `values_by_hash`.

`UnsignedDataSet` is the only map-bearing message hashed in consensus, so
`hash_proto` now re-encodes it with charon-compatible map-entry bytes
(always emitting both fields, key-sorted via `BTreeMap`) before hashing.
Byte-exact vectors reproduced from charon's deterministic marshal pin the
three empty-field cases and confirm parity for non-empty entries.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
emlautarom1

This comment was marked as outdated.

@emlautarom1 emlautarom1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This solution is partially complete:

let hash = msg::hash_proto(&value)?;

propose computes the hash via hash_proto (now charon encoding) and separately packs Any::from_msg(&value) (prost encoding) onto value_tx

let hash = msg::hash_proto_bytes(&local.value)?;

get_value drains that Any and indexes it under hash_proto_bytes(&local.value), i.e. the raw prost bytes. No canonicalization.

if M::PACKAGE == pbcore::UnsignedDataSet::PACKAGE && M::NAME == pbcore::UnsignedDataSet::NAME {
let decoded =
pbcore::UnsignedDataSet::decode(encoded.as_slice()).map_err(Error::UnmarshalProto)?;
encoded.clear();
encode_unsigned_data_set(&decoded.set, &mut encoded);
}

We previously encode, then for this specific edge case decode, and encode again. It seems like we have some unnecessary work performed.


Note that the solution proposed is very case-oriented: the root problem is that prost skips map-like entries where the value is equal to the type's default. This is very specific to prost, and it's not a bug since Protobuf is underspecified in this regard. There have been reports on the prost repo about this:

For the time being, I think we can move forward with this PR as long as we HEAVILY annotate this as a hack. A proper solution requires:

  • Evaluating other library that uses the same serialization mechanism (following the C++ reference impl), thus ensuring that hashing is consistent.
  • Do a general workaround for all types.

For the later, we could do the following:

prost-build adds #[prost(prost_path = "…")] on every generated type, and prost-derive routes all encoding calls through that path, including the map-like ones.

I propose to add a a shim module that re-exports prost but shadows encoding::btree_map / encoding::hash_map, using "standard" encodings onto every generated message

pub mod prost {
    pub use ::prost::*;                 // Message, Name, alloc, bytes, …
    pub mod encoding {
        pub use ::prost::encoding::*;   // shadowed by the modules below
        pub mod btree_map { /* encode/encoded_len{,_with_default} that always emit both fields; merge re-exported from prost */ }
        pub mod hash_map  { /* idem */ }
    }
}

Then, set prost_build::Config::prost_path("::pluto_proto::prost") in compile_protos so that the shim module is used. This avoid forking, using alternative libraries, or per-type workarounds as this PR introduces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants