Refactor codec and framing - #2317
Open
bit-aloo wants to merge 31 commits into
Open
Conversation
bit-aloo
force-pushed
the
2026-08-25-refactor-codec-and-framing
branch
from
August 26, 2026 06:07
1c54c68 to
b5db63e
Compare
bit-aloo
marked this pull request as draft
August 27, 2026 09:52
The manual (len + n - 1) / n needed a clippy allow to stay.
Nothing has constructed it since get_header stopped being fallible, and a header length is a count of bytes, so UnexpectedHeaderLength carries a usize rather than an isize.
payload and serialized were two Options of which exactly one was ever
set, so serialize and encoded_length each carried a panic!("Impossible
state") for the combination the type allowed but the code never built.
One field holding either alternative makes that state unrepresentable.
The accessor panicked for a frame built from a message, which is a state a caller can hold, so the panic was reachable from safe use of the API.
get_header returned an Option that was always Some, a leftover of the Frame trait that used to unify Sv2Frame with HandShakeFrame, and every caller had to unwrap or expect it.
The constructor is public, so its expect("Invalid header") was reachable
with any buffer shorter than a header. Returning the error the header
parser already produces also lets from_bytes parse the header once
instead of once for the size hint and once to build the frame.
An already serialized frame reached swap_with_slice, which panics unless the two slices are the same length, and an unserialized one indexed dst at Header::SIZE before writing. Both are now a DestinationTooShort.
The Noise specification writes it that way, and the crate was using both spellings. Renames HandShakeFrame and the Frame::HandShake variant.
get_payload_when_handshaking allocated a Vec on every call, and named a phase that a HandshakeFrame is never outside of.
Constructors first, then what a frame does, then what it reports about itself. Also documents the two Frame variants.
FramingError and FramingSv2Error both wrapped framing_sv2::Error; From produced the second and the encoder built the first by hand. Also fixes the "initiato" typo in the neighbouring message.
The encoder and decoder each defined Buffer, and only for the pooled case: without the feature it arrived as an import rename, so the type the crate builds on was hard to see. The 512 and 2^16 * 5 literals sat in the constructors.
Their module headers were plain comments in private modules, so none of that documentation reached the rendered docs. As doc comments they also turned out to link buffer_sv2 types through binary_sv2. The AeadBuffer re-export stays crate-internal: a pub use in a private module exposed nothing, and making the module public would have changed that.
StandardSv2Frame and StandardEitherFrame describe a frame that is being encoded just as much as one being decoded, so the decoder was not the place to define them. Frame is only used by the noise path there now, so it moves in with the rest of the noise-gated imports.
State, HandshakeRole and the two transport halves lived in lib.rs, where nobody looking for the handshake would think to find them, and State had two impl blocks for no reason. The move also drops the h2f import alias and puts the crate's own Result alias where core::result::Result<T, Error> was spelled out.
The decoder and encoder structs listed the fields they share in different orders, and the decoders declared Default before the new they call. Also groups the imports each module had split across several lines.
The State enum let a connection be asked for anything at any time, and the codec answered by checking at run time: step_0 on a responder, step_1 on an initiator, splitting a state that had not completed a handshake, decoding a transport frame mid-handshake. Each of those was an error variant, and one of them was an unreachable!(). A connection is now a Handshake<Initiator>, a Handshake<Responder> or a Transport, each carrying only what that phase can do, and the step that completes a handshake consumes it. The encoder and decoder split their noise paths to match, taking the frame type their phase actually uses, which also retires the two try_into conversions that could report the wrong kind of frame at run time. NotInHandshakeState, InvalidStepForInitiator, InvalidStepForResponder and UnexpectedNoiseState describe situations that can no longer be built, so they go.
Three of them: a step that belongs to the other role, a handshake reused after the step that consumed it, and a Transport built without one. Each was checked to fail for its own reason, not an incidental one.
framing_sv2 pulled in noise_sv2, and with it secp256k1, chacha20poly1305, zeroize and rand, to read one constant: AEAD_MAC_LEN. The two items that needed it, Header::encrypted_len and ENCRYPTED_SV2_FRAME_HEADER_SIZE, were used by codec_sv2 alone, which already depends on noise, so both move there. Header::len becomes the public payload_length that the arithmetic needs, and framing_sv2 is left with binary_sv2 and an optional buffer_sv2.
Every consumer that decodes a frame has to turn the None into an error of its own, and until now there was no variant that said what happened.
Nothing has produced or matched one since the codec gave each noise phase its own type: the encoder takes the frame its phase uses and the decoder returns it, so no caller has to ask which kind it got. That leaves the ExpectedSv2Frame and ExpectedHandshakeFrame variants, which only its TryFrom impls reported, and codec_sv2's StandardEitherFrame alias.
One type held either a message waiting to be serialized or the bytes of a frame that had been read, and every accessor paid for the two cases it could not both serve: payload returned an Option that is None only for a frame nobody decoded, and each type parameter was dead weight on one side of the split. Downstream wrote .ok_or(...)? on 23 frames that had just come off the wire. Sv2Frame<T> now carries the message on the way out and SerializedSv2Frame<B> carries the bytes on the way in, so payload is infallible, from_message needs no buffer type and from_bytes needs no message type. The decoders return the serialized frame, the encoders take the message frame, and Error::FrameNotSerialized has nothing left to report.
A decoder hands back the bytes it read, and has done since the frame split, so the T it carried never constrained anything: it was a PhantomData and a Serialize + GetSize + Deserialize bound that suggested the decoder knew what it was decoding. StandardDecoder::<Message>::new() becomes StandardDecoder::new().
An encoder serializes a frame and hands the bytes on; which of the two frame types it was given only decides how those bytes are produced. EncodableFrame says exactly that, so the encoders take either kind, and a caller that sometimes holds a message and sometimes holds bytes it framed itself can implement the trait for its own type instead of converting one into the other. With the frame carrying its own message type, the encoders no longer need one: Encoder::<Message>::new() becomes Encoder::new(), as it did for the decoders.
A frame that is already serialized has nothing to serialize: it now offers as_bytes and into_bytes, and writing it into a buffer is the encoder's job, through EncodableFrame::encode_into.
Nothing in the workspace calls it.
handshake_message_to_frame was a free function next to the constructor it duplicates; it becomes HandshakeFrame::from_message.
StandardSv2Frame pinned no buffer once the frame split, so it named the same type twice.
get_size returns a usize, so on a 64-bit target `as u32` truncated a message past u32::MAX into a length Header::from_len accepts, producing a frame whose header under-declares its payload by ~4 GiB.
A peer declares the payload length in the header it sends before any of that payload, so sizing the buffer from that declaration let six bytes reserve close to 16 MiB, and an encrypted header 16.8 MiB. Read the frame a chunk at a time instead, so the buffer grows with the data that arrives.
bit-aloo
force-pushed
the
2026-08-25-refactor-codec-and-framing
branch
from
August 28, 2026 09:36
4f06c41 to
7ff8e04
Compare
bit-aloo
marked this pull request as ready for review
August 28, 2026 09:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #2276
closes: #2277
closes: https://github.com/project-loupe/audit-stratum/issues/46
closes: #2115
closes: #2280
closes: https://github.com/project-loupe/audit-stratum/issues/60
closes: #2288
closes: https://github.com/project-loupe/audit-stratum/issues/183
closes: https://github.com/project-loupe/audit-stratum/issues/133
closes: https://github.com/project-loupe/audit-stratum/issues/182
closes: https://github.com/project-loupe/audit-stratum/issues/179
closes: #2327
companion stratum-mining/sv2-apps#806