Skip to content

feat(beacon): log a Glamsterdam banner after the first gloas block - #666

Open
MegaRedHand wants to merge 3 commits into
feat/beacon-gloas-livefrom
feat/beacon-glamsterdam-banner
Open

MegaRedHand wants to merge 3 commits into
feat/beacon-gloas-livefrom
feat/beacon-glamsterdam-banner

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Logs a Glamsterdam banner when the fork actually goes live, following lambdaclass/ethrex#7341:

  • Trigger: the import of the chain's first gloas block, meaning the first block gloas applies to while it did not apply to its parent. Missed slots at the start of the fork epoch still leave a first block. It fires on every import route (gossip, range sync, a block this node's validator client published), right after the block's own Block imported successfully line.
  • Once per process: the decision settles on the first gloas block the process imports, banner or not. Blocks import parent-first, so a node that sees the chain's first gloas block sees it before any other gloas block. A node started or checkpoint-synced after the fork logs nothing, and it does not pay a whole-block parent decode on every later import to learn that again.
  • Art: a polar bear in the startup logo's style, over ethlambda's wordmark and Glamsterdam's, in crates/blockchain/assets/glamsterdam_banner.txt. Like the logo it uses block characters, but it has no ASCII fallback: a terminal that cannot decode UTF-8 shows it garbled. Each line is logged separately, so every line carries the log prefix.
    ▄                                          ░
                           ▀
            ▀                                          ▄
  ░                                    ▄
                                    ▄▄▄▄▄▄▄▄▄▄
                        ▄▄▄▄▄▄▄████████████████▄
                 ▄▄  ▄████████████████████████████▀
             ▄▄███████████████████████████████████
          ▄███▄███████████████████████████████████
          ▀▀▀▀▀▀█████████████████████████████████▀
                    ██████████▀▀▀▀▀▀▀▀▀▀████████▀
                    ██████ ░░░░   ░░░░   ▀██████
                    █████  ░░░░   ░░░░    ▀████
                    █████  ░░░░   ░░░░     ████▄
                   ▄█████▄ ░░░░░ ░░░░░    ▄█████▄
                  ▀▀▀▀▀▀▀▀               ▀▀▀▀▀▀▀▀
▄███████▀▀▀▀█▀▀▀█▀██▀█▀█████▀▀██▀███▀█▀▀▀██▀▀▀███▀▀█████████▄
████████ ▀▀███ ██ ▀▀ █ ████ ▀▀ █ ▄▀▄ █ ▀▀▄█ ██ █ ▀▀ █████████
████████ ▀▀▀██ ██ ██ █ ▀▀▀█ ██ █ ███ █ ▀▀▄█ ▀▀▄█ ██ █████████
████▀▀▀█▀█████▀▀██▀███▀██▀▀▀█▀▀▀█▀▀▀▀█▀▀▀██▀▀▀███▀▀██▀███▀███
███ █▀▀█ ████ ▀▀ █ ▄▀▄ █▄▀▀███ ██ ▀▀██ ▀▀▄█ ██ █ ▀▀ █ ▄▀▄ ███
███▄▀▀ █ ▀▀▀█ ██ █ ███ █▀▀▀▄██ ██ ▀▀▀█ █▄▀█ ▀▀▄█ ██ █ ███ ███
  ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀

It is followed by a structured line: Glamsterdam is live: imported the first gloas block slot=… epoch=… block_root=….

Verification

  • glamsterdam::tests (7 tests: crossing the fork, missed slots, pre-fork blocks neither log nor settle, once per process, node started after the fork, parent block not on record, no gloas scheduled) all pass.
  • cargo test -p ethlambda-blockchain --profile release-fast --lib --bins: 243 passed. make lint is clean.
  • Not run on a devnet across a fulu to gloas boundary.

Marks the fork in the node's own log at the moment it goes live, as
ethrex does for amsterdam (lambdaclass/ethrex#7341). The banner follows
the import of the chain's first gloas block: the first block gloas
applies to while it did not apply to its parent, so missed slots at the
start of the fork epoch still leave one.

The decision settles on the first gloas block this process imports,
banner or not. Blocks import parent-first, so a node that sees the
chain's first gloas block sees it before any other, and a node started
after the fork would otherwise pay a whole-block parent decode on every
import for nothing.

The art is a placeholder: a polar bear in sunglasses over the wordmark,
plain ASCII so it needs no locale fallback.
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR for the Glamsterdam banner feature, focusing on correctness, security, performance, and Rust best practices.

Overall Assessment

This is a well-crafted, low-risk feature. The code is correct, safe, and thoughtfully optimized. The lazy evaluation of parent_slot to avoid expensive SSZ decoding is particularly well-designed.


Detailed Review

crates/blockchain/src/glamsterdam.rs

Line 33-34: pending field visibility

pub(crate) struct GlamsterdamBanner {
    pending: bool,
}

Good: Field is private, enforcing state machine invariants through methods only.

Lines 52-58: on_block_imported — chain check ordering

if !self.pending || store.chain() != Chain::Beacon {
    return;
}
let parent_slot = || store.block_entry(&parent_root).map(|(slot, _)| slot);

Excellent: The store.chain() != Chain::Beacon check prevents the closure from being constructed for non-beacon chains, avoiding unnecessary allocation. The lazy closure is only created when needed.

Lines 68-77: take_first_gloas_block — state machine correctness

fn take_first_gloas_block(
    &mut self,
    config: &Config,
    slot: u64,
    parent_slot: impl FnOnce() -> Option<u64>,
) -> bool {
    if !self.pending || !is_gloas(config, slot) {
        return false;
    }
    self.pending = false;
    parent_slot().is_some_and(|parent_slot| !is_gloas(config, parent_slot))
}

Correct and elegant. The state is settled ( pending = false ) in all paths after the initial check. The is_some_and usage is idiomatic Rust.

One subtle point: if parent_slot() panics, self.pending is already false, so the banner will never log on retry. This is correct behavior—better to miss the banner than double-log it.

Lines 80-84: is_gloas fork detection

fn is_gloas(config: &Config, slot: u64) -> bool {
    config
        .fork_at_epoch(compute_epoch_at_slot(slot))
        .has_payload_envelopes()
}
**Question:** This checks `has_payload_envelopes()` rather than a specific fork name. Is this the canonical way to detect "Gloas or later"? 

If a future fork removes payload envelopes, this would break. Consider documenting this assumption or using a more explicit fork version check if the intent is strictly "Gloas and later forks."

#### Lines 88-100: `log` function
```rust
fn log(slot: u64, block_root: H256) {
    info!("");
    for line in BANNER.lines() {
        info!("{line}");
    }
    info!("");

The empty info!("") calls will emit log lines with just the prefix. This is intentional for visual separation, but verify your log formatter handles empty messages gracefully (some formatters may omit them or render awkwardly).

Consider: info!("\n{}\n", BANNER) would be more efficient (single log record), but you explicitly want per-line prefixes. The current approach is correct for your stated goal.

Line 97: block_root = %ShortRoot(&block_root.0)

Good: Uses ShortRoot for compact logging rather than full 64-char hex.


crates/blockchain/src/lib.rs

Lines 572-575: Field documentation

/// Whether the Glamsterdam banner may still be logged. Beacon-only; see
/// [`crate::glamsterdam`].
glamsterdam_banner: crate::glamsterdam::GlamsterdamBanner,

Minor: The doc comment says "Whether..." suggesting a bool, but the type is GlamsterdamBanner. Suggest: /// Tracks whether the Glamsterdam banner may still be logged...

Lines 2852-2858: Integration point

self.glamsterdam_banner.on_block_imported(
    &self.store,
    slot,
    block_root,
    parent_root,
);

Correct placement: After the "Block imported successfully" log, so the banner follows as documented.


Tests (glamsterdam.rs lines 116-176)

Comprehensive and well-named. Covering:

  1. Basic fork crossing
  2. Missed slots at fork boundary
  3. Pre-fork blocks don't settle
  4. Idempotency (once-only logging)
  5. Post-fork startup (no banner)
  6. Missing parent (no banner)
  7. No scheduled fork (no banner)

Minor gap: No test for Chain::Lean path (the early return in on_block_imported). This is implicitly covered by integration tests, but a unit test would be complete.


Performance

  • Memory: GlamsterdamBanner is 1 byte (bool), negligible overhead per BlockChainServer.
  • CPU: The lazy parent_slot closure avoids SSZ decoding except for the one block that might trigger the banner. After that, pending == false makes subsequent checks O(1).
  • I/O: Banner string is include_str! at compile time, no runtime file access.

Security

No security concerns. This is purely cosmetic logging with no consensus impact. The feature correctly cannot affect state transition or fork choice.


Suggestions (All Minor)

Priority Location Suggestion
Low glamsterdam.rs:82 Document why has_payload_envelopes() is the proxy for Gloas detection
Low lib.rs:572 Rephrase doc comment to match type (GlamsterdamBanner, not bool)
Very Low glamsterdam.rs:88 Consider tracing::info!(target: "glamsterdam", ...) for filterable logs

Conclusion

Approve with minor nits. The code is correct, safe, performant, and well-tested. The lazy evaluation pattern and state machine design are exemplary. Fix the doc comment wording if convenient; otherwise merge as-is.


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

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 666: Glamsterdam banner

Overall this is clean and small, and it is well covered by tests. The "settle on the first gloas block" design keeps the per-import cost to a single bool check after the first gloas block. I found no correctness or security problems, and the change doesn't touch consensus logic. I did not build or run the tests.

Minor points, none blocking:

  1. Sync replay (glamsterdam.rs, log): A node that is syncing from before the fork, for example range-syncing a Sepolia or Platåberget data directory, will log "Glamsterdam is live" when it replays the fork-crossing block. That can be well after the fork actually happened. If that matters, gate on the block being near the wall-clock slot, or reword to "imported the first gloas block". The current wording is acceptable for a banner.

  2. Competing forks: The banner settles on the first gloas block this process imports. If that block is on a branch that is later orphaned, the banner is spent on it. The the_banner_is_logged_once test documents this. It is cosmetic, but a one-line note in the GlamsterdamBanner docs would make the choice explicit.

  3. Cost of the parent lookup: store.block_entry(&parent_root) decodes the whole parent block. That happens once, since pending is cleared before the call, so the cost is fine. The ordering is intentional and correct: it is cleared before parent_slot() runs, so a missing parent can't cause a retry on every import.

  4. Test gap: Every test goes through take_first_gloas_block. on_block_imported itself (the Chain::Beacon gate and the store.config() wiring) is untested. That is acceptable given the thin wrapper, but a lean-store no-op test would be cheap if a harness for it already exists.

  5. Style: The file matches the surrounding conventions, including tracing shorthand, ShortRoot, and the doc comments. Logging line by line so each line gets the formatter prefix is a sensible choice. info!("") for spacing is fine.

LGTM.


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

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  • Overall this looks small, isolated, and non-consensus-critical. I don’t see issues in fork choice, attestation processing, justification/finalization, state transition, XMSS, or SSZ paths because this PR only adds post-import logging state.

  • The lazy parent lookup in crates/blockchain/src/glamsterdam.rs:51 is a good performance choice; it avoids a beacon block decode on every import and only pays once when a Gloas-era block is first seen.

  • Correctness: crates/blockchain/src/glamsterdam.rs:72 clears pending before proving the imported block is actually the chain’s first Gloas block. If store.block_entry(&parent_root) returns None transiently or for an anchor/import edge case, the banner is permanently suppressed. The comment says blocks import parent-first, but this function is being used on the hot path after import, so I’d rather make the state transition robust to missing-parent metadata. Suggest only settling to false when you intentionally want “started-after-fork / checkpoint-sync-past-fork” semantics, and otherwise keep pending until you can distinguish that case reliably.

  • Maintainability: the detection logic is easy to follow, but the naming is a bit confusing: the module/file is glamsterdam, while the comments and logic are about the first gloas block, e.g. crates/blockchain/src/glamsterdam.rs:1, crates/blockchain/src/glamsterdam.rs:57, crates/blockchain/src/glamsterdam.rs:77. If “Glamsterdam” is the banner name and “Gloas” is the fork trigger, that’s fine, but a short doc comment explicitly connecting the two would reduce reader confusion.

  • Testing: the unit coverage is solid for the helper, especially missed-slot and once-only behavior in crates/blockchain/src/glamsterdam.rs:119 and crates/blockchain/src/glamsterdam.rs:146. One missing case is an integration-style test around the real on_block_imported path with a beacon store, since the main subtlety here is the dependency on Store::block_entry behavior from crates/storage/src/store.rs:2497.

  • No obvious security or memory-safety issues. The new code is read-only over store state, uses no unsafe code, and failure mode is limited to logging behavior rather than consensus state mutation.


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

Replaces the placeholder art with a polar bear in the startup logo's
style: block characters over a wordmark plate like the logo's own. The
banner is no longer plain ASCII, so a terminal that cannot decode UTF-8
shows it garbled, as it would the logo without its ASCII fallback.
The polar bear now stands over both plates, ethlambda's above
Glamsterdam's, so the banner says which client crossed the fork.
MegaRedHand added a commit that referenced this pull request Oct 5, 2026
…3-636-638-gloas-live

One conflict, in `BlockChainServer::for_replay`'s struct literal: tmp
moved `store` last because `ChainEvents::new` borrows it first. Kept
both, with `glamsterdam_banner` before `store`.
@MegaRedHand MegaRedHand added the beacon Ethereum Beacon Chain client label Oct 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beacon Ethereum Beacon Chain client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant