Skip to content

feat: implement related contracts mechanism (depth=1)#3650

Open
sanity wants to merge 8 commits intomainfrom
related-contracts-depth1
Open

feat: implement related contracts mechanism (depth=1)#3650
sanity wants to merge 8 commits intomainfrom
related-contracts-depth1

Conversation

@sanity
Copy link
Copy Markdown
Collaborator

@sanity sanity commented Mar 25, 2026

Problem

Related contracts allow a contract's validate_state to return RequestRelated(contract_ids) requesting other contracts' state for validation. This enables use cases like River room membership gates (contract B validates against contract A's state) and Bitcoin header verification. The mechanism was partially wired up but had broken code paths, no safety limits, no real tests, and a buggy recursive validation flow with DEPENDENCY_CYCLE_LIMIT_GUARD = 100.

Approach

Depth=1 only, one round. When validate_state returns RequestRelated(ids), we fetch those contracts locally and re-call validate_state exactly once. If the second call also returns RequestRelated, that's an error. This prevents amplification attacks where a malicious contract requests new contracts on every retry.

Key design decisions:

  • Conservative start: depth=1 is easy to reason about and increase later
  • One round only: contracts must declare all dependencies in one call
  • Local fetch only: the fetch_related_for_validation helper uses lookup_key + state_store.get() (available on all executor types). Network fetch happens automatically via GET auto-subscribe when contracts are first retrieved
  • Network inbound deferred: when another node broadcasts an update, RequestRelated still errors — the sending node already validated. Filing a separate issue for this

Abuse prevention:

  • Self-reference explicitly rejected before any fetch
  • Max 10 related contracts per request
  • 10s overall fetch timeout
  • Empty RequestRelated(vec![]) rejected
  • Duplicate IDs deduped via HashSet
  • Second RequestRelated after providing data → error (depth>1 not supported)

Changes

  1. fetch_related_for_validation helper — shared validation logic with all safety checks
  2. Simplified verify_and_store_contract — replaced 100-iteration loop with clean depth=1 flow
  3. Fixed 4 validation call sites — new PUT, post-merge, perform_contract_put, get_updated_state
  4. Extended MockWasmRuntimeValidateOverride enum enables per-contract validation behavior in tests
  5. Updated .claude/rules/contracts.md — documented depth=1 design decisions

Testing

10 new unit tests covering all paths and edge cases:

Test What it validates
test_put_with_related_succeeds PUT gated contract after gate exists locally
test_put_with_gate_closed Validation returns Invalid
test_put_self_reference_rejected Contract requests own ID → error
test_put_too_many_related_rejected >10 related contracts → error
test_put_missing_related_errors Non-existent contract → error, not hang
test_depth_exceeded_rejected AlwaysRequestRelated → depth>1 error
test_empty_request_related Empty vec → error
test_duplicate_ids_deduped Same ID twice → fetched once, succeeds
test_existing_contract_put_with_related Merge path fetches related
test_update_with_related_succeeds Delta update path fetches related

Full test suite: 2108 passed, 0 failures, clippy clean, fmt clean.

[AI-assisted - Claude]

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

The existing .unwrap() calls are all pre-existing code, not introduced by this PR. I have enough to write the review.


Rule Review: One INFO finding — duplicate safety checks

Rules checked: contracts.md, code-style.md, testing.md, git-workflow.md
Files reviewed: 6 (runtime.rs, mock_wasm_runtime.rs, pool_tests.rs, related_contract_tests.rs, wasm_conformance_tests.rs, .claude/rules/contracts.md)

Warnings

None.

Info

  • crates/core/src/contract/executor/runtime.rs (the new fetch_related_for_validation_network impl, around the safety-check block) — The three safety guards — empty-list rejection, self-reference rejection, and > MAX_RELATED_CONTRACTS_PER_REQUEST rejection — are copy-pasted verbatim from fetch_related_for_validation. The two functions differ only in how they fetch (local state_store.get vs local_state_or_from_network). If either constant or check changes in the future, both copies must be updated in sync. Extracting the three guards into a shared private helper (e.g., validate_related_ids(key, ids) -> Result<HashSet<_>, ExecutorError>) would eliminate the drift risk. (rule: code-style.md — style/DRY concern)

Everything else looks well-implemented:

  • No .unwrap() in new production paths; all error paths use map_err / ok_or_else / ?.
  • fetch_related_for_validation correctly documents and enforces the depth=1 contract in its doc comment and returns only Valid/Invalid from Ok(...).
  • The RELATED_FETCH_TIMEOUT via tokio::time::timeout (not sleep) is acceptable — the rules only ban tokio::time::sleep() explicitly, and timeout wraps a bounded async op rather than consuming time directly.
  • The removal of DEPENDENCY_CYCLE_LIMIT_GUARD loop and the ValidateResult::Valid => unreachable!() replacement with tracing::error! + internal_error() are both positive improvements.
  • Test coverage is thorough: boundary (exactly 10), >10 rejection, self-reference, empty, dedup, depth>1, missing related, merge path, and update path are all exercised.
  • Updated contracts.md accurately documents the new depth=1 design, limits, and anti-patterns.

Advisory review against .claude/rules/. Critical patterns are enforced by the Rule Lint CI job.

@sanity sanity requested a review from iduartgomez March 26, 2026 02:34
}
continue;
// Validate with depth=1 related contract resolution
let result = self
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should document here for future reference to protect against depth requests

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added a detailed depth protection comment at this call site documenting all the safety mechanisms: depth=1 limit, one-round-only, self-reference rejection, max 10 contracts, and timeout. Points to the constants for reference.

[AI-assisted - Claude]

sanity and others added 5 commits March 27, 2026 12:22
When a contract's validate_state returns RequestRelated(ids), the executor
now fetches those contracts locally and re-calls validate_state with the
populated RelatedContracts map. This enables contracts to validate their
state against other contracts' state (e.g., membership gates).

Design decisions:
- Depth=1 only: related contracts cannot request their own related contracts
- One round: if second validate_state also returns RequestRelated, it's an error
- Limits: max 10 related contracts, 10s timeout, no self-reference
- Network inbound path deferred (sending node already validated)

Key changes:
- Add fetch_related_for_validation helper with abuse prevention
- Simplify verify_and_store_contract from 100-iteration loop to depth=1 flow
- Fix 4 code paths: new PUT, post-merge, perform_contract_put, get_updated_state
- Extend MockWasmRuntime with configurable validate_overrides for testing
- Add 10 unit tests covering success, rejection, and edge cases

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove dead code paths for RequestRelated after fetch_related_for_validation
(which resolves it internally), consolidate validation_error helper, clean up
test comments, and remove redundant .into_owned() call.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace unreachable!() with proper error returns (no panics in production)
- Add init_tracker.fail_initialization cleanup when validation errors
- Merge initial_related with fetched data on retry call
- Replace unreachable! in validation_error with defensive error logging
- Fix docs inconsistency about network inbound behavior
- Add boundary test (exactly 10 related contracts)
- Add multi-contract test (3 distinct related contracts)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add detailed comment at verify_and_store_contract documenting all
depth=1 safety mechanisms per Nacho's review feedback.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codex review found two issues:

P1: fetch_related_for_validation only did local lookups, breaking
first-time publishes that depend on contracts not yet stored locally.
Added fetch_related_for_validation_network on Executor<Runtime> that
uses local_state_or_from_network. The Executor<Runtime>-specific paths
(verify_and_store_contract, perform_contract_put, get_updated_state)
now use this network-aware version while the bridged generic impl
keeps local-only lookups.

P2: validation_error always returned ContractError::Update, but PUT
paths should return StdContractError::Put. Added validation_error_put
for PUT paths to preserve correct error semantics.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sanity sanity force-pushed the related-contracts-depth1 branch from 02ab734 to 2d1542f Compare March 27, 2026 17:35
@sanity sanity enabled auto-merge March 31, 2026 00:38
@sanity sanity added this pull request to the merge queue Mar 31, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 31, 2026
@sanity sanity enabled auto-merge March 31, 2026 02:02
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