Skip to content

refactor: centralize MLS group-state validation in state_validation.rs #260

@erskingardner

Description

@erskingardner

Context

#256 introduced a new top-level module, crates/mdk-core/src/state_validation.rs, as part of the receiver-side admin-depletion fix for marmot-security#29. It now holds helpers that reason about MLS group composition before vs. after a staged commit, including:

  • live_member_identities
  • post_commit_member_identities
  • validate_active_admins*
  • validate_admin_depletion
  • validate_admin_invariant_after_commit

The module's docs intentionally define this validation in terms of Nostr identities represented by live MLS leaves, not raw MLS leaf count. Multiple MLS leaves may legitimately carry the same Nostr identity, so identity-set deduplication is expected and must be preserved. The public get_members(group_id) API remains the storage-loading wrapper for callers that do not already hold an MlsGroup.

This issue tracks completing that cleanup by moving the remaining MLS group-state and group-evolution validators out of message/group operation modules and into state_validation.rs.

Goal

Make validation easier to reason about by formalizing this split:

  • crates/mdk-core/src/messages/validation.rs: Nostr transport, event-shape, timestamp, tag, and event-author validation.
  • crates/mdk-core/src/state_validation.rs: MLS group-state, staged-commit, authorization, identity-in-leaf, admin-identity, group-creation, and group-update invariants.
  • crates/mdk-core/src/groups.rs: group operations and storage orchestration, calling shared state validators rather than owning bespoke validation logic.

This should be a pure organization/refactor PR. The point is to make invariants discoverable and reusable, not to change protocol behavior.

Move from messages/validation.rs to state_validation.rs

  • validate_identity_unchanged
  • validate_proposal_identity
  • validate_commit_identities
  • validate_commit_authorization
  • is_pure_self_update_commit
  • is_self_remove_only_commit

Move the associated unit tests for these functions alongside them where practical.

Move from groups.rs to state_validation.rs

  • is_leaf_node_admin
  • validate_group_members
  • prune_and_validate_admin_update

Move or update the associated unit tests so they continue to cover the same behavior after relocation.

Leave in messages/validation.rs

These are Nostr transport / event-shape concerns and should stay put:

  • verify_rumor_author
  • validate_event / validate_event_at
  • validate_created_at_with_now
  • extract_nostr_group_id

After moving the MLS-state helpers, update the module-level docs so this file clearly describes Nostr/message validation rather than MLS state validation.

Leave in groups.rs

Keep group operations, storage writes, MLS commit creation/merge orchestration, and general group query helpers in groups.rs. It is fine for those methods to call validators from state_validation.rs.

Do not move general identity extraction helpers such as pubkey_from_credential, pubkey_for_leaf_node, pubkey_for_member, or get_own_pubkey unless doing so clearly reduces coupling without widening the behavioral scope.

Constraints / non-goals

  • No behavior change: preserve existing acceptance/rejection behavior.
  • Preserve existing error variants and strings unless a compile-time visibility change makes a tiny adjustment unavoidable.
  • Preserve Nostr identity semantics: multiple MLS leaves may represent the same PublicKey; do not add identity uniqueness checks.
  • Keep these helpers internal (pub(crate) where cross-module callers need access, not public SDK API).
  • Preserve the existing validate_admin_depletion precondition: it reads the current/pre-commit group-data extension and is only appropriate for proposal-time checks where no staged commit exists. Commit-time validation must continue to use the staged/post-commit group context.
  • Avoid renaming state_validation.rs in this PR unless there is a clear consensus that the churn is worth it.

Acceptance criteria

  • The six MLS-state helpers listed from messages/validation.rs live in state_validation.rs.
  • The three group-evolution validators listed from groups.rs live in state_validation.rs.
  • messages/validation.rs contains only Nostr/message transport validators and matching tests.
  • groups.rs delegates group-state validation to state_validation.rs instead of owning bespoke validator methods.
  • Associated tests are moved or updated without losing coverage.
  • cargo test -p mdk-core passes.
  • Before PR submission, just precommit passes.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions