chore(l1): review peer handler and misc logs #5150

fedacking · 2025-10-31T21:34:19Z

Motivation

We are in the process of downgrading logs that are useless to the user to debug and moving the format to structured logging from tracing.

Description

Reviewed peer_handler logs, downgrading and structuring them
Reviewed transaction broadcast logs
Made the current allocator log debug
Made gas estimator error debug (they are expected after snap sync)
Made rocksdb not print column families in info (useless info for the user)

Progress for #4950

github-actions · 2025-10-31T21:37:01Z

Lines of code report

Total lines added: 27
Total lines removed: 0
Total lines changed: 27

Detailed view

+----------------------------------------------+-------+------+
| File                                         | Lines | Diff |
+----------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_handler.rs | 1789  | +26  |
+----------------------------------------------+-------+------+
| ethrex/crates/storage/store_db/rocksdb.rs    | 1541  | +1   |
+----------------------------------------------+-------+------+

ElFantasma · 2025-10-31T21:48:06Z

crates/networking/p2p/peer_handler.rs

-                    "Sync Log 5: Requesting block headers from peer {peer_id}, chunk_limit: {chunk_limit}"
+                    %peer_id,
+                    %chunk_limit,
+                    "request_block_headers: Requesting block headers"


Isn't the message a bit redundant?

ElFantasma · 2025-10-31T21:52:31Z

crates/networking/p2p/peer_handler.rs

-                        "We had an error dumping the last accounts to disk {}",
-                        err.error
+                        error=%err.error,
+                        "We had an error dumping the last accounts to disk",


Suggested change

"We had an error dumping the last accounts to disk",

"Failed to dump the last accounts to disk",

ElFantasma · 2025-10-31T21:54:19Z

crates/networking/p2p/peer_handler.rs

                        .expect("Shouldn't have a join error")
                        .inspect_err(|err| {
-                            error!("We found this error while dumping to file {err:?}")
+                            error!(error=?err.error, "We found this error while dumping to file")


Suggested change

error!(error=?err.error, "We found this error while dumping to file")

error!(error=?err.error, "Error dumping to file")

ElFantasma · 2025-10-31T21:55:42Z

crates/networking/p2p/peer_handler.rs

-                result
-                    .inspect_err(|err| error!("We found this error while dumping to file {err:?}"))
+                result.inspect_err(
+                    |err| error!(error=?err.error, "We found this error while dumping to file"),


Suggested change

|err| error!(error=?err.error, "We found this error while dumping to file"),

|err| error!(error=?err.error, "Error dumping to file"),

ElFantasma · 2025-10-31T21:57:49Z

crates/networking/p2p/tx_broadcaster.rs

            });
            connection.outgoing_message(txs_message).await.unwrap_or_else(|err| {
-                error!(peer_id = %format!("{:#x}", peer_id), err = ?err, "Failed to send transactions");
+                debug!(peer_id = %format!("{:#x}", peer_id), err = ?err, "Failed to send transactions");


I think it's ok to have this on error: If it fails the broadcaster is broken and user should be aware.

ElFantasma · 2025-10-31T21:58:01Z

crates/networking/p2p/tx_broadcaster.rs

            );
            connection.outgoing_message(hashes_message.clone()).await.unwrap_or_else(|err| {
-                error!(peer_id = %format!("{:#x}", peer_id), err = ?err, "Failed to send transactions hashes");
+                debug!(peer_id = %format!("{:#x}", peer_id), err = ?err, "Failed to send transactions hashes");


ElFantasma · 2025-10-31T21:58:07Z

crates/networking/p2p/tx_broadcaster.rs


                let _ = self.broadcast_txs().await.inspect_err(|_| {
-                    error!("Failed to broadcast transactions");
+                    debug!("Failed to broadcast transactions");


ElFantasma

In general it LGTM.

Left some minor comments and suggestions

mpaulucci · 2025-11-03T17:09:59Z

crates/networking/p2p/peer_handler.rs

        let sync_head_number_retrieval_start = SystemTime::now();

-        info!("Retrieving sync head block number from peers");
+        debug!("Retrieving sync head block number from peers");


I believe having one log when the requesting start isn't bad, but I'll check if there aren't anothers.

mpaulucci · 2025-11-03T17:10:12Z

crates/networking/p2p/peer_handler.rs

            .unwrap_or_default();

-        info!("Sync head block number retrieved");
+        debug!("Sync head block number retrieved");


mpaulucci · 2025-11-03T17:11:35Z

crates/networking/p2p/peer_handler.rs

-                                "[SYNCING] Received empty/invalid headers from peer, penalizing peer {peer_id}"
+                            debug!(
+                                %peer_id,
+                                "[SYNCING] Received empty/invalid headers from peer, penalizing peer"


I would remove the [SYNCING], or in general every [FOO]

mpaulucci · 2025-11-03T17:11:43Z

crates/networking/p2p/peer_handler.rs

        chunk_limit: u64,
    ) -> Result<Vec<BlockHeader>, PeerHandlerError> {
-        debug!("Requesting block headers from peer {peer_id}");
+        debug!(%peer_id, "Requesting block headers from peer");


mpaulucci · 2025-11-03T17:12:07Z

crates/networking/p2p/peer_handler.rs

        state_root: H256,
        tx: tokio::sync::mpsc::Sender<(Vec<AccountRangeUnit>, H256, Option<(H256, H256)>)>,
    ) -> Result<(), PeerHandlerError> {
        debug!(


mpaulucci · 2025-11-03T17:12:21Z

crates/networking/p2p/peer_handler.rs

                    remaining_start: chunk_start,
                    remaining_end: chunk_end,
                };
                debug!(


mpaulucci · 2025-11-03T17:16:34Z

crates/networking/p2p/peer_handler.rs

                                    task_count += 1;
                                }
-                                debug!("Split big storage account into {chunk_count} chunks.");
+                                debug!(chunk_count, "Split big storage account into chunks.");


probably trace? Or is this exceptional?

fedacking added 7 commits October 31, 2025 12:37

Update tx_broadcaster.rs

169223d

Update tx_broadcaster.rs

5529416

global allocator

63a79aa

Merge remote-tracking branch 'origin/main' into chore/review-logs

8ff3264

Fixed gas estimator tip

d1680a1

Improved Peer Handler Logs

562340b

Update rocksdb.rs

ee042b6

fedacking requested a review from a team as a code owner October 31, 2025 21:34

fedacking self-assigned this Oct 31, 2025

github-actions bot added the L1 Ethereum client label Oct 31, 2025

github-project-automation bot added this to ethrex_l1 Oct 31, 2025

fedacking mentioned this pull request Oct 31, 2025

L1: Improve logs and downgrade unimportant logs #4950

Open

22 tasks

ElFantasma reviewed Oct 31, 2025

View reviewed changes

ElFantasma approved these changes Oct 31, 2025

View reviewed changes

Merge branch 'main' into chore/review-logs

8c01bc5

mpaulucci reviewed Nov 3, 2025

View reviewed changes

crates/networking/p2p/peer_handler.rs

remaining_start: chunk_start,

remaining_end: chunk_end,

};

debug!(

Copy link

Collaborator

mpaulucci Nov 3, 2025

Choose a reason for hiding this comment

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

trace

mpaulucci reviewed Nov 3, 2025

View reviewed changes

	"We had an error dumping the last accounts to disk",
	"Failed to dump the last accounts to disk",

	error!(error=?err.error, "We found this error while dumping to file")
	error!(error=?err.error, "Error dumping to file")

	\|err\| error!(error=?err.error, "We found this error while dumping to file"),
	\|err\| error!(error=?err.error, "Error dumping to file"),

chore(l1): review peer handler and misc logs #5150

Are you sure you want to change the base?

chore(l1): review peer handler and misc logs #5150

Conversation

fedacking commented Oct 31, 2025

Uh oh!

github-actions bot commented Oct 31, 2025

Lines of code report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElFantasma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ElFantasma left a comment •

edited

Loading