feat: [DSM-148] CertifiedSlicePool::classify_advert() - #11601
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 3511248. Security OverviewDetected Code Changes
|
Now that #11203 has merged, we can reenable the mainnet NNS recovery system test. Due to compatibility reasons, this test must be turned into a "remote" recovery (which could lead to a few extra minutes of test time) but can be turned back to "local" once the changes reach mainnet.
Classifies an advertised stream header by how much of its content the pool already has: accounted for by the cached stream position (`InPayload`), covered by the pooled slice (`Pooled`), covered by the peer's recorded header (`Duplicate`), or none of the above (`Actionable`). Content is messages or signals; or a `begin` far enough along to garbage collect a reject signal of ours, which is worth fetching on its own, hence the `have_reject_signal_between` argument. Also restores `peer_header()` as a plain accessor, for the advert handler and (later) the advert task to read the recorded headers back. Nothing calls this yet; the handler follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
24e88ea to
2df00a3
Compare
…sify_advert_collecting_reject_signal().
| header.end() <= messages_end | ||
| && header.signals_end() <= signals_end | ||
| // Plus no reject signal of ours left for the advertised `begin` to | ||
| // garbage collect. | ||
| && !have_reject_signal_between(header_begin, header.begin()) |
There was a problem hiding this comment.
@alin-at-dfinity Unless I missed something, this is not covered by tests.
There was a problem hiding this comment.
If you are referring to the have_reject_signal_between test, it is covered by pool_classify_advert_collecting_reject_signal(). The (simulated) reject signal at messages_begin (which only the advert covers) vs messages_begin - 1 (which the various references already cover) is the difference between Actionable vs all the other outcomes (depending on which reference we're setting).
Messages and signals are covered by the pool_classify_advert() test.
There was a problem hiding this comment.
┌──────────────────────────────────────────────────────────────────────┬──────────────────────┐
│ Mutation in certified_slice_pool.rs:1522 │ Result │
├──────────────────────────────────────────────────────────────────────┼──────────────────────┤
│ header.end() <= messages_end → true │ survives — 23 passed │
├──────────────────────────────────────────────────────────────────────┼──────────────────────┤
│ header.signals_end() <= signals_end → true │ survives — 23 passed │
├──────────────────────────────────────────────────────────────────────┼──────────────────────┤
│ !have_reject_signal_between(header_begin, header.begin()) → true │ caught │
├──────────────────────────────────────────────────────────────────────┼──────────────────────┤
│ swap args → have_reject_signal_between(header.begin(), header_begin) │ caught │
├──────────────────────────────────────────────────────────────────────┼──────────────────────┤
│ drop all three X = X.max(Y) accumulations → X = Y │ survives — 23 passed │
└──────────────────────────────────────────────────────────────────────┴──────────────────────┘
There was a problem hiding this comment.
Improved the unit test coverage. This stack of PRs started off as one monolithic change, so tests were written to cover the whole lot, not just this PR. Although it's quite possible that even so the coverage was more aimed at lines of code (e.g. new signals count as Actionable) rather than all combinations (e.g. new signals compared to the pool contents count as Actionable).
Regardless, improved the coverage.
But in the process, I realized that there was actually a (non-critical) hole in the classification logic. Namely, the messages of a pooled slice may not start from the (cached) expected message index if we just built a payload that did not get selected. So it's not sufficient to look at the messages.end() of a pooled slice, we also need to ensure that messages.begin() extends gap-free whatever we already inducted or put into payloads. So also added that and a test for it.
It's all in #11621.
3 participants
Classifies an advertised stream header by how much of its content the pool already has: accounted for by the cached stream position (
InPayload), covered by the pooled slice (Pooled), covered by the peer's recorded header (Duplicate), or none of the above (Actionable). Content is messages or signals; or abeginfar enough along to garbage collect a reject signal of ours, which is worth fetching on its own, hence thehave_reject_signal_betweenargument.Also restores
peer_header()as a plain accessor, for the advert handler and (later) the advert task to read the recorded headers back.Nothing calls this yet; the handler follows.