fix: [DSM-148] Advert classification: check for gap-free pooled slices

[alin-at-dfinity]

A pooled slice may not extend gap-free whatever we've already inducted or put into payloads. (This can happen e.g. if the replica built a payload which was then dropped in favor of another, which included fewer messages.) So when deciding whether the contents of the advertised header are already covered by the pooled slice (Pooled), also check for gap-free extension of the expected message index.

Also improve test coverage of existing code, by looking at all combinations of extra message / extra signal / nothing new across all steps.

[zeropath-ai]

✅ No security or compliance issues detected. Reviewed everything up to 0892147.

Security Overview

• 🔎 Scanned files: 2 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=e7b5bf8e-be19-404c-82a9-038e221e924b&codeScanTypes=PrScan&tab=issues&detectionTypes=traditional,businessLogic,customRule,IAC,SECRETS,CICD,EOL,uncategorized

Detected Code Changes

Change Type

Relevant files

Refactor

► rs/xnet/payload_builder/src/certified_slice_pool.rs     - Update imports: add STREAM_INDEX_MAX and use it for expected_message_index     - Initialize and utilize expected_message_index to govern message_end calculation     - Adjust logic to only update messages_end when pooled slice begins before or at expected_message_index ► rs/xnet/payload_builder/tests/certified_slice_pool.rs     - Extend tests with new scenarios ensuring adverts beyond current reference points are actionable - Add extensive test blocks to verify classify_advert behavior with extra messages/signals and accumulation of reference points - Add tests for gap-before-pooled-slice handling and corresponding advert outcomes

[mraszyk]

the slice does not (necessarily) cover the whole stream after garbage collect so the comment sounds imprecise

[mraszyk]

AFAICT a pooled slice implies a recorded stream position: the refill task only polls peers() (i.e. the keys of stream_positions) and garbage_collect() drops the slices of any subnet not in the new map. So this STREAM_INDEX_MAX fallback should be practically unreachable: only by put() before any GC (as in tests), or by a refill racing with a garbage_collect() that dropped the peer.

Maybe say so in the comment; or default conservatively, so that pooled messages never count towards messages_end without a stream position (since we cannot consult the certified state here, we have no way of knowing whether the pooled slice extends it gap-free).

[mraszyk]

Suggested change

	// Conversely, with no message included into a block, but the stream position
	// ahead of the pooled slice in both signals and collected reject signals: only
	// the pooled slice covers the advertised messages.
	let advert = StreamHeaderBuilder::new()
	.begin(messages_begin.increment())
	.end(stream.messages_end())
	.signals_end(stream.signals_end().increment())
	.build();
	let reject_signal_at_begin = |from, to| from <= messages_begin && messages_begin < to;
	let pool = pool_at(ExpectedIndices {
	message_index: messages_begin,
	signal_index: stream.signals_end().increment(),
	max_no_gc_header_begin: messages_begin.increment(),
	});
	// Conversely, with no message included into a block, but the stream position
	// ahead of the pooled slice in signals: only
	// the pooled slice covers the advertised messages.
	let advert = StreamHeaderBuilder::new()
	.begin(messages_begin)
	.end(stream.messages_end())
	.signals_end(stream.signals_end().increment())
	.build();
	let reject_signal_at_begin = |from, to| from <= messages_begin && messages_begin < to;
	let pool = pool_at(ExpectedIndices {
	message_index: messages_begin,
	signal_index: stream.signals_end().increment(),
	max_no_gc_header_begin: messages_begin,
	});

I'd simplify this to only test signals.

[mraszyk]

Otherwise,

            message_index: messages_begin,
            max_no_gc_header_begin: messages_begin.increment(),

is inconsistent: we cannot have a reject signal for a message we did not see.

[mraszyk]

Reject signals (i.e. begin accumulation) could then be covered by a dedicated scenario, with the stream position covering all of the advertised messages and signals, and the (header-only) pooled slice contributing only a begin advanced past a reject signal we still hold, i.e. the one thing that makes it worth inducting (cf. ExpectedIndices::max_no_gc_header_begin):

        // All of the stream's messages and signals were included into blocks, but a
        // reject signal just before `messages_begin` is still to be collected: only
        // the (header-only) pooled slice, beginning past it, covers the advertised
        // `begin`.
        let reject_signal_before_begin =
            |from, to| from <= messages_begin.decrement() && messages_begin.decrement() < to;
        let pool = pool_at(ExpectedIndices {
            message_index: stream.messages_end(),
            signal_index: stream.signals_end(),
            max_no_gc_header_begin: messages_begin.decrement(),
        });
        assert_eq!(
            classify_advert_with(
                &pool,
                SRC_SUBNET,
                &stream.header(),
                &reject_signal_before_begin
            ),
            XNetAdvertOutcome::Pooled
        );

This needs a #[filter(#test_slice.0.messages_begin().get() > 0)], as in pool_classify_advert_collecting_reject_signal(), to leave room for the reject signal before messages_begin.