Skip to content

feat: improve log messages #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MegaRedHand merged 16 commits into from
Jan 29, 2026
Merged

feat: improve log messages #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d3b108c
log: add ShortRoot display helper for truncated hashes
MegaRedHand Jan 29, 2026
5fd3464
log: add full context to block reception from gossip
MegaRedHand Jan 29, 2026
45da221
log: add block import success and context to failures
MegaRedHand Jan 29, 2026
8d4923f
log: add finalization event logging
MegaRedHand Jan 29, 2026
2f852ff
log: add fork choice head update and justification event logging
MegaRedHand Jan 29, 2026
b4f49fe
log: add attestation processing with target/source
MegaRedHand Jan 29, 2026
2b609bc
log: enhance attestation reception with target/source
MegaRedHand Jan 29, 2026
4a42fea
log: add block and attestation publication details
MegaRedHand Jan 29, 2026
e8da495
log: add peer count to connection logs
MegaRedHand Jan 29, 2026
2dabc22
fix: add tracing dependency and use div_ceil for clippy
MegaRedHand Jan 29, 2026
19f6064
fix: standardize field order in block logs
MegaRedHand Jan 29, 2026
e8b3f10
fix: add target_root and source_root to attestation publication log
MegaRedHand Jan 29, 2026
23695d6
Apply suggestion from @greptile-apps[bot]
MegaRedHand Jan 29, 2026
1809149
refactor: simplify log field syntax using tracing shorthands
MegaRedHand Jan 29, 2026
b87a06a
chore: fmt
MegaRedHand Jan 29, 2026
a6fee9d
docs: add logging patterns to CLAUDE.md
MegaRedHand Jan 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,51 @@ rm -rf leanSpec && make leanSpec/fixtures # Regenerate test fixtures (require
let _timing = metrics::time_state_transition();
```

### Logging Patterns

**Use tracing shorthand syntax for cleaner logs:**
```rust
// ✅ GOOD: Shorthand for simple variables
let slot = block.slot;
let proposer = block.proposer_index;
info!(
%slot, // Shorthand for slot = %slot (Display)
proposer, // Shorthand for proposer = proposer
block_root = %ShortRoot(&block_root.0), // Named expression
"Block imported"
);

// ❌ BAD: Verbose
info!(
slot = %slot,
proposer = proposer,
...
);
```

**Standardized field ordering (temporal → identity → identifiers → context → metadata):**
```rust
// Block logs
info!(%slot, proposer, block_root = ..., parent_root = ..., attestation_count, "...");

// Attestation logs
info!(%slot, validator, target_slot, target_root = ..., source_slot, source_root = ..., "...");

// Consensus events
info!(finalized_slot, finalized_root = ..., previous_finalized, justified_slot, "...");

// Peer events
info!(%peer_id, %direction, peer_count, our_finalized_slot, our_head_slot, "...");
```

**Root hash truncation:**
```rust
use ethlambda_types::ShortRoot;

// Always use ShortRoot for consistent 8-char display (4 bytes)
info!(block_root = %ShortRoot(&root.0), "...");
```

### Relative Indexing (justified_slots)
```rust
// Bounded storage: index relative to finalized_slot
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 17 additions & 2 deletions crates/blockchain/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use ethlambda_state_transition::is_proposer;
use ethlambda_storage::Store;
use ethlambda_types::primitives::H256;
use ethlambda_types::{
ShortRoot,
attestation::{Attestation, AttestationData, SignedAttestation},
block::{BlockSignatures, BlockWithAttestation, SignedBlockWithAttestation},
primitives::TreeHash,
Expand Down Expand Up @@ -277,6 +278,7 @@ impl BlockChainServer {
let slot = signed_block.message.block.slot;
let block_root = signed_block.message.block.tree_hash_root();
let parent_root = signed_block.message.block.parent_root;
let proposer = signed_block.message.block.proposer_index;

Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Here is the review for the code snippet provided:

  1. Import Ordering: The Rust community generally prefers to order imports alphabetically within each import group for consistency and readability. The recent change disrupts this ordering for ShortRoot. Line imports should look like: 
    use ethlambda_types::{
    ShortRoot,
    attestation::{Attestation, AttestationData, SignedAttestation},
    block::{BlockSignatures, BlockWithAttestation, SignedBlockWithAttestation},
    primitives::TreeHash,
    signature::ValidatorSecretKey,
    state::Checkpoint,
};
  2. Security Considerations: Without full function implementations or context, it is not possible to ensure that security best practices are followed. It's essential to ensure robust validation of blocks and attestations, especially boundary and authorization checks.
  3. Potential Bugs and Error Handling: Make sure each function and interaction, especially network or IO tasks, have appropriate error handling. Rust’s Result and Option types should be leveraged.
  4. Fork Choice Correctness: Ensure that changes do not negatively impact the fork choice rule. If this code impacts that logic, double-check against the specification.
  5. Code Readability: Although the import modifications are minor, maintaining alphabetical order contributes to readability, especially as more imports are added.
  6. Memory Safety: Rust provides guarantees for memory safety, but ensure that there are no usages of unsafe blocks unless absolutely necessary. Also, use borrowing appropriately to avoid unnecessary copies.
  7. Performance Implications: Anytime concurrency is involved, as in the usage of spawned_concurrency::tasks, evaluate the potential for performance bottlenecks, especially in a consensus-critical system.

To proceed, I suggest reordering the imports back to an alphabetically sorted list and ensure comprehensive test coverage for all consensus-critical operations discussed above. Additional context or code would allow for a deeper review.

// Check if parent block exists before attempting to process
if !self.store.contains_block(&parent_root) {
Expand All @@ -296,13 +298,26 @@ impl BlockChainServer {
// Parent exists, proceed with processing
match self.process_block(signed_block) {
Ok(_) => {
info!(%slot, "Block processed successfully");
info!(
%slot,
proposer,
block_root = %ShortRoot(&block_root.0),
parent_root = %ShortRoot(&parent_root.0),
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Correctness and Potential Bugs

  • Ensure that the self.process_block(signed_block) function returns comprehensive error types that are thoroughly checked and handled. It's crucial in the context of blockchain clients to have detailed error handling for each operation.

Security Vulnerabilities

  • The logging of %err might expose sensitive error information which could potentially be used to exploit vulnerabilities. Make sure the error messages do not leak any sensitive or exploitable information.

Performance Implications

  • The changes you made do not introduce any new performance concerns. Ensure that logging does not cause performance degradation during high-throughput scenarios.

Rust Best Practices and Idiomatic Patterns

  • The change from slot = %slot to %slot is more idiomatic and concise. Well done.

Memory Safety and Proper Error Handling

  • The usage of match statements for error handling is idiomatic in Rust. However, ensure the error paths are fully tested as part of the module's test suite to catch potential uncaught exceptions.

Code Readability and Maintainability

  • The updated usage of logging macros is more readable and aligns with Rust best practices, which improves maintainability.

Consensus-layer Considerations

  • Ensure that self.process_block integrates properly with your fork choice rule and that it is correctly processing and validating attestations. Incorrect implementations can lead to consensus failure.
  • Verify compliance with the state transition function requirements of the Ethereum protocol, including slot processing and block processing logic correctness.

Overall, the PR needs further verification and explanations about how errors are handled and secured before it can be merged.

"Block imported successfully"
);

// Check if any pending blocks can now be processed
self.process_pending_children(block_root);
}
Err(err) => {
warn!(%slot, %err, "Failed to process block");
warn!(
%slot,
proposer,
block_root = %ShortRoot(&block_root.0),
parent_root = %ShortRoot(&parent_root.0),
%err,
"Failed to process block"
);
}
}
}
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Correctness and Potential Bugs

  • The new variable proposer is correctly referenced and logged. Ensure that this field is properly initialized and validated as part of block validation outside the snippet.

Security Vulnerabilities

  • No immediate security vulnerabilities are apparent from this diff. However, consider if logging detailed block information poses any security/privacy risks (e.g., if logs are accessible to unauthorized actors).

Performance Implications

  • Logging additional data such as proposer, block_root, and parent_root will have a minor impact on performance, especially if the logging subsystem is not optimized for concurrent access or if logging is done at a very high throughput.

Rust Best Practices and Idiomatic Patterns

  • The expansion of the info! and warn! macros is idiomatic and follows Rust's convention for structured logging.

Memory Safety and Proper Error Handling

  • The current mechanism for logging errors is appropriate; however, ensure that err includes sufficient context about the error for better debugging.
  • It appears error handling on process_block is limited to logging. Ensure that in the case of an error, other system components remain consistent and secure.

Code Readability and Maintainability

  • The change improves logging detail, which aids in debugging and auditing; avoid over-logging to prevent log flooding.

Consensus-layer Considerations

  • Fork Choice (LMD GHOST / 3SF-mini): Ensure that process_block accurately updates fork choice rules, as this is critical for consensus.
  • Attestation Processing and Validation: This snippet does not include attestations, but ensure the block's inclusion respects consensus rules.
  • Justification and Finalization Logic: Critical for client consensus accuracy, make sure that justification/finalization logic outside this snippet is thorough and includes verification steps.
  • State Transition Functions (process_slots, process_block): Validate these functions for precise state updates.
  • XMSS Signature Verification and Aggregation: No changes observed; correctness depends on implementation outside this snippet.
  • SSZ Encoding/Decoding Correctness: Non-applicable for the visible diff; ensure correctness outside this snippet if applicable.

Overall, the additions concerning logging enhance observability, but attention to the broader consensus and performance impacts is essential. Consider also scrutinizing upstream and downstream code interactions not visible in this change.

Expand Down
33 changes: 32 additions & 1 deletion crates/blockchain/src/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use ethlambda_state_transition::{
};
use ethlambda_storage::{ForkCheckpoints, SignatureKey, Store};
use ethlambda_types::{
ShortRoot,
attestation::{AggregatedAttestation, Attestation, AttestationData, SignedAttestation},
block::{
AggregatedAttestations, AggregatedSignatureProof, AggregationBits, Block, BlockBody,
Expand Down Expand Up @@ -43,6 +44,22 @@ fn update_head(store: &mut Store) {
info!(%old_head, %new_head, "Fork choice reorg detected");
}
store.update_checkpoints(ForkCheckpoints::head_only(new_head));

if old_head != new_head {
let old_slot = store.get_block(&old_head).map(|b| b.slot).unwrap_or(0);
let new_slot = store.get_block(&new_head).map(|b| b.slot).unwrap_or(0);
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Review for ethlambda PR

  1. Imports Organization:

    • The import of ShortRoot was moved. Rust groups imports alphabetically within the same module bracket. Consistency in import ordering can improve readability. Consider organizing imports alphabetically to maintain consistency.

  2. Code Correctness & Potential Bugs:

    • Without additional context on the specific changes in function implementation, it's crucial that all consensus-related functions, such as process_slots, process_block, etc., strictly follow the specifications. Any deviation or incorrect logic could lead to consensus failure.

  3. Security Vulnerabilities:

    • Ensure that any cryptographic dependency, especially concerning XMSS signature verification and aggregation, is up to date and has undergone rigorous security audits.
    • Validate inputs rigorously, especially in attestation processing, to prevent overflow attacks or invalid state transitions.

  4. Performance Implications:

    • If there are changes involving loops or recursive calls, verify the computational complexity and ensure that they operate efficiently with Ethereum's expected load.
    • Benchmark any critical path changes for performance regressions.

  5. Rust Best Practices:

    • Ensure all operations that can fail are handled using Rust’s idiomatic error handling. The Result and Option types should be used extensively, and the ? operator should be checked for potential misuse.

  6. Memory Safety & Error Handling:

    • Ensure that all pointers (e.g., if using raw pointers or unsafe code) are handled with utmost care to avoid memory leaks or undefined behavior.
    • Validate that lifetimes are correctly used and there are no unnecessary clones that could lead to performance overhead.

  7. Code Readability & Maintainability:

    • Comment critical sections of the code, especially where fork choice logic or state transitions are implemented, to enhance clarity for future maintainers.

  8. Consensus-layer Considerations:

    • Ensure that fork choice rule implementation (LMD GHOST / 3SF-mini) aligns with the Ethereum specification.
    • Attestation handling should account for all edge cases, ensuring that they are both timely and correctly incorporated into the fork choice.
    • Review any alterations in state transition functions (e.g., process_slots, process_block) to guarantee correctness in state updates.
    • Validate SSZ encoding/decoding—ensure it handles the Ethereum consensus requirements fully and correctly.

Conclusion

There are concerns around ensuring consistent import order, strict correctness in state management and processing logic, and a strong emphasis on security practices. It's recommended to address these considerations before proceeding with the merge.

let justified_slot = store.latest_justified().slot;
let finalized_slot = store.latest_finalized().slot;
info!(
head_slot = new_slot,
head_root = %ShortRoot(&new_head.0),
previous_head_slot = old_slot,
previous_head_root = %ShortRoot(&old_head.0),
justified_slot,
finalized_slot,
"Fork choice head updated"
);
}
}

/// Update the safe target for attestation.
Expand Down Expand Up @@ -196,7 +213,7 @@ pub fn on_gossip_attestation(
return Err(StoreError::SignatureVerificationFailed);
}
}
on_attestation(store, attestation, false)?;
on_attestation(store, attestation.clone(), false)?;
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Correctness and Potential Bugs

  • Line 50-51: The use of unwrap_or(0) can potentially mask bugs if get_block unexpectedly returns None. It's more prudent to handle this case explicitly, perhaps with error logging, to avoid silently continuing with potentially incorrect data. Consider replacing with expect() for clearer diagnostics.

Security Vulnerabilities

  • No immediate security vulnerabilities are evident in this diff snippet. However, ensure that state access functions like get_block perform appropriate validation and error handling especially given the critical nature of consensus data.

Performance Implications

  • Lines 48-54: Fetching justified_slot and finalized_slot separately might introduce slight overhead especially in high-frequency operations. If these values are frequently accessed together, consider batching them in a single database query.

Rust Best Practices and Idiomatic Patterns

  • Usage of bindings like justified_slot and finalized_slot is more idiomatic and improves readability. Good improvement here.

Memory Safety and Proper Error Handling

  • As previously mentioned, be cautious with silent fallbacks like unwrap_or(0). Prefer clearer error propagation to prevent masking possible bugs.

Code Readability and Maintainability

  • The introduction of local bindings for computed values (e.g., justified_slot, slot) improves readability by reducing direct field accesses and inline computations within the info! macro. This makes the code easier to read and maintain.

Consensus-layer Considerations

  • Ensure that the logic handling forks and head updates withstands the LMD GHOST / 3SF-mini algorithms, maintaining correctness and adhering to specifications.
  • Always double-check attestation handling, as these contribute to potential forks. Make sure there is adequate testing to handle boundary and edge cases.

Overall, the code shows improvements by adopting more idiomatic practices but requires attention on error handling to ensure robustness in critical systems like blockchain clients.


if cfg!(not(feature = "skip-signature-verification")) {
// Store signature for later lookup during block building
Expand All @@ -206,6 +223,20 @@ pub fn on_gossip_attestation(
store.insert_gossip_signature(signature_key, signature);
}
metrics::inc_attestations_valid("gossip");

let slot = attestation.data.slot;
let target_slot = attestation.data.target.slot;
let source_slot = attestation.data.source.slot;
info!(
slot,
validator = validator_id,
target_slot,
target_root = %ShortRoot(&attestation.data.target.root.0),
source_slot,
source_root = %ShortRoot(&attestation.data.source.root.0),
"Attestation processed"
);

Ok(())
}

Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

  • Code Correctness and Potential Bugs:

    • The change in line 211 to clone the attestation before passing it to on_attestation prevents borrowing issues but potentially introduces unnecessary clone operations if not required, which could impact performance.

  • Security Vulnerabilities:

    • Nothing is immediately evident from this diff, but it is crucial to ensure that any input used in printf! style logging, especially from network sources, is sanitized to avoid security risks.

  • Performance Implications:

    • Cloning attestation on line 211 can lead to performance issues, especially in high-load scenarios. Consider refactoring on_attestation to either take a reference or handle copying internally when necessary.

  • Rust Best Practices and Idiomatic Patterns:

    • The use of unwrap_or(0) in old_slot and new_slot calculations could be reconsidered for better error handling to differentiate between a missing block and a genuine zero slot, if applicable for this consensus model.

  • Memory Safety and Proper Error Handling:

    • Line 211 could utilize as_ref or as_mut based on what on_attestation requires to avoid excessive cloning.
    • Ensuring safeguard against unwraps spilling panics to production code paths by using better error contextual error handling mechanisms.

  • Code Readability and Maintainability:

    • Logging structured information on line 44 to 57 is comprehensive and improves traceability. However, the same must be extended to error situations for holistic debugging.

  • Consensus-layer Considerations:

    • Fork choice logging is thorough but ensure that key state updates respect atomicity throughout the function's transactional scope.
    • No issues with attestation logging are identified, but continuously validate against consensus invariants to avoid deciding on stale state representations.

Overall, the proposed changes should consider minimizing unnecessary object cloning, closely managing slot errors, and improving debug logging for error cases.

Expand Down
1 change: 1 addition & 0 deletions crates/blockchain/state_transition/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ethlambda-types.workspace = true
ethlambda-metrics.workspace = true

thiserror.workspace = true
tracing.workspace = true

[dev-dependencies]
serde.workspace = true
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

General Overview

The addition of a new dependency, tracing, in the workspace is a good move for improving observability and debugging, which is crucial in maintaining a consensus client.

Specific Concerns

1. Code Correctness & Potential Bugs

There is no direct code to review in the provided snippet, but adding tracing indicates possible changes elsewhere for logging. Ensure that all such changes have appropriate log levels set (e.g., trace, debug, info, warn, error) to avoid excessive logging, which can obscure critical information.

2. Security Vulnerabilities

While tracing itself does not introduce vulnerabilities, increased logging can inadvertently expose sensitive information if not properly managed. Double-check logging statements to ensure no private keys, raw transaction data, or sensitive state information are logged.

3. Performance Implications

Implementing tracing can introduce performance overhead, especially if used excessively in high-traffic code paths. Consider using conditional compilation flags to disable verbose logging in production builds.

4. Rust Best Practices & Idiomatic Patterns

Adding tracing is idiomatic to Rust's asynchronous environment. Ensure #[instrument] macros are used appropriately on functions that benefit from detailed execution context.

5. Memory Safety & Proper Error Handling

tracing does not directly affect memory safety unless incorrect log handling somehow disrupts execution flow. Ensure that log-related function calls do not swallow errors that need proper propagation.

6. Code Readability & Maintainability

Ensure that logging improves, rather than complicates, code readability. Use structured logging to maintain consistent, parseable logs that contribute to maintainability.

Consensus-layer Considerations

This addition doesn't directly affect consensus logic but can support diagnosing issues in fork choice, attestation processing, and state transitions by reproducing internal states or changes.

Actionable Suggestions

  • Review Log Levels: Audit where tracing is used and assign appropriate levels.
  • Sensitive Data: Ensure that no sensitive data is inadvertently logged.
  • Performance-Test Logging: Benchmark code with logging enabled to understand performance impact.

In conclusion, while the addition of tracing can be beneficial for the client's debugging and operational visibility, caution must be taken around what is logged, to safeguard performance and security.

Expand Down
23 changes: 23 additions & 0 deletions crates/blockchain/state_transition/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use std::collections::HashMap;

use ethlambda_types::{
ShortRoot,
block::{AggregatedAttestations, Block, BlockHeader},
primitives::{H256, TreeHash},
state::{Checkpoint, JustificationValidators, State},
};
use tracing::info;

mod justified_slots_ops;
pub mod metrics;
Expand Down Expand Up @@ -296,6 +298,16 @@ fn process_attestations(
target.slot,
);

let justified_slot = target.slot;
let threshold = (2 * validator_count).div_ceil(3);
info!(
justified_slot,
justified_root = %ShortRoot(&target.root.0),
vote_count,
threshold,
"Checkpoint justified"
);

justifications.remove(&target.root);

// Consider whether finalization can advance.
Expand All @@ -307,6 +319,17 @@ fn process_attestations(
state.latest_finalized = source;
metrics::inc_finalizations("success");

let finalized_slot = source.slot;
let previous_finalized = old_finalized_slot;
let justified_slot = state.latest_justified.slot;
info!(
finalized_slot,
finalized_root = %ShortRoot(&source.root.0),
previous_finalized,
justified_slot,
"Checkpoint finalized"
);

// Shift window to drop finalized slots from the front
let delta = (state.latest_finalized.slot - old_finalized_slot) as usize;
justified_slots_ops::shift_window(&mut state.justified_slots, delta);
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Correctness and Potential Bugs

  1. Functionality: The added logging (info!) provides useful information about checkpoint justification and finalization. However, it is crucial to ensure that logging does not impact performance or expose sensitive information. Ensure these logs are not too verbose in production environments, as they might lead to log overflow or increased I/O operations which can slow down the system.

Security Vulnerabilities

  1. Logging Sensitive Data: Consider if the justified_root and finalized_root should be treated as sensitive data. If so, logging them could expose sensitive information in log files. Proper encryption or redaction might be necessary.

Performance Implications

  1. Logging Overhead: The overhead from the newly introduced info! logging statements should be minimal, especially in a consensus client where performance is critical. Ensure that in high-frequency scenarios, this logging does not become a bottleneck.

Rust Best Practices and Idiomatic Patterns

  1. Imports Organization: The addition of ShortRoot and tracing::info is correctly following Rust's best practices for imports. Keeping imports well-organized helps maintain readability and efficiency.

Memory Safety and Error Handling

  1. Error Handling: There is no data validation before logging. Make sure that target.slot, target.root, and other params are always expected values before logging to avoid propagating invalid state information.

Code Readability and Maintainability

  1. Informative Logging: The added info! logs improve the traceability of justification and finalization logic, making it easier to diagnose issues in these critical consensus mechanisms.

  2. Code Formatting: The formatting follows Rust conventions and is clean, which aids maintainability.

Additional Notes

  • Consensus Layer Considerations: Without seeing the remainder of the consensus logic, especially around fork choice and attestation validation, it's crucial to ensure that these logs accurately reflect state transitions according to the Ethereum Beacon Chain specification.

  • Overall, the logging-related change looks like a positive addition in terms of transparency, provided that performance and security are managed appropriately.

Expand Down
12 changes: 12 additions & 0 deletions crates/common/types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,15 @@ pub mod genesis;
pub mod primitives;
pub mod signature;
pub mod state;

/// Display helper for truncated root hashes (8 hex chars)
pub struct ShortRoot<'a>(pub &'a [u8; 32]);

impl std::fmt::Display for ShortRoot<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
for byte in &self.0[..4] {
write!(f, "{:02x}", byte)?;
}
Ok(())
}
}
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

General Feedback:

The proposed code appears to introduce a utility for displaying a shortened version of Ethereum root hashes. While the implementation is straightforward, there are a few areas that could benefit from improvement to adhere to Rust best practices and ensure future maintainability.

Code Correctness and Potential Bugs:

  • fmt::Display Implementation: The fmt function iterates over the first 4 bytes of a 32-byte array but does not handle unexpected array lengths. Ensure the input is always of length 32, or implement error handling to provide feedback when the assumption is invalid.

Security Vulnerabilities:

  • Security Considerations: Given this code pertains to display formatting, there are no direct security implications. However, be vigilant about inputs to this struct in other parts of the codebase.

Performance Implications:

  • Iteration over Bytes: The performance cost of iterating over 4 bytes is negligible in this context.

Rust Best Practices and Idiomatic Patterns:

  • Explicit Lifetimes: The use of an explicit lifetime 'a in ShortRoot<'a> is appropriate and clear.
  • Destructuring: Consider pattern matching to destructure or slice arrays directly if you're expecting precisely 32 elements.

Memory Safety and Proper Error Handling:

  • Error Handling: The write! macro uses the ? operator, which is idiomatic. Ensure calling contexts are prepared to handle potential fmt::Error results or panics.

Code Readability and Maintainability:

  • Code Documentation: Expand the doc comment to explain the use case for ShortRoot and its role or importance in the broader application context.
  • Naming: Consider renaming ShortRoot to something more self-explanatory, like TruncatedRootDisplay if its primary utility is format representation.

Consensus-layer Considerations:

  • The PR provided does not directly interface with consensus logic such as fork choice, attestation validation, or state transition functions. Ensure that inputs to this display helper are validated elsewhere.

Conclusion:

While the implementation is acceptable, addressing the identified concerns will enhance the overall robustness and maintainability of the code. Specifically, adding error handling for unexpected input sizes will prevent potential issues when used within the broader codebase. Assuming inputs are always valid can lead to future bugs when the codebase evolves.

53 changes: 48 additions & 5 deletions crates/net/p2p/src/gossipsub/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use ethlambda_types::{attestation::SignedAttestation, block::SignedBlockWithAttestation};
use ethlambda_types::{
ShortRoot, attestation::SignedAttestation, block::SignedBlockWithAttestation,
};
use libp2p::gossipsub::Event;
use ssz::{Decode, Encode};
use tracing::{error, info, trace};
use tree_hash::TreeHash;

use super::{
encoding::{compress_message, decompress_message},
Expand Down Expand Up @@ -32,7 +35,18 @@ pub async fn handle_gossipsub_message(server: &mut P2PServer, event: Event) {
return;
};
let slot = signed_block.message.block.slot;
info!(%slot, "Received new block from gossipsub, sending for processing");
let block_root = signed_block.message.block.tree_hash_root();
let proposer = signed_block.message.block.proposer_index;
let parent_root = signed_block.message.block.parent_root;
let attestation_count = signed_block.message.block.body.attestations.len();
info!(
%slot,
proposer,
block_root = %ShortRoot(&block_root.0),
parent_root = %ShortRoot(&parent_root.0),
attestation_count,
"Received block from gossip"
);
server.blockchain.notify_new_block(signed_block).await;
}
Some(ATTESTATION_TOPIC_KIND) => {
Expand All @@ -49,7 +63,16 @@ pub async fn handle_gossipsub_message(server: &mut P2PServer, event: Event) {
};
let slot = signed_attestation.message.slot;
let validator = signed_attestation.validator_id;
info!(%slot, %validator, "Received new attestation from gossipsub, sending for processing");
info!(
%slot,
validator,
head_root = %ShortRoot(&signed_attestation.message.head.root.0),
target_slot = signed_attestation.message.target.slot,
target_root = %ShortRoot(&signed_attestation.message.target.root.0),
source_slot = signed_attestation.message.source.slot,
source_root = %ShortRoot(&signed_attestation.message.source.root.0),
"Received attestation from gossip"
);
server
.blockchain
.notify_new_attestation(signed_attestation)
Expand Down Expand Up @@ -77,7 +100,15 @@ pub async fn publish_attestation(server: &mut P2PServer, attestation: SignedAtte
.behaviour_mut()
.gossipsub
.publish(server.attestation_topic.clone(), compressed)
.inspect(|_| trace!(%slot, %validator, "Published attestation to gossipsub"))
.inspect(|_| info!(
%slot,
validator,
target_slot = attestation.message.target.slot,
target_root = %ShortRoot(&attestation.message.target.root.0),
source_slot = attestation.message.source.slot,
source_root = %ShortRoot(&attestation.message.source.root.0),
"Published attestation to gossipsub"
))
.inspect_err(|err| {
tracing::warn!(%slot, %validator, %err, "Failed to publish attestation to gossipsub")
});
Expand All @@ -86,6 +117,9 @@ pub async fn publish_attestation(server: &mut P2PServer, attestation: SignedAtte
pub async fn publish_block(server: &mut P2PServer, signed_block: SignedBlockWithAttestation) {
let slot = signed_block.message.block.slot;
let proposer = signed_block.message.block.proposer_index;
let block_root = signed_block.message.block.tree_hash_root();
let parent_root = signed_block.message.block.parent_root;
let attestation_count = signed_block.message.block.body.attestations.len();

// Encode to SSZ
let ssz_bytes = signed_block.as_ssz_bytes();
Expand All @@ -99,7 +133,16 @@ pub async fn publish_block(server: &mut P2PServer, signed_block: SignedBlockWith
.behaviour_mut()
.gossipsub
.publish(server.block_topic.clone(), compressed)
.inspect(|_| info!(%slot, %proposer, "Published block to gossipsub"))
.inspect(|_| {
info!(
%slot,
proposer,
block_root = %ShortRoot(&block_root.0),
parent_root = %ShortRoot(&parent_root.0),
attestation_count,
"Published block to gossipsub"
)
})
.inspect_err(
|err| tracing::warn!(%slot, %proposer, %err, "Failed to publish block to gossipsub"),
);
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Code Correctness and Potential Bugs:

  1. The use of ShortRoot seems appropriate but ensure ShortRoot correctly implements the necessary traits and handles the byte slice safely as printing raw bytes could lead to unexpected behaviors if not handled properly.

Security Vulnerabilities:
2. Ensure all incoming data is validated and sanitized, though this is more verbose logging and doesn't change the core of consensus logic, being vigilant with these additions is critical in blockchain. The logging could inadvertently expose sensitive info, so ensure ShortRoot only logs what is necessary.
3. In Rust, race conditions and data consistency issues should be considered. Ensure logging doesn't block any critical paths, given that these logs occur async, consideration for thread safety around blockchain.notify_new_block and blockchain.notify_new_attestation is important.

Performance Implications:
4. Using info! level logging can potentially produce a large amount of log data, this can be significant in a production environment leading to IO bottlenecks. Consider using lower verbosity levels unless necessary during normal operation.

Rust Best Practices and Idiomatic Patterns:
5. The code generally adheres to Rust's idioms, using async functions and idiomatic error handling is good practice.

Memory Safety and Proper Error Handling:
6. inspect and inspect_err are used correctly to handle success/failure cases. Ensure that any error in publishing doesn't impact state transitions/code paths negatively.

Code Readability and Maintainability:
7. Code readability is generally good, but the additional use of ShortRoot for log statements should have corresponding documentation to explain this abstraction's purpose and behavior.
8. Consider consistent logging patterns throughout the codebase, keeping format and verbosity.

Consensus-Layer Considerations:
9. No apparent logic related to fork choice, attestation processing, or justification has been altered. However, ensure that the log information properly reflects the current state without assumptions. Additional matches or guards could be included to catch edge cases not otherwise handled.
10. No changes to process_slots or process_block functions are apparent. Ensure existing logic comprehensively covers state transitions, especially when new logging or side-effects are introduced.

Overall, while the changes mostly affect logging and do not directly modify core consensus functions, thoroughly testing these additions in an integration or simulation environment is recommended to ensure they do not inadvertently introduce race conditions or reveal any sensitive operational data.

Expand Down
23 changes: 21 additions & 2 deletions crates/net/p2p/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,20 @@ async fn handle_swarm_event(server: &mut P2PServer, event: SwarmEvent<BehaviourE
let direction = connection_direction(&endpoint);
if num_established.get() == 1 {
server.connected_peers.insert(peer_id);
let peer_count = server.connected_peers.len();
metrics::notify_peer_connected(&Some(peer_id), direction, "success");
// Send status request on first connection to this peer
let our_status = build_status(&server.store);
info!(%peer_id, %direction, finalized_slot=%our_status.finalized.slot, head_slot=%our_status.head.slot, "Added connection to new peer, sending status request");
let our_finalized_slot = our_status.finalized.slot;
let our_head_slot = our_status.head.slot;
info!(
%peer_id,
%direction,
peer_count,
our_finalized_slot,
our_head_slot,
"Peer connected"
);
server
.swarm
.behaviour_mut()
Expand Down Expand Up @@ -290,9 +300,18 @@ async fn handle_swarm_event(server: &mut P2PServer, event: SwarmEvent<BehaviourE
};
if num_established == 0 {
server.connected_peers.remove(&peer_id);
let peer_count = server.connected_peers.len();
metrics::notify_peer_disconnected(&Some(peer_id), direction, reason);
info!(
%peer_id,
%direction,
%reason,
peer_count,
"Peer disconnected"
);
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

General

  • Code Correctness and Potential Bugs: While the changes seem safe regarding their immediate functionality, ensure that our_finalized_slot and our_head_slot are correctly derived values and used elsewhere in the code if necessary. This prevents potential divergence between the consensus view and stored state.

  • Security Vulnerabilities: No immediate issues spotted with the current diff, but ensure peer_id, direction, and reason are securely handled across the codebase to avoid spoofing or logging sensitive information.

Performance Implications

  • The extraction of our_finalized_slot and our_head_slot into separate variables is likely negligible in terms of performance but might slightly improve the logging efficiency by avoiding repeatedly evaluating our_status.*.slot.

Rust Best Practices and Idiomatic Patterns

  • Your updated logging syntax is more idiomatic by using short form for pattern matching with structures, (%peer_id vs peer_id = %peer_id), which is good for readability and maintainability.

Memory Safety and Proper Error Handling

  • Ensure that all interactions with server.store through build_status(&server.store) correctly handle error cases, although not directly affected by this change, generally important for memory safety.

Code Readability and Maintainability

  • The revised logging code is more concise and improves readability. However, make sure that these logging messages remain consistent across the entire codebase to maintain clarity.

Consensus-Layer Considerations

  • Fork Choice Correctness: Without more context on how the gathered slots (our_finalized_slot, our_head_slot) are used, ensure they are accurate representations from the consensus layer's fork choice.

  • Attestation Processing and Validation: Verify that peers' status provides consistent information relevant to attestation data requirements.

  • Justification and Finalization Logic: The logging improvements facilitate better monitoring, which aids quick response to forks or consensus inconsistencies.

  • State Transition Functions: Not directly impacted by the logging changes, but ensure consistency with how slots are logged in state transitions.

  • XMSS and SSZ Concerns: This diff doesn't affect signature verification or encoding logic.

In conclusion, the logging improvements are beneficial, though ensure holistic correctness and security throughout the codebase. Further context-specific analysis would solidify the assurance levels of these changes.

} else {
info!(%peer_id, %direction, %reason, "Peer connection closed but other connections remain");
}
info!(%peer_id, %direction, %reason, "Peer disconnected");
}
SwarmEvent::OutgoingConnectionError { peer_id, error, .. } => {
let result = if error.to_string().to_lowercase().contains("timed out") {
Copy link

@github-actions github-actions bot Jan 29, 2026

Choose a reason for hiding this comment

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

Review Comments

Code Correctness and Potential Bugs

  • Line 246-262: The change from our_status to their_*_slot in the info! macro seems incorrect based on the context of sending our status to the peer. The log should reflect our_finalized_slot and our_head_slot instead of their_* unless there's a specific reason for this change.

Security Vulnerabilities

  • No critical security vulnerabilities identified in this part of code. However, ensure that peer information and error messages do not leak sensitive information that could be exploited.

Performance Implications

  • Line 249: Usage of server.connected_peers.len() on every connection and disconnection has constant time complexity since connected_peers is a HashSet. This is efficient but avoid unnecessary calls if the log line isn’t needed.

Rust Best Practices and Idiomatic Patterns

  • Line 252 & 306: Consider avoiding the to_string() method for error handling; prefer borrowing if the error is already a String, or use error.to_lowercase().contains("") directly if the lowercase conversion is necessary.

Memory Safety and Proper Error Handling

  • Line 246-315: Ensure that all unwrap() and expect() calls are handled properly to avoid panics in unexpected scenarios, though this context seems safe from visible panic causes.

Code Readability and Maintainability

  • Lines 252-254 and 297-300: The info! logs are informative but could be improved by maintaining consistency in the log message formatting for better readability and debugging.

Consistency and Fault Tolerance

  • Fork choice correctness, attestation processing, etc.: This snippet doesn't directly relate to these consensus-critical components, but ensure any changes adhere to consensus specifications in related modules.

In summary, the main concern revolves around log message correctness and the unnecessary use of to_string().to_lowercase(). The logging changes are mostly positive for insight, but caution is needed to ensure accurate information is logged.

Expand Down
Loading