Reject trailing bytes in BFT transmission payloads

[eranrund]

Motivation

BFT transmission validation previously accepted raw transaction and solution buffers that contained a valid serialized object followed by trailing bytes. The checksum was computed over the full raw buffer, but the payload was later canonicalized to Data::Object, stripping the trailing bytes and causing future checksum computations to differ for the same TransmissionID.

This PR adds strict deserialization for BFT transaction and solution transmissions so raw buffers must contain exactly one serialized object. Padded buffers are rejected before canonicalization.

Test Plan

Unit tests were added for the new strict deserialization helpers. Aside from that we will be relying on the existing tests.

Documentation

N/A

Backwards compatibility

From what I can tell this is not expected to cause any backward compatibility issues.

[vicsn]

Thx!

I think this may not be backwards compatible.

When e.g. only 50% of the network upgraded to this version, a transaction in a proposal may not be deserialized anymore, making it impossible for 66% of proposals to be certified. This would have to be guarded by the most recent ConsensusVersion.

Doing ConsensusVersion guarded checks during block creation is easy, doing it in snarkOS is extra tricky because different nodes may be at different block heights while they're proposing asynchonrously for different consensus rounds.

So what can be done is:

1. In fn propose_batch we stop including these at CONSENSUS_HEIGHT(ConsensusVersion::whatever)
2. In 'fn process_batch_proposewe stop signing atCONSENSUS_HEIGHT(ConsensusVersion::whatever)+50`, because we can be sure all nodes reached that height by then

There is some prior art for this in the code history but we also pruned some of this ductapy code over time. I'd also welcome an abstracted function which handles activating logic in both paths at the two different block heights.

We have an upgrade-test in merge-workflow which may help to simulate this, but it's not practical to simulate every possible edge case transaction - CI would take forever.

--
The same problem as above may occur in the future if we ever change the max transaction size. One way to make that more robust, is to not pick the latest max transaction size, but:

• in the fn propose_batch flow use the one at ConsensusVersion(current_height)
• in the fn process_batch_propose flow allow the max size from max(max_transaction_sizes[(ConsensusVersion(current_height-100))..]

--
At this point this whole enhancement seems incredibly complex, so please do share your 2 cents on if it's worth it from a maintainability perspective. Maybe the attack vector is not worth closing at this point or there is a simpler approach which solves most of the issues.

[eranrund]

Thank you for the detailed response @vicsn!

I want to separate two concerns: max transaction size, and trailing bytes / non-canonical transmission payloads.

Max transaction size (the N::LATEST_MAX_TRANSACTION_SIZE() check)

I agree that if this value changes, then nodes running an old version could refuse transactions from newer upgraded nodes, or vice versa, depending on whether the value increased or decreased.

For example, if the value increases, old nodes would reject transactions that exceed their size limit but are accepted by upgraded nodes. Unless I am missing something, this behavior is not changed by this PR.

I am also not sure there is a way to fully address this for non-upgraded nodes, since they can only apply the logic they already know about. Either way, I think this is tangential to the checksum / trailing-bytes concern, and I think its a separate hardening discussion. I'm happy to dive deeper into it.

Rejecting upon discovering trailing bytes

I am not convinced it will be backwards-incompatible (but certainly open to find out that it is!). Here's my thinking on it:

Normal unconfirmed transaction / solution ingress (e.g. client REST broadcasts) are deserialized before reaching BFT. Consensus sends typed objects to the primary as Data::Object, so original trailing bytes are not preserved for local BFT inclusion (solutions, transactions).

The relevant BFT binary Transmission data path I traced is missing-transmission fetch. A peer proposal can reference a transmission the node does not have, causing the node to sync from the batch header, identify missing transmissions, and fetch them from the peer.

That fetch goes through worker.get_or_fetch_transmission. On a cache miss, the worker waits for a TransmissionResponse, and finish_transmission_request calls ensure_transmission_is_well_formed before completing the pending fetch (response validation).

On an old node (running code before this PR), that first validation call can accept a padded Data::Buffer and mutate it into Data::Object (transaction mutation, solution mutation). After that mutation, the checksum no longer matches the checksum in the TransmissionID, because the trailing bytes are not preserved by canonical object serialization.

However, process_batch_propose_from_peer re-runs ensure_transmission_is_well_formed on the returned missing_transmissions before check_batch_header and before signing (second validation and return, signing happens later). That second validation is what appears to prevent an old honest validator from signing a fresh batch proposal containing such a fetched transmission today.

The changes in this PR should result in rejecting earlier rather than changing an old honest validator from "would sign" to "would not sign". The change also removes reliance on the caller performing that second validation pass, which seems like useful defense in depth.

A broader invariant

While thinking about this, I also had a thought that the broader invariant is canonical binary round-tripping:

Transaction::read_le(bytes)?.to_bytes_le()? == bytes

and similarly for solutions. Trailing bytes are one way to violate that invariant, but not the only possible way. Checking the full canonical round-trip invariant would require reserializing, which may have a measurable cost, especially for large transactions.

I haven't traced through all the binary serialization and deserialization paths so I am not sure if it is possible to have something such as, for example, deserializing a bool where any non-zero byte is stored as true in the struct, and then when serializing back we write 1. If you think this might be a concern I am happy to spend some time reviewing this, it shouldn't be too difficult to look at all the various read_ methods with the help of AI.