Skip to content

docs: add spec deviations page#499

Draft
MegaRedHand wants to merge 3 commits into
mainfrom
docs/spec-deviations-page
Draft

docs: add spec deviations page#499
MegaRedHand wants to merge 3 commits into
mainfrom
docs/spec-deviations-page

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

Summary

Adds docs/spec_deviations.md documenting where ethlambda intentionally diverges from the leanSpec reference, mainly for performance:

  • Asynchronous signature aggregation with early stop — aggregation runs off the actor loop on a spawn_blocking worker with a 750ms deadline, vs leanSpec's inline synchronous aggregate().
  • Attestation scoring on block building — attestations are tier-scored (Finalize > Justify > Build) and selected against a projected post-state when packing a block, vs leanSpec's greedy slot-ordered inclusion.

Also registers the page in the mdBook SUMMARY.md under a new Development section so it actually renders in the book.

Source references were verified against current code (line numbers, and the tier-dependent attestation tie-break per leanSpec #1149).

Split out from #498, which now carries only the image-tag wording changes.

Documents two performance-driven deviations from leanSpec (asynchronous
signature aggregation with early stop; attestation scoring on block
building) and registers the page in the mdBook SUMMARY under a new
Development section so it renders in the book.

Source references verified against current code: line numbers and the
tier-dependent attestation tie-break (leanSpec #1149).
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review: PR #499 — docs: add spec deviations page

This is a documentation-only change (new docs/spec_deviations.md + a SUMMARY.md entry). No source code is touched, so the usual correctness/security/consensus concerns don't apply — I instead verified the doc's factual claims against the current codebase.

Verification performed:

Claim in doc Verified against
run_aggregation_worker, aggregation.rs:504 Matches exactly
AGGREGATION_DEADLINE, aggregation.rs:33 = 750ms Matches
cancel.is_cancelled() check, aggregation.rs:519 Matches
MILLISECONDS_PER_INTERVAL = 800ms, lib.rs:47 Matches
select_attestations, block_builder.rs:170 Matches
enum Tier, block_builder.rs:473 (Finalize > Justify > Build) Matches
EntryScore::ordering_key, block_builder.rs:517 (tier-dependent tiebreak) Matches
MAX_ATTESTATIONS_DATA = 8, common/types/src/block.rs:109 Matches

All line numbers and code references are accurate as of the current main. The SUMMARY.md change follows the existing mdBook section pattern (# Consensus, # Operations, now # Development) and will render correctly.

Assessment: No issues found. The content is accurate, well-sourced, and appropriately scoped (split cleanly from PR #498 per the description). Good practice documenting why each deviation exists rather than just what differs from leanSpec — useful for onboarding and future spec-conformance audits.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. docs/spec_deviations.md overstates the aggregation equivalence. The text says “same XMSS proofs are produced,” but the implementation cancels the worker at +750 ms and stops starting new jobs once cancel.is_cancelled() is set, so a timed-out session can publish only a subset of the proofs the synchronous leanSpec path would have completed. See crates/blockchain/src/lib.rs and crates/blockchain/src/aggregation.rs. I’d reword this to say the proof logic is the same per completed job, but ethlambda may intentionally emit partial results under deadline pressure.

  2. docs/spec_deviations.md embeds exact source line numbers in rendered documentation (aggregation.rs:504, lib.rs:47, block_builder.rs:517, etc.). Those will drift on routine refactors and leave the doc stale even when the behavior is unchanged. For maintainability, prefer stable symbol/file references or repo permalinks instead of raw line numbers in prose.

Aside from those doc issues, I did not find code correctness, security, or performance regressions in this PR because it only changes documentation. I did not run tests.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

Review of PR #499: Spec Deviations Documentation

Overall, this is valuable documentation for consensus auditability. However, several clarifications are needed regarding determinism, safety bounds, and documentation maintenance.

Critical Concerns

1. Wall-clock dependence in consensus-critical code (aggregation.rs)
The 750ms early-stop mechanism introduces hardware/execution-speed dependence. Different validators with different CPU loads may produce different partial aggregates at the deadline.

  • Line 12-14: Clarify whether this non-determinism is acceptable because:
    • The block is still valid with any subset of valid aggregates, OR
    • There is a deterministic minimum set that must complete before cancellation
  • Risk: If two validators produce conflicting blocks due to timing jitter, fork choice must handle this. Document that partial aggregation does not affect slashability or consensus safety.

2. "Same XMSS proofs are produced" contradiction

  • Line 16: Claims "same XMSS proofs are produced" but then describes "partial-result bound". These are contradictory.
  • Fix: Change to "same validation rules apply; deviation is in the subset of signatures aggregated, not the aggregation algorithm itself."

3. Determinism of attestation scoring (block_builder.rs)

  • Line 24: The scoring uses data_root as final tiebreak. Ensure the doc notes that:
    • The ordering must be strictly deterministic across all architectures (no floating-point in score calculation, no HashMap iteration order)
    • EntryScore::ordering_key (line 517) must use canonical SSZ roots, not runtime pointers or non-deterministic hashes

Security & Performance

4. Timing attack surface

  • Line 13: The 50ms safety margin (800ms - 750ms) assumes specific network topology. Consider documenting:
    • The assumption of sub-50ms gossip propagation time
    • Whether this is configurable for different network conditions (testnets vs mainnet)
    • Consequences if the deadline hits during the 50ms window (missed slot?)

5. Cancellation safety

  • Line 14: "in-flight jobs finish" suggests cooperative cancellation. Verify and document:
    • Whether XMSS aggregation is atomic per group (no partial state corruption if cancelled)
    • Whether cancel.is_cancelled() (line 519) is checked between groups or also during long XMSS operations
    • Memory safety: Ensure the spawned blocking task doesn't hold references to actor state that could be invalidated by the deadline timer

Documentation Quality

6. Fragile line number references
Line numbers will drift immediately upon code changes. Suggest:

  • Use module paths + function names as primary references: crates/blockchain/src/aggregation.rs::run_aggregation_worker
  • Add GitHub permanent links with commit hash for precise line references: https://github.com/lambdaclass/ethlambda/blob/<commit>/crates/...

7. Missing equivalence proof for attestation scoring

  • Line 27: "both produce a valid block" is asserted but not proven. While true, consider noting:
    • Whether ethlambda's scoring could produce a different canonical chain than leanSpec under extreme network latency
    • Whether the tier prioritization (Finalize > Justify > Build) could delay finalization in edge cases compared to slot-order greedy inclusion

Minor Issues

8. Inconsistent formatting

  • Line 1: "ethlambda" vs repository name "ethlambda" (consistency check)
  • Line 11: tokio::task::spawn_blocking code formatting is good, but consider linking to Tokio docs for cancellation behavior

9. XMSS context
Since XMSS is non-standard (Ethereum typically uses BLS), consider adding a footnote explaining that XMSS signature aggregation is itself a spec deviation from standard Ethereum consensus, or link to where that's documented.

Summary

The deviations described appear sound from a consensus perspective (partial aggregation produces valid states; scoring preserves validity), but the documentation should explicitly address determinism and timing assumptions. Request changes for Items 1, 2, and 6 before merge.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a docs/spec_deviations.md page documenting where ethlambda intentionally diverges from the leanSpec reference implementation, and registers the page in docs/SUMMARY.md under a new "Development" section. All code cross-references (line numbers, symbol names, constant values) were verified against current source and are accurate.

  • Covers two deviations: asynchronous XMSS signature aggregation with a 750ms cancellation deadline, and tier-scored attestation selection during block building (Finalize > Justify > Build), both with correct pointers to implementation symbols and leanSpec equivalents.
  • File ends with two trailing blank lines (line 24–25) instead of one; minor but inconsistent with the rest of the docs directory.

Confidence Score: 5/5

Documentation-only change with no impact on runtime behavior; safe to merge.

All code cross-references (function names, constant values, line numbers) were verified against current source and are accurate. The only findings are a single-word grammar fix and a trailing blank line — neither affects the correctness or completeness of the documentation.

No files require special attention.

Important Files Changed

Filename Overview
docs/spec_deviations.md New documentation page; all symbol names and line numbers verified against current source. Minor grammar nit on line 10 ("ran" should be "runs") and two trailing blank lines at EOF.
docs/SUMMARY.md Adds a new "Development" section with the Spec Deviations entry; change is minimal and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[interval 2 tick] --> B[Actor snapshots state]
    B --> C[spawn_blocking: run_aggregation_worker]
    B --> D[send_after 750ms deadline]
    C --> E{cancel.is_cancelled?}
    E -- No --> F[aggregate_job]
    F --> G[AggregateProduced to Actor]
    G --> E
    E -- Yes --> H[AggregationDone to Actor]
    D --> I[cancel token set] --> E
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[interval 2 tick] --> B[Actor snapshots state]
    B --> C[spawn_blocking: run_aggregation_worker]
    B --> D[send_after 750ms deadline]
    C --> E{cancel.is_cancelled?}
    E -- No --> F[aggregate_job]
    F --> G[AggregateProduced to Actor]
    G --> E
    E -- Yes --> H[AggregationDone to Actor]
    D --> I[cancel token set] --> E
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
docs/spec_deviations.md:10
Minor grammar issue: "ran" is past tense, but the sentence describes current, ongoing behavior. "runs" (or "has run") matches the present-tense framing used in the rest of the section.

```suggestion
Aggregation runs off the main BlockChainServer actor loop and stops early once it runs out of time.
```

### Issue 2 of 2
docs/spec_deviations.md:23-24
The file ends with two blank lines rather than one, which is inconsistent with other pages in the `docs/` directory.

```suggestion
- **Equivalence:** both produce a valid block; ethlambda prioritizes attestations that advance finality/justification rather than processing in slot order.
```

Reviews (1): Last reviewed commit: "docs: add spec deviations page to the bo..." | Re-trigger Greptile

Comment thread docs/spec_deviations.md Outdated

## Asynchronous signature aggregation with early stop

Aggregation runs off the main BlockChainServer actor loop and stops early once it ran out of time.

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.

P2 Minor grammar issue: "ran" is past tense, but the sentence describes current, ongoing behavior. "runs" (or "has run") matches the present-tense framing used in the rest of the section.

Suggested change
Aggregation runs off the main BlockChainServer actor loop and stops early once it ran out of time.
Aggregation runs off the main BlockChainServer actor loop and stops early once it runs out of time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/spec_deviations.md
Line: 10

Comment:
Minor grammar issue: "ran" is past tense, but the sentence describes current, ongoing behavior. "runs" (or "has run") matches the present-tense framing used in the rest of the section.

```suggestion
Aggregation runs off the main BlockChainServer actor loop and stops early once it runs out of time.
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread docs/spec_deviations.md Outdated
Comment on lines +23 to +24
- **Equivalence:** both produce a valid block; ethlambda prioritizes attestations that advance finality/justification rather than processing in slot order.

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.

P2 The file ends with two blank lines rather than one, which is inconsistent with other pages in the docs/ directory.

Suggested change
- **Equivalence:** both produce a valid block; ethlambda prioritizes attestations that advance finality/justification rather than processing in slot order.
- **Equivalence:** both produce a valid block; ethlambda prioritizes attestations that advance finality/justification rather than processing in slot order.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/spec_deviations.md
Line: 23-24

Comment:
The file ends with two blank lines rather than one, which is inconsistent with other pages in the `docs/` directory.

```suggestion
- **Equivalence:** both produce a valid block; ethlambda prioritizes attestations that advance finality/justification rather than processing in slot order.
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

- Drop drift-prone line numbers; keep symbol + file references.
- Correct the aggregation equivalence: the interval-2 worker snapshots
  only current-slot raw gossip signatures (skips proof reuse / stale
  groups) and may emit a partial result on cancellation, so it does not
  reproduce leanSpec's full aggregate set. Note the subset still yields a
  valid block.
- Clarify the attestation cap is 8 distinct AttestationData entries, with
  per-entry proofs compacted back to one-per-data later.
- Grammar (ran -> runs); trailing-newline cleanup.
MegaRedHand added a commit that referenced this pull request Jul 6, 2026
## Summary

Clarifies the Docker image-tag wording in `README.md` and `RELEASE.md`:

- `unstable` is built from the latest `main` commit.
- `latest` / `devnetX` are the latest **stable** devnet images.
- Bumped the stale `devnet3` "at the time of writing" and `docker pull`
examples to `devnet5` (the current devnet).

## Notes

- Left untouched: `Cargo.toml` `leansig` `devnet4` branch (real
dependency), the `devnet3/devnet4` test-ignore comment in `encoding.rs`,
and the README "Older devnets" list (`devnet1-4` are correctly the
*older* ones; `devnet5` is current).
- The new `docs/spec_deviations.md` page (and its `SUMMARY.md` index
entry) was split into #499.
@MegaRedHand MegaRedHand marked this pull request as draft July 6, 2026 15:53
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.

1 participant