Redundant CBOR encoding of messages during block validation
- Dominant language
- Rust
- Stars
- 697
- Forks
- 200
- Avg merge
- 1d 5h
- Merged PRs (30d)
- 65
Description
> [!NOTE]
> This was AI found and seems correct, but do your own checks. Regardless of that, a test will be useful.
## Summary
Forest re-encodes every message in a block to CBOR at least three times while validating it, and
throws each encoding away. The root cause is that `Cid::from_cbor_blake2b256` materialises the whole
encoding into a `Vec` purely to feed a hasher, and neither `Message` nor `SignedMessage` caches
its CID.
No consensus risk in fixing this: the changes below alter only *how* the bytes are produced, never
what they are.
## Where the encodings happen
`src/utils/cid/mod.rs`, the root of the problem:
```rust
fn from_cbor_blake2b256(obj: &S) -> Result {
let bytes = fvm_ipld_encoding::to_vec(obj)?; // full Vec allocation
Ok(Self::from_cbor_encoded_raw_bytes_blake2b256(&bytes)) // hash it, drop it
}
```
`Message::cid()` (`src/shim/message.rs:188`) and `SignedMessage::cid()`
(`src/message/signed_message.rs:83`) both route through it. Neither memoises.
Per **BLS** message, inside one `check_block_messages` call
(`src/chain_sync/tipset_syncer.rs`):
| # | Site | Fate of the bytes |
| - | ---- | ----------------- |
| 1 | `tipset_syncer.rs:397` — `m.cid().to_bytes()`, for the BLS aggregate check | hashed, dropped |
| 2 | `tipset_syncer.rs:424` — `to_vec(msg)?.len()`, for `price_list.on_chain_message` | length read, dropped |
| 3 | `tipset_syncer.rs:511` → `TipsetValidator::compute_msg_root` → `Cid::from_cbor_blake2b256` | hashed, dropped |
Per **SECP** message, three as well, in different shapes:
| # | Site | What is encoded |
| - | ---- | --------------- |
| 1 | `tipset_syncer.rs:493` — `check_msg(msg.message(), ..)` → `to_vec` | the inner unsigned `Message` |
| 2 | `src/shim/crypto.rs:160` — `msg.message().cid().to_bytes()` inside `authenticate_msg` | the inner `Message` again |
| 3 | `tipset_syncer.rs:511` → `compute_msg_root` | the full `SignedMessage` |
Delegated (EIP-155/1559) signatures cost **two more**: `src/shim/crypto.rs:147` compares
`msg.message().cid() == filecoin_msg.cid()`, building both sides from scratch.
### It is worse across a block's lifetime
`compute_msg_root` is reached from three independent places on the same data, with no memoisation
between them:
- `src/chain_sync/validation.rs:101` — gossip block validation
- `src/blocks/tipset.rs:613` — `FullTipset::persist`
- `src/chain_sync/tipset_syncer.rs:511` — `check_block_messages`
A block that arrives over gossip, gets persisted, then gets fully validated re-encodes every message
it contains three additional times.
## Proposed work
### 1. Stream into the hasher instead of buffering (highest value/effort ratio)
Change `Cid::from_cbor_blake2b256` in `src/utils/cid/mod.rs` to serialise directly into the Blake2b
digest via `fvm_ipld_encoding::to_writer` and a small `io::Write` adapter around the hasher.
- Removes one heap allocation per call, sized to the encoded object.
- No API change, no call-site churn.
- Blast radius is much wider than block validation: 17 non-test call sites, plus every
`Message::cid()` / `SignedMessage::cid()` in the mempool, chain store, and eth-mapping paths.
Keep `from_cbor_encoded_raw_bytes_blake2b256` as-is — it has callers that already hold the bytes.
### 2. Count bytes without allocating
`to_vec(x)?.len()` appears at:
- `src/chain_sync/tipset_syncer.rs:424`
- `src/interpreter/vm.rs:433` and `:463` (these run per message per epoch for **every** FVM
version, not just the pre-nv18 path — the most valuable of the set)
- `src/message_pool/msgpool/msg_pool.rs:587`
- `src/libp2p/chain_exchange/provider.rs:348`
- `src/message/signed_message.rs:88` (`SignedMessage::chain_length`)
A counting `io::Write` sink passed to `fvm_ipld_encoding::to_writer` makes all of them
allocation-free. Consider exposing it as `crate::utils::encoding::encoded_len(&impl Serialize)`.
### 3. Cache the CID on the message (separate, larger change)
The deeper fix, and the one that actually removes the 3×. `CachingBlockHeader` already does exactly
this for headers with a `OnceCell`; `Message` has no equivalent.
Bigger blast radius — `Message` is widely used and derives `Clone`/`Hash`/`PartialEq`, so a cache
field needs care (skip it in comparisons and hashing, and in the `serde` impls). Worth doing only
after 1 and 2 are measured. Note the subtlety already documented at
`src/message/signed_message.rs:75-78`: `SignedMessage::cid()` is **not**
`Cid::from_cbor_blake2b256(signed_msg)` for BLS messages — it delegates to the inner message. Any
cache has to preserve that.
## Measuring
`check_block_messages` runs on validated sync, not on snapshot import, so the headline win is live
sync and `forest-tool` validation runs — **not** import throughput. Measure before claiming
otherwise.
Suggested: `forest-tool benchmark` against a calibnet snapshot, plus a targeted criterion bench over
`compute_msg_root` on a realistic block (mainnet blocks carry up to `BLOCK_MESSAGE_LIMIT` = 10000
messages; typical tipsets are in the hundreds to low thousands).
Correctness is easy to pin down: CIDs and roots must be byte-identical before and after. Existing
coverage in `src/chain_sync/validation.rs:372,386` (`compute_msg_root`) and the
`src/message/signed_message.rs` CID tests should catch a regression, and both changes are
mechanical enough that a differential test over arbitrary `Message` values is cheap to add.
## Suggested split
- PR 1: items 1 and 2. Mechanical, independently benchmarkable, no consensus risk.
- PR 2: item 3, only if 1 and 2 leave a measurable gap.
Contributor guide
Assessment
This issue has not been assessed yet.