Fix mux CBOR framing and add property tests

[satran004]

This PR fixes mux-level CBOR framing so inbound mini-protocol payloads are split by scanning CBOR byte boundaries instead of decode/re-encoding through cbor-java.

Key changes:

• Adds CborByteScanner for byte-preserving CBOR item boundary detection.
• Updates MiniProtoStreamingByteToMessageDecoder to emit original payload bytes unchanged.
• Preserves indefinite-length CBOR encodings such as BF FF.
• Removes the small default incomplete-buffer cap so large LocalStateQuery responses are not rejected.
• Rejects unregistered mux protocol ids.
• Improves write failure handling in Agent.
• Adds jqwik property tests for mux segmentation, protocol interleaving, float/simple values, and malformed CBOR close behavior.
• Adds mux CBOR scanning documentation.

Validation:

• ./gradlew :core:test
• focused mux property tests
• preprod sync completed successfully

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[edridudi]

Byte-preserving de-framing is clearly the right fix — re-serialising through cbor-java was never going to be safe for a protocol where the bytes are the contract, and CborByteScanner is a much better primitive than round-tripping DataItems. The property tests are a welcome addition. Comments inline.

One thing I'd like changed before merge

The default constructor now passes UNLIMITED_INCOMPLETE_BUFFER (-1), and validateIncompleteBuffer early-returns on any negative value — so every existing caller silently loses the accumulation ceiling. A peer that opens a registered mini-protocol, sends 0x9F, and then streams filler forever without a break byte will grow ProtocolChannel.bytes until the JVM dies. The malformed-CBOR catch never fires, because the stream is well-formed — it's simply never finished. Same with a declared ~2GB byte string that stalls.

Since yaci dials out to public relays, that's a remotely-triggerable OOM in the default config, and it's a regression relative to the old fixed cap.

I read the warning in docs/mux-cbor-byte-scanning.md ("do not add a small global cap without validating LocalStateQuery…") and I agree with it — but it argues against a small cap, not against any cap. LSQ ledger-state responses and Leios EBs are large; they aren't unbounded. A generous, configurable finite default (a few MB, or sized to the largest legitimate message) preserves the fix and keeps the DoS door shut. Unlimited is fine as an explicit opt-out for trusted-peer deployments.

Worth addressing

• emitCompleteMessages' array-headed grouping heuristic replaces the old maxCollateralInputs hack and is the subtlest new logic here — and the property generator never produces a message that exercises the merge branch. Would like the invariant stated in the doc and covered by a test.
• Poison-and-close on an unregistered protocol id is a behaviour change from the old lenient auto-create. I think it's correct, but it's undocumented and silent to the application.
• ProtocolChannel.append is O(n²) in message size (full BytesUtil.merge copy per 64KB segment; ~2.1GB copied for one 16MB message). Pre-existing, but this PR removes the cap and is motivated by large Leios blocks, so it stops being theoretical.
• discardBytes silently drops frames straddling consumedLength, safe only because callers always pass a frame boundary. Undocumented invariant.

Not reviewed

I did not review Agent.java (~+85/-17) and I don't think anyone else has either — left a note there with the specific paths I'd want checked (partial segmented write, duplicate failure callback, locking on the Channel).

Cross-PR

Worth noting on #167: ADR 0008's BF FF → BF finding means the Leios fetch path is corruptible on wire-legal input until this lands. #167 currently works around it by disabling empty-bitmap requests. I'd sequence this one first and then drop that workaround, rather than shipping it.

Also, the PR body mentions "preprod sync completed successfully" — was that run with the new poison-on-unregistered-protocol semantics against a node speaking protocols we don't register? That's the case I'd most want on the record before this goes in.

[edridudi]

The default constructor now accumulates without bound — remote heap exhaustion from an untrusted peer.

This is the one thing I'd want changed before merge.

The back-compat constructor passes UNLIMITED_INCOMPLETE_BUFFER (-1), and validateIncompleteBuffer (L131-133) early-returns whenever maxIncompleteBufferSize < 0. So for every existing caller — NodeClient, TCPNodeClient, everything that constructs the decoder with just agents — there is now no ceiling at all on ProtocolChannel.bytes.

Concretely, against a malicious or simply broken relay:

• Open a registered mini-protocol (BlockFetch id 3, or Leios 18/19).
• Send a segment whose payload is 0x9F (indefinite-length array start), then stream 65535-byte filler segments forever and never send the 0xFF break.
• emitCompleteMessages finds no complete frame, so consumedLength == 0, so discardBytes never runs. append() keeps merging. Heap grows until OOM.

The RuntimeException | StackOverflowError catch at L81 does not save us here: the stream is well-formed CBOR, just never terminated. Nothing throws. Same result with a definite-length byte string declaring ~2GB (0x5A 0x7F FF FF FF) and then stalling — CborByteScanner reports IncompleteCborException forever while the buffer grows.

Since yaci dials out to public relays, that's a remotely-triggerable OOM in the default configuration.

I read docs/mux-cbor-byte-scanning.md and I think the warning there is being over-applied:

Do not add a small global cap here without validating LocalStateQuery and other large payloads.

That argues against a small cap, not against any cap. LocalStateQuery ledger-state responses and Leios EBs are large, but they are not unbounded — they have a finite worst case. Suggest shipping a finite, generous default (a few MB, or sized to the largest legitimate message, configurable) so validateIncompleteBuffer poisons and closes the channel on runaway accumulation. Unlimited should be the explicit opt-out, not the default that every existing caller silently inherits.

Cheap hardening on top: validate the declared top-level CBOR length against that ceiling as soon as the header bytes arrive, rather than waiting to accumulate to it.

[edridudi]

O(n²) accumulation — matters more after this PR, because Leios raises message sizes.

BytesUtil.merge allocates a new array and copies the entire accumulated buffer on every 64KB segment. For a message arriving as k segments, total copying is 64KB * (1+2+…+k).

A 16MB payload (large EB, or a ledger-state query result) = ~256 segments ≈ 2.1GB copied for a single logical message, plus the GC churn. 64MB payload is ~16× worse. scanCompleteFrames() then re-scans from scanOffset over the whole accumulated buffer on each append, compounding it.

This is pre-existing, but this PR is the one that removes the accumulation cap and is motivated by large Leios blocks, so the quadratic term stops being theoretical.

Suggest an amortized-growth accumulator — ByteArrayOutputStream, a doubling byte[], or a Netty CompositeByteBuf — and materialising a contiguous array only when a frame is actually emitted.

[edridudi]

Two things here:

1. Undocumented invariant. discardBytes drops any cached frame that straddles consumedLength (startOffset < consumedLength < endOffset). That is only safe because emitCompleteMessages always passes a messageEnd, i.e. a frame boundary. Nothing asserts or documents it, and the frame-grouping loop in the decoder is exactly the kind of code that could later produce a non-boundary offset. Silent message loss if it ever does. Worth an explicit assert/precondition, or at minimum a comment naming the invariant.

2. Copy per emit. When one read delivers N small back-to-back messages, each emit copies the entire remaining tail into a fresh array. A read/start offset advanced in place, compacting only when the offset grows large, avoids the repeated copy.

[edridudi]

This is a behaviour change worth calling out explicitly in the PR description and in docs/mux-cbor-byte-scanning.md.

The old decoder leniently auto-created a ProtocolChannel for any protocol id it hadn't seen. This one poisons the decoder and tears down the whole TCP connection on the first segment for an unregistered id — and the same at L81-87 for any malformed CBOR.

I think stricter is right. But it means a peer that previously worked (speaking a mini-protocol we don't register, e.g. because we attach a subset of agents) now kills the connection rather than being ignored. That's a real compatibility surface for anyone embedding yaci with a reduced agent set, and it's silent — poisoned then drains bytes with no further signal to the application.

At minimum: document it, and consider dispatching a disconnect/error to listeners rather than just log.warn + ctx.close(), so the app can distinguish "peer spoke something unexpected" from an ordinary disconnect.

[edridudi]

Catching StackOverflowError and logging it at WARN alongside ordinary malformed-CBOR means a genuine unbounded-recursion regression in CborByteScanner degrades into "one more noisy peer" in production logs rather than a loud failure.

The depth guard in the scanner should be the real defence. If the catch stays as a backstop, log the SOE at ERROR with a distinct message so it's separable in monitoring from routine malformed input.

Also: the outer catch (Exception e) { log.error("Decoding error", e); } at the bottom swallows and continues without poisoning or closing — worth a second look at whether that path can now leave a channel in a half-consumed state.

[edridudi]

The message-grouping heuristic here — "a message is an array-headed frame plus any following non-array frames" — is the replacement for the old LocalStateQuery maxCollateralInputs special-case, and it's the part of this PR I'd most like a second pair of eyes on.

breaking out when messageEnd < bytes.length && !startsWithMessage(bytes, messageEnd) means a complete, emittable message is withheld whenever the next byte isn't an array header — including when it's a trailing non-array item that belongs to the next message rather than this one. If that judgement is ever wrong the mini-protocol stalls with data sitting in the buffer, which is a hard failure mode to debug in the field.

Is there a spec-level statement that mini-protocol messages are always array-headed and that trailing non-array top-level items always belong to the preceding message? If so it's worth citing it in docs/mux-cbor-byte-scanning.md right here. The property test's randomMuxMessage only ever generates array-headed messages, so the grouping branch isn't exercised (see my comment on the test).

[edridudi]

The jqwik properties are a good addition. Three coverage gaps I'd close, roughly in value order:

1. Multi-frame grouping is never generated. randomMuxMessage always emits an array (or tagged array), so the while (frameIndex < frames.size() && !isMessageStart(...)) merge loop in emitCompleteMessages — the trickiest new logic in the PR — is never exercised. Add a generator variant that appends non-array top-level items after an array within one logical message and assert the grouping boundary.

2. No large / multi-byte declared lengths. randomByteString/randomTextString use random.nextInt(32), and the array/map generators are similarly small, so every generated item fits a 1-byte length header and usually a single segment. The addLength overflow and IncompleteCborException-vs-bufferLength boundaries — precisely what the scanner rewrite changes — are untested. A definite-length byte string with a 2- or 4-byte header (hundreds to tens of thousands of bytes) pushed through splitStream would cover the accumulate-then-emit-exactly-once path.

3. Reserved additional-info coverage is one major type deep. randomMalformedCbor case 5 only emits 0xfc. If the reserved AI 28-30 branch were mistakenly relaxed for, say, byte strings, nothing here would catch it. Cheap fix — emit reserved-AI initial bytes across major types 0-6 (0x1c 0x3c 0x5c 0x7c 0x9c 0xbc 0xdc) plus 0xfd/0xfe.

Nothing above blocks; (1) is the one that would actually protect the new code.

[edridudi]

Flagging that I have not reviewed this file properly, and I think it needs it — it's the second-largest change in the PR (~+85/-17) and ships a 297-line AgentWriteMessageTest, but the PR description frames the change as "improves write-failure handling", which undersells it.

Three things I'd want someone to look at specifically:

• Partial segmented write. In writeSegmentedMessage, if the loop writes segment k of n and then a write fails, does the peer see a truncated mux message before the channel closes? If so, is that distinguishable from a clean disconnect on their side?
• Double close / duplicate failure callback. Can a write-failure path and the channel-inactive path both fire the failure callback for the same message?
• Synchronising on the Channel object — if that's what's happening, it's a lock we don't own and Netty may take internally.

Happy to be told these are all handled; I just don't want it to ride through on the strength of the CBOR review.