Skip to content

docs(l2): fix the Aligned contract-addresses link and stop link-checking x.com - #7266

Open
ilitteri wants to merge 2 commits into
mainfrom
docs/fix-dead-links
Open

ilitteri wants to merge 2 commits into
mainfrom
docs/fix-dead-links

Conversation

@ilitteri

@ilitteri ilitteri commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Motivation

The docs link check (Link Check in the mdBook workflow) has been failing on every PR with the same 9 URLs, none of which are the PR's own changes.

Description

  • Aligned renumbered its contract-addresses guide (7_contract_addresses8_contract_addresses; the old URL is a 404). Both references updated.

  • x.com returns 403 to non-browser clients, so the eight profile links in the based-rollup page fail the check while resolving fine in a browser. Added the domain to lychee.toml's exclude list, alongside the existing entries for medium.com and mirror.xyz, which have the same behaviour.

  • The Teku entry in the consensus-client list pointed at consensys.io/teku, now a 404; it points at docs.teku.consensys.io instead. A full local lychee run over docs/ with this configuration reports 954 links, 0 errors.

Checklist

  • Docs and CI config only.

…top link-checking x.com

Aligned moved its contract addresses guide from 7_contract_addresses to
8_contract_addresses; the old URL returns 404 and the docs link check has been
failing every PR on it. The same check also fails on eight x.com profile links
in the based-rollup page: x.com answers non-browser clients with 403 while the
profiles resolve fine in a browser, so the domain is excluded like medium.com
and mirror.xyz already are.
@ilitteri
ilitteri requested a review from a team as a code owner September 9, 2026 14:25
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.


The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.


ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rsunsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.


Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

  1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
    reviewers, and add environment: to the two jobs that sign.
  2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

@github-actions github-actions Bot added the L2 Rollup client label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR diff focusing on the aspects relevant to an Ethereum execution client codebase.

Summary

This PR contains documentation link updates and a link checker configuration change. No Rust code, EVM logic, consensus rules, or blockchain-critical code is modified.


Review by File

1. docs/l2/deployment/aligned.md (lines 53-56)

Change: Updates Aligned Layer documentation link from /guides/7_contract_addresses to /guides/8_contract_addresses

  • Verification needed: Confirm this URL path change is correct with Aligned Layer's actual documentation structure. The PR assumes the renumbering is accurate.
  • No security concerns: Documentation-only change.

2. docs/l2/fundamentals/ethrex_l2_aligned_integration.md (lines 541-544)

Change: Same link update as above

  • Consistency: Good — both references to this URL are updated together, preventing broken links.
  • Suggestion: Consider if this URL is referenced elsewhere in the codebase. A quick grep for 7_contract_addresses would ensure no stale references remain.

3. lychee.toml (lines 24-28)

Change: Adds x\.com to link checker exclusion list with explanatory comment

Assessment:

Aspect Evaluation
Correctness Valid regex escape (\.); matches x.com domain
Rationale Accurate — X/Twitter returns 403 to non-browser clients
Comment clarity Good — explains why not just what

Minor suggestion: The comment says "profile links resolve in a browser" which is true, but the exclusion applies to all x.com links, not just profiles. Consider: "x.com answers non-browser clients with 403; links are verified to resolve correctly in browsers." (line 27)


Overall Assessment

Approve with minor note.

This is a straightforward maintenance PR. The changes are:

  • Correctly scoped to documentation and CI configuration
  • Internally consistent (both doc files updated together)
  • Properly motivated (external URL structure changed; link checker false-positive addressed)

No blockchain-critical code affected. No further action required beyond optional comment refinement in lychee.toml.


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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

This is a docs/CI-only change with no execution-client code, so most of the review checklist (EVM, gas, consensus, RLP, state trie) doesn't apply. The changes are correct and minimal, matching the stated motivation. Two small observations on the regex pattern:

Findings

  1. lychee.toml:31 — unanchored regex can over-match domains containing "x.com" as a substring. Since lychee's exclude entries are regexes (as shown by the escaped dots on other entries), the bare pattern x\.com will match any URL containing the substring x.com, not just the x.com domain — e.g. https://telex.com/... or https://foo.com/vertex.com/bar would also be silently excluded from link checking. Given the existing entries (medium\.com, mirror\.xyz, etc.) have the same unanchored style, this is consistent with the file's existing convention rather than a new problem introduced by this PR, but since x is a single character it's far more prone to accidental collisions than medium or mirror. Consider anchoring to the host, e.g. ^https://(www\.)?x\.com or at least ://x\.com, to avoid excluding unrelated links that happen to contain the substring.

  2. Link correctness (docs/l2/deployment/aligned.md:56, docs/l2/fundamentals/ethrex_l2_aligned_integration.md:544) — I can't independently browse to Aligned's docs site from here to confirm 8_contract_addresses is the correct renumbered slug, but the change is consistent between both occurrences and matches the PR description's explanation. Low risk either way since it's an external doc link, not runtime behavior.

No correctness, security, or consensus-related concerns — this PR doesn't touch any execution/consensus code paths.


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

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

This PR only changes documentation links and the Lychee exclusion list, so I don’t see any impact on execution, consensus, gas accounting, storage, or validation code paths. The x.com exclusion in lychee.toml is consistent with the existing bot-blocked host exceptions, and the updated Aligned docs references in docs/l2/deployment/aligned.md and docs/l2/fundamentals/ethrex_l2_aligned_integration.md look straightforward.

Residual risk is limited to external-doc drift: I did not validate the remote Aligned URLs from this environment, so this review assumes guides/8_contract_addresses is the intended live destination.


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

consensys.io/teku now returns 404; the client's documentation lives at
docs.teku.consensys.io. With this the link check passes over the whole docs
tree (954 links, 0 errors).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant