Skip to content

serviceability: authorize only new multicast roles - #4302

Merged
martinsander00 merged 2 commits into
mainfrom
ms/2596-authorize-new-multicast-roles
Sep 9, 2026
Merged

serviceability: authorize only new multicast roles#4302
martinsander00 merged 2 commits into
mainfrom
ms/2596-authorize-new-multicast-roles

Conversation

@martinsander00

@martinsander00 martinsander00 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Multicast updates check allowlists only when a user gains a publisher or subscriber role.
  • Existing EdgeSeat feed subscriptions remain intact when users add allowed publishing.

Testing

  • New tests recreate a publisher addition while the direct subscriber allowlist omits an existing feed role.

This change addresses infra#2596.

@martinsander00
martinsander00 requested review from a team and a lite review from Copilot September 9, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The authorization logic change is localized, is mirrored in the SDK preflight, and is covered by new program and SDK regression tests plus changelog entries.

Pull request overview

This PR changes UpdateMulticastGroupRoles so it checks multicast allowlists only when the instruction would grant a new publisher or subscriber role, instead of re-authorizing roles the user already holds. This aligns serviceability-program behavior with the EdgeSeat feed subscription model so existing subscriptions remain valid when adding publishing rights.

Changes:

  • Update onchain allowlist enforcement to run only for newly granted multicast roles.
  • Update the Rust SDK command preflight checks to mirror the onchain behavior.
  • Add regression tests (program + SDK) and document behavior in the changelog.
File summaries
File Description
smartcontract/sdk/rs/src/commands/multicastgroup/subscribe.rs Skip SDK-side allowlist checks for roles the user already has; add a regression unit test.
smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs Gate onchain allowlist checks on whether the role is newly granted for that group.
smartcontract/programs/doublezero-serviceability/tests/multicastgroup_subscribe_test.rs Add an integration test that preserves an existing subscriber role when adding publisher after sub allowlist removal.
CHANGELOG.md Add Serviceability and SDK entries describing the behavior change.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@martinsander00
martinsander00 enabled auto-merge (squash) September 9, 2026 19:34

@nikw9944 nikw9944 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks right: skipping the allowlist for an already-held role is state-neutral, and a genuinely new unallowlisted role is still rejected. One stale comment on the authorization model.

Docs & comments (non-blocking)

  • smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs:333 — The comments describing the multicast authorization model now contradict the code beneath them. This loop comment says each group "must be on the pass's allowlist", and the check_mgroup_allowlists doc (line 63-68) says a caller runs the allowlist check or the EdgeSeat feed gate, "never neither".

@martinsander00
martinsander00 merged commit 31f3a3c into main Sep 9, 2026
37 checks passed
@martinsander00
martinsander00 deleted the ms/2596-authorize-new-multicast-roles branch September 9, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants