Skip to content

feat(xmtp_mls): same-commit registry lookahead — register-then-write in one commit#3866

Closed
tylerhawkes wants to merge 2 commits into
mainfrom
tyler/registry-lookahead
Closed

feat(xmtp_mls): same-commit registry lookahead — register-then-write in one commit#3866
tylerhawkes wants to merge 2 commits into
mainfrom
tyler/registry-lookahead

Conversation

@tylerhawkes

@tylerhawkes tylerhawkes commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Problem

Receivers dispatched and policy-checked every AppDataUpdate proposal against the pre-commit COMPONENT_REGISTRY snapshot. A commit that registers a new component and writes to it in the same commit was therefore rejected by every receiver — the write's type dispatch found no registry entry (UnknownComponent) and its policy lookup hit deny-by-default (NoRegistryEntry). "Register in an earlier commit" was documented as a convention (registry_table.rs) but enforced by nothing on the send side: any future client that batched the two would produce commits every fielded receiver rejects. Since v1.11 is the first stable release carrying the app-data validators, this acceptance-widening is only shippable now.

Change

New RegistryOverlay (groups/app_data/mod.rs): tracks the registry snapshot as a commit's proposals are walked in queue order, folding each COMPONENT_REGISTRY mutation so subsequent proposals see it. Both receive stages fold through this one type, so their semantics cannot drift:

  • Apply path (accumulate_app_data_updates): the in-batch chaining already computes the post-proposal registry snapshot; the overlay adopts it (set_snapshot) instead of re-applying the delta. Folding here is optimistic — results are discarded unless commit validation later passes.
  • Validate path (validate_app_data_update_proposals_in_commit): the overlay folds a registry mutation only after that proposal passes its own hardcoded super-admin-only policy check, so overlaid entries carry exactly the trust of entries committed earlier. Trusting them same-commit is equivalent to trusting them one commit later — no new abuse surface.

Queue order is fixed by the commit, so every honest receiver folds identically. Write-before-register in queue order still deterministically fails. Remove of the whole registry component clears the overlay to deny-all (that operation is itself rejected at validation — see #3863). The change-control docs in registry_table.rs and the stale corner-case comment in accumulate_app_data_updates now state the queue-order rule instead of the old trap.

validate_one_app_data_update becomes pub(super) (mirroring its _with_old_value sibling) for direct test coverage.

Testing

  • test_accumulate_registry_lookahead_register_then_write: registry-insert + write batched in queue order applies; the write dispatches through the just-registered String type. Mutation-verified: with the overlay's snapshot adoption neutralized, the test fails; restored, it passes.
  • test_accumulate_registry_lookahead_write_before_register_fails: reversed queue order still fails UnknownComponent — the overlay looks back, never ahead.
  • test_validate_registry_lookahead_policy_sees_folded_entry: drives validate_one_app_data_update + RegistryOverlay::fold directly — the same pair the commit-validation loop walks: deny-by-default before the fold, allow-all entry admits the write after it.
  • Existing intra-batch chaining test untouched and green. Full xmtp_mls + xmtp_mls_common suites against the local node: 1502/1502 (known flake should_reconnect retried). Changed files clean under clippy 1.97 (remaining flags on this machine are pre-existing trunk debt in identity.rs / mls_sync.rs / bidi_fuzz_tests.rs).
  • Not covered (noted honestly): an end-to-end staged-commit test through the full commit-validation loop — no production sender currently batches register+write in one commit, and hand-building the staged commit needs a harness this PR doesn't add. The loop-level fold is exercised via the direct-drive test above.

Composes with #3863: entry-level validation there means only structurally valid, super-admin-authored registry mutations ever reach the overlay.

Part of the pre-v1.11 app-data hardening plan (P0.6) — the last of the Phase-0 protocol-acceptance changes. Siblings: #3863, #3864, #3865.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PkZpajnDV829SVuW9danrW

Note

Add same-commit registry lookahead so register-then-write succeeds in one commit

  • Introduces RegistryOverlay in mod.rs to maintain an in-batch view of the component registry while processing AppDataUpdate proposals, updating the overlay as each registry mutation is applied in queue order.
  • Updates accumulate_app_data_updates and validate_app_data_update_proposals_in_commit in validated_commit.rs to use the overlay so later proposals in the same commit see components registered earlier in the batch.
  • A write queued before its registration in the same commit still fails (UnknownComponent); removing the registry mid-batch yields a deny-all view for subsequent proposals.
  • Adds tests covering register-then-write success, write-before-register failure, and direct overlay validation behavior.

Macroscope summarized ec6ef73.

@tylerhawkes
tylerhawkes requested a review from a team as a code owner July 17, 2026 00:39
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Claude finished @tylerhawkes's task —— View job


Code Review Complete

Review Date: 2026-07-20 18:50 UTC

Summary

The author's self-review (2026-07-17T08:36:00Z) already identifies two critical blocking issues with this PR. I've reviewed the code and can confirm these issues are valid and represent serious problems that should prevent this from merging into v1.11.


Critical Issues (Already Identified by Author)

1. Order Divergence Between Apply and Validate Paths

The PR's central premise is that both receive stages "fold registry updates identically in queue order." However:

  • Apply path (accumulate_app_data_updates in mod.rs:219): Walks committed_proposals() in raw commit-body order
  • Validate path (validate_app_data_update_proposals_in_commit in validated_commit.rs:1569): Walks staged_commit.app_data_update_proposals(), which the openmls fork sorts by (component_id, removes-first)

Since COMPONENT_REGISTRY (0x8000) is the minimum component ID, registry mutations always sort to the front during validation, making validation order-insensitive while apply enforces body order. This falsifies the invariant stated in lines 53-66 of mod.rs.

2. Honest-Sender Fork Risk

The loosened accumulate_app_data_updates runs at sender stage time (via stage_app_data_propose_and_commit at mod.rs:432), allowing an honest sender to build+publish a register+write batch that every receiver rejects at the standalone-proposal arrival gate (which validates against committed registry with no overlay). This causes the sender to fork itself off the group.


Additional Code Quality Observations

Beyond the blocking issues:

Line mod.rs:276-280: The comment states "The in-batch chaining above already computed the post-proposal snapshot; adopt it instead of re-applying the delta." This optimization is correct but tightly couples the overlay's correctness to the apply path's computation, making the code fragile to refactoring.

Coverage Gap: The PR notes (in description) that end-to-end staged-commit testing through the full commit-validation loop is missing. Given the order-divergence issue, this gap is particularly concerning.

Function visibility change (validated_commit.rs:1310): Making validate_one_app_data_update pub(super) for testing is reasonable, but the test coverage added doesn't catch the order-divergence bug.


Recommendation

Strongly agree with the author's recommendation to defer this PR. The two fork risks and the falsified invariant make this unsuitable for v1.11. Same-commit register-then-write can be added correctly later, gated behind a PROPOSALS_MIN_PROTOCOL_VERSION floor bump.

If this must ship in v1.11, the reconciliation would require:

  1. Validation iterating the same source/order as apply (not the sorted iterator)
  2. Standalone-proposal arrival path needs consistent overlay semantics
  3. Fix contradictory change-control docs at both validator entry points

@macroscopeapp

macroscopeapp Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces new runtime behavior in MLS commit validation - enabling same-commit register-then-write patterns that were previously rejected. While well-tested and authored by a code owner, this changes core validation logic affecting what commits are accepted across all receivers, warranting human review.

No code changes detected at ec6ef73. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.60563% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.68%. Comparing base (62976d0) to head (ec6ef73).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/xmtp_mls/src/groups/app_data/mod.rs 64.28% 20 Missing ⚠️
crates/xmtp_mls/src/groups/validated_commit.rs 80.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3866      +/-   ##
==========================================
+ Coverage   85.61%   85.68%   +0.06%     
==========================================
  Files         415      415              
  Lines       67401    67706     +305     
==========================================
+ Hits        57704    58012     +308     
+ Misses       9697     9694       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tylerhawkes

Copy link
Copy Markdown
Contributor Author

Re-review (D9): BLOCK — recommend deferring this loosening out of v1.11

Adversarial re-review + an independent verifier found two critical issues, both rooted in the same-commit register-then-write loosening this PR introduces:

  1. The central invariant is false. The PR's premise is "both receive stages fold registry updates identically in queue order." They don't: the apply stage walks committed_proposals() in raw commit-body order, while the validate stage walks staged_commit.app_data_update_proposals(), which the openmls fork sorts by (component_id, removes-first) (proposal_store.rs:505). Because COMPONENT_REGISTRY (0x8000) is the minimum id, registry mutations always sort to the front of validation — so validation is order-insensitive while apply enforces body order. No same-version fork is reachable today (apply gates first, deterministically), but every "the two stages can't drift" comment shipped here is wrong, one shape ([register X, write X, Remove registry]) both-accepts yet silently de-migrates, and any future refactor flips acceptance silently.

  2. It arms an honest-sender fork. The loosened accumulate_app_data_updates (with the overlay) runs at sender stage time too, so an honest sender can build+publish a register+write batch that every receiver rejects at the standalone-proposal arrival gate (which validates against the committed registry, with no overlay) → the sender forks itself off the group.

Recommendation: defer. libxmtp doesn't currently produce same-commit register-then-write (registration and component-writes each go through their own propose-then-commit cycle), so this is speculative future-proofing that arms two fork risks while falsifying its own invariant. Keeping the strict pre-PR behavior (write-to-unregistered-component rejected) is the safe choice for the permanent-floor v1.11 contract. Same-commit register-then-write can be added later, correctly, gated behind a PROPOSALS_MIN_PROTOCOL_VERSION floor bump — loosening acceptance post-cut needs a floor bump anyway, so nothing is lost by deferring.

If we do want it in v1.11, the reconciliation is non-trivial: validation must iterate the same source/order as apply (not the fork's sorted iterator), the standalone-proposal arrival path needs consistent overlay semantics, and the contradictory change-control docs at both validator entry points need fixing — see app-data-v1.11-hardening-plan.md "D9 re-review results".

@tylerhawkes

Copy link
Copy Markdown
Contributor Author

This currently goes against XIP-81 spec, so will be closing for now.

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.

2 participants