feat: [DSM-148] XNetAdvertHandler for XNetPayloadBuilderImpl - #11602
alin-at-dfinity wants to merge 1 commit into
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to d194f26. Security OverviewDetected Code Changes
|
21f7bf3 to
828ebb1
Compare
Handles an advert — a certified, header-only stream slice pushed by a source subnet — by classifying it against our certified state first (`NothingNew`, answered with our own certified header) and then against the pool (§6). Only the two outcomes that act on the advert being genuine, `NothingNew` and `Actionable`, pay for a threshold-signature verification; the rest are dropped on the unverified claim alone, which can only understate what the peer holds. Every verified header is recorded in `peer_headers`: our only record of how far the peer has garbage collected its messages, hence of whether we still owe it an advert. Nothing calls this yet; the endpoint follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
828ebb1 to
d194f26
Compare
| /// The reason for rejecting a XNet advert. | ||
| #[derive(Debug)] | ||
| pub enum XNetAdvertError { | ||
| /// Could not be decoded, or its certification could not be verified. |
There was a problem hiding this comment.
How about distinguishing the two reasons in this enum?
| /// Records the header as the peer's high-water-mark header, unless one with | ||
| /// a greater certified height is already on record. |
There was a problem hiding this comment.
Requirement: the header has been verified.
| /// a certification at `CERTIFIED_HEIGHT`. | ||
| /// | ||
| /// Unlike `make_certified_stream_slice`, which goes through `FakeStateManager` | ||
| /// and its CBOR encoding, this can be handed to `decode_slice_header()`. |
There was a problem hiding this comment.
amend the comment by noting that this encoding is pbuf
| /// and its CBOR encoding, this can be handed to `decode_slice_header()`. | ||
| pub(crate) fn make_advert(stream: &Stream) -> CertifiedStreamSlice { | ||
| // `REMOTE_SUBNET`'s state, holding the advertised stream to us. Not our own | ||
| // state: the canonical traversal skips the loopback stream. |
There was a problem hiding this comment.
| // state: the canonical traversal skips the loopback stream. | |
| // state: an advert always targets a different subnet (and the canonical traversal skips the loopback stream so it could not be created anyway). |
| store | ||
| .expect_decode_certified_stream_slice() | ||
| .times(verifications) | ||
| .returning(move |_, _, _| Ok(decoded.clone())); |
There was a problem hiding this comment.
could we also assert that the input is as expected (to match that we decode "an advert carrying advertised's header" as in the doc comment)?
| /// An advert offering messages we have not inducted is actionable, and its | ||
| /// header is recorded. | ||
| #[tokio::test] | ||
| async fn handle_advert_actionable() { |
There was a problem hiding this comment.
the test handle_advert_redundant_copies starts off the same as this one so maybe they could be merged
| assert_matches!( | ||
| payload_builder.handle_advert(REMOTE_SUBNET, make_advert(&advertised)), | ||
| Ok(XNetAdvertOutcome::InPayload) | ||
| ); |
There was a problem hiding this comment.
we could also assert here that the (unverified as asserted by the zero in advert_store(&advertised, 0)) header was not recorded in the pool
| assert_matches!( | ||
| payload_builder.handle_advert(REMOTE_SUBNET, make_advert(&advertised)), | ||
| Ok(XNetAdvertOutcome::NothingNew) | ||
| ); |
There was a problem hiding this comment.
we could also assert that the (verified) advert was recorded
| LOCAL_SUBNET, | ||
| StreamConfig { | ||
| message_begin: OWN_MESSAGES_BEGIN, | ||
| message_end: OWN_MESSAGES_BEGIN, |
There was a problem hiding this comment.
is message_end: OWN_MESSAGES_BEGIN chosen so that the only thing we have for the sender is signal_end so that the sender can gc messages and advance message_begin: 0? or is it chosen so that the slice has no messages and thus it is always header-only?
| let expected_reply = own_header.clone(); | ||
| store | ||
| .expect_encode_certified_stream_slice() | ||
| .returning(move |_, _, _, _, _| Ok(own_header.clone())); |
There was a problem hiding this comment.
here we could assert that no messages are requested in the stream slice
| self.slice_pool.record_peer_header( | ||
| source_subnet, | ||
| slice.header(), | ||
| advert.certification.height, |
There was a problem hiding this comment.
we should also validate the height witness from the certificate:
| .certified_stream_store | ||
| .decode_certified_stream_slice(source_subnet, registry_version, &advert) | ||
| .map_err(|err| XNetAdvertError::Invalid(err.to_string()))?; | ||
| debug_assert_eq!(slice.header(), &claimed, "Inconsistent slice decoding"); |
There was a problem hiding this comment.
should we maybe turn this into a critical error since this would indicate malicious behavior?
|
|
||
| /// Records the header as the peer's high-water-mark header, unless one with | ||
| /// a greater certified height is already on record. | ||
| fn record_peer_header( |
There was a problem hiding this comment.
Peer headers are never pruned.
2 participants
Handles an advert — a certified, header-only stream slice pushed by a source subnet — by classifying it against our certified state first (
NothingNew, answered with our own certified header) and then against the pool. Only the two outcomes that act on the advert,NothingNewandActionable, require a threshold-signature verification; the rest are dropped on the unverified claim alone, which can only understate what the peer holds.Every verified header is recorded in
peer_headers: our only record of how far the peer has garbage collected its messages, hence of whether we still owe it an advert.Nothing calls this yet; the endpoint follows.