revert(l1): drop blockTimestamp from RPC log objects (#7142)

[ilitteri]

Motivation

Reverts #7142.

The blockTimestamp key that #7142 added to RPC log objects makes eight hive rpc-compat cases fail: the simulation compares responses byte-exactly against a corpus pinned to execution-apis d08382ae, which predates the field. The pin cannot move forward (every revision whose fixtures carry blockTimestamp also carries a pre-merge test chain, which ethrex does not support), so the daily hive report has been stuck at RPC API Compatibility: 88/96 (91.67%) since the change landed. #7142 anticipated this and excluded the eight cases in KNOWN_EXCLUDED_TESTS, but the coverage loss is real — all four eth_getLogs cases are in the excluded set, leaving that method with no rpc-compat coverage at all.

Description

Removes block_timestamp from RpcLog (one struct change covers eth_getLogs, eth_getTransactionReceipt and eth_getBlockReceipts, which all build the same type), and drops the eight KNOWN_EXCLUDED_TESTS entries plus the docs/known_issues.md section that #7142 introduced.

Trade-off, kept explicit: log objects no longer carry blockTimestamp, diverging from the other clients that populate it. Hive conformance and full eth_getLogs rpc-compat coverage win over the convenience field.

How to test

cargo test -p ethrex-rpc --lib

End to end, validated locally with hive at the CI-pinned commit (7c4c99e) against a locally built image of this branch:

./hive --client-file ../fixtures/hive/clients.yaml --client ethrex \
  --sim ethereum/rpc-compat --sim.parallelism 4 --sim.loglevel 1 \
  --sim.buildarg "branch=d08382ae5c808680e976fce4b73f4ba91647199b"

Result: tests=96 failed=0 — including all eight previously excluded cases (fixture diff analysis of the failing daily run showed the extra blockTimestamp key was the only difference in every case).

Checklist

• No Store schema change, so STORE_SCHEMA_VERSION is untouched.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

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.rs → unsync_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]

🤖 Kimi Code Review

Overall Assessment: The PR mechanically removes the blockTimestamp field from RPC log objects to resolve hive test incompatibilities. While the implementation is consistent, this creates a divergence from other major clients (geth, besu, reth, etc.) that populate this field.

Major Concerns

1. API Compatibility Break (Design Decision)

• File: crates/networking/rpc/types/receipt.rs (lines 121–129)
• Issue: Removing block_timestamp from RpcLog breaks compatibility with other execution clients. The deleted comments explicitly note that "every other client populates it" and that indexers rely on it to avoid "a separate block lookup per receipt."
• Impact: Users and indexers depending on this field will experience breakage. While the field is optional in the execution-apis schema, it is a de facto standard.
• Recommendation: Document this divergence in docs/ or comments. Consider making this configurable or clearly announce it as a breaking change in release notes.

2. Empty Array in CI Script

• File: .github/scripts/check-hive-results.sh (lines 62–63)
• Issue: Leaving KNOWN_EXCLUDED_TESTS=( empty rather than removing the variable entirely is slightly messy, though functionally harmless.
• Suggestion: Either remove the variable entirely (and the jq filter that uses it) or add a comment explaining it's reserved for future exclusions.

Code Quality

3. Test Coverage Gap

• File: crates/networking/rpc/types/receipt.rs (lines 309–353 deleted)
• Issue: The removal of block_timestamp_is_on_the_log_and_not_on_the_receipt eliminates test coverage for log field placement. While hive tests cover this indirectly, unit tests for serialization boundaries are valuable.
• Suggestion: Consider adding a test asserting that blockTimestamp is absent from logs, ensuring the field doesn't accidentally regress back (which would re-break hive tests).

4. Documentation Cleanup

• File: docs/known_issues.md
• Issue: The known issue is removed, which is correct if the hive tests now pass, but there's no documentation explaining why ethrex diverges from other clients by omitting the timestamp.
• Suggestion: Add a note to RPC documentation stating that blockTimestamp is intentionally omitted from log objects despite being present in other clients.

Correctness Verification

The changes are internally consistent:

• crates/networking/rpc/eth/block.rs:398 correctly removes the unused block_timestamp binding.
• crates/networking/rpc/eth/logs.rs:319 correctly removes the field from the log construction.
• crates/networking/rpc/types/receipt.rs:61 and :145 correctly update function signatures.
• Test expectations in receipt.rs are properly updated to match the new serialization format.

No security vulnerabilities or memory safety issues identified.

---

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

[github-actions]

🤖 Claude Code Review

That's fine — I've already verified consistency by reading the code directly; no need to force approval for a build. The static analysis is sufficient given how small and mechanical this diff is.

Review: PR 7234 — revert(l1): drop blockTimestamp from RPC log objects

This is a clean, mechanical revert of PR #7142, and it does what it says.

Correctness

• All call sites are consistent after the revert: RpcReceipt::new (receipt.rs:65, block.rs:432) and RpcLog::new/struct literal (logs.rs:308-317) no longer pass/accept block_timestamp, and no stale references remain anywhere in the tree (verified via grep).
• block.rs:397 still computes block_hash for receipt fetching — unrelated to the removed block_timestamp local, so no leftover unused variable there. Same check on logs.rs: block_header is still used for bloom matching and deriving block_num/block_hash, so removing its use for the timestamp doesn't leave a dead binding or trigger an unused-variable warning.
• The RpcLog struct drops the #[serde(with = "serde_utils::u64::hex_str", default)] field cleanly; no leftover derive/attribute artifacts.
• Test fixtures (receipt.rs) are updated to match the new (reverted) JSON shape, and the now-redundant block_timestamp_is_on_the_log_and_not_on_the_receipt test is removed along with its rationale comment — appropriate, since it specifically asserted the field's presence.
• docs/known_issues.md and KNOWN_EXCLUDED_TESTS are cleaned up together, so there's no dangling documentation referencing removed exclusions.

Consensus/RPC-spec considerations

• blockTimestamp on log objects is optional per execution-apis, so removing it is spec-compliant — not a breaking change for spec-conforming clients, just a divergence from what geth/besu/nethermind/reth/erigon populate (explicitly acknowledged as a trade-off in the PR description).
• No state-machine, gas, trie, or validation logic is touched — this is purely response-shaping code in the RPC layer, so there's no consensus-relevant risk here.

Process

• The revert is well-justified: hive's rpc-compat simulation does byte-exact comparison against a fixture corpus pinned before blockTimestamp existed, and the pin can't move forward without pulling in pre-merge chain fixtures ethrex doesn't support. Restoring 8/8 previously-excluded rpc-compat cases (all eth_getLogs coverage) is a reasonable near-term win over an optional convenience field.
• Checklist confirms no Store schema impact, consistent with the diff (no persisted-type changes).

No issues found — this looks safe to merge as a straightforward revert.

---

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

[github-actions]

🤖 Codex Code Review

1. High: blockTimestamp is being removed from every serialized RpcLog (crates/networking/rpc/types/receipt.rs:109-138, crates/networking/rpc/eth/logs.rs:312-320, and the receipt construction path in crates/networking/rpc/eth/block.rs:432-437). That changes the wire format of eth_getLogs, eth_getTransactionReceipt, and eth_getBlockReceipts. In this repo that field was added intentionally for client compatibility, and deleting the coverage test in the same patch means the regression would become silent. I would restore the field and the test unless there is a spec-backed reason to intentionally break RPC compatibility.

2. High: .github/scripts/check-hive-results.sh:62-63 drops all rpc-compat exclusions, but PR CI is still pinned to execution-apis commit d08382ae... in .github/workflows/pr-main_l1.yaml:309-311. The workflow comment there explicitly says that pin is required because newer rpc-compat fixtures switch to a pre-merge chain ethrex cannot import. With that pin unchanged, removing the exclusions looks like a deterministic CI break rather than a real fix. I would keep the exclusions until the simulator input changes in the same PR.

No EVM, gas-accounting, trie, RLP, or memory-safety concerns stood out in this diff; the main problems are RPC correctness/compatibility and CI behavior.

---

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

[github-actions]

Lines of code report

Total lines added: 0
Total lines removed: 61
Total lines changed: 61

Detailed view

+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/block.rs     | 410   | -2   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/eth/logs.rs      | 603   | -1   |
+-----------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/types/receipt.rs | 273   | -58  |
+-----------------------------------------------+-------+------+

[MegaRedHand]

LGTM. Let's revert this while we look for a workaround