-
Notifications
You must be signed in to change notification settings - Fork 309
Update logging for block policy errors #2571
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.
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
base: master
Are you sure you want to change the base?
Conversation
2dc9149 to
769a1f1
Compare
09aa093 to
a854d26
Compare
9cfb165 to
32a5e2f
Compare
ea50b59 to
b3f2732
Compare
32a5e2f to
a34c8b7
Compare
a34c8b7 to
235067c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors error handling and logging for block policy validation by introducing specific error types (EthBlockPolicyError and EthBlockPolicyBlockValidatorError) to replace the generic BlockPolicyError, improves error logging with additional context, and adds metrics tracking for specific error conditions.
Key changes:
- Introduces a dedicated error module (
monad-eth-block-policy/src/error.rs) with specific error types for Ethereum block policy validation - Enhances error logging by changing many
trace!calls todebug!and adding contextual information (block seq_num, round, transaction details) - Adds metrics tracking for execution lagging, bad state root, and base fee errors directly in the block policy layer
- Removes unnecessary
chain_configparameters fromupdate_committed_blockandresetmethods in theBlockPolicytrait - Updates
SystemTransactionErrorandSystemTransactionValidationErrorto derivePartialEq, Eqfor better error handling
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| monad-eth-block-policy/src/error.rs | New error module defining EthBlockPolicyError and EthBlockPolicyBlockValidatorError with appropriate From trait implementations |
| monad-eth-block-policy/src/lib.rs | Major refactoring: replaces BlockPolicyError with EthBlockPolicyError, updates logging from trace to debug with enhanced context, adds metrics tracking, removes phantom data field from validator |
| monad-consensus-types/src/block.rs | Removes generic BlockPolicyError types, adds associated BlockPolicyError type to BlockPolicy trait, simplifies method signatures, introduces PassthruBlockPolicyError |
| monad-system-calls/src/validator.rs | Adds PartialEq, Eq derives to error enums and includes transaction in debug logging |
| monad-state/src/lib.rs | Updates block_policy.reset() call to remove unnecessary chain_config parameter |
| monad-eth-txpool/src/pool/mod.rs | Updates error type references and removes ? from non-fallible validator constructor |
| monad-eth-txpool/tests/pool.rs | Removes unnecessary chain_config parameter from test code |
| monad-eth-txpool-executor/src/lib.rs | Updates block policy method calls to remove chain_config parameter |
| monad-updaters/src/txpool.rs | Updates block policy method calls to use simplified signatures |
| monad-consensus-state/src/lib.rs | Simplifies update_committed_block call by removing chain_config parameter |
| monad-blocktree/src/blocktree.rs | Simplifies error handling and adds debug logging for coherency check failures |
| monad-eth-block-validator/tests/coherency_test.rs | Adds Metrics::default() parameter to test to match updated trait signature |
| monad-blocktree/Cargo.toml | Adds tracing dependency for new debug logging |
| Cargo.lock | Reflects new tracing dependency for monad-blocktree |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Some(account_balance) = maybe_account_balance else { | ||
| warn!( | ||
| debug!( | ||
| seq_num =?self.block_seq_num, |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent formatting in debug macro. The format should be seq_num =? self.block_seq_num (with space after =?) to match the pattern used elsewhere in this file (e.g., lines 406, 422, 443, 461, 476). The same issue appears on lines 525, 541, 567, 582, and 598.
| ?is_emptying_transaction, | ||
| "Non-emptying txn can not be accepted insufficient reserve balance" | ||
| is_emptying_transaction, | ||
| "Non-emptying txn can not be accepted. insufficient reserve balance" |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error: "can not" should be "cannot" (one word).
| "Non-emptying txn can not be accepted. insufficient reserve balance" | |
| "Non-emptying txn cannot be accepted. insufficient reserve balance" |
| ?is_emptying_transaction, | ||
| "Non-emptying txn can not be accepted insufficient reserve balance" | ||
| is_emptying_transaction, | ||
| "Non-emptying txn can not be accepted. insufficient reserve balance" |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message lacks proper capitalization and punctuation: "insufficient reserve balance" should be "Insufficient reserve balance." or rephrase to "Txn validation failed: Non-emptying txn cannot be accepted due to insufficient reserve balance"
| "Non-emptying txn can not be accepted. insufficient reserve balance" | |
| "Txn validation failed: Non-emptying txn cannot be accepted due to insufficient reserve balance." |
| ?is_emptying_transaction, | ||
| "Emptying txn can not be accepted insufficient reserve balance" | ||
| is_emptying_transaction, | ||
| "Txn validation failed: Emptying txn can not be accepted insufficient reserve balance" |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message is misleading. It says "insufficient reserve balance" but the check is account_balance.balance < txn_max_gas (regular balance) and returns InsufficientBalance. The message should be: "Txn validation failed: Emptying txn cannot be accepted due to insufficient balance" (also fix "can not" → "cannot").
| "Txn validation failed: Emptying txn can not be accepted insufficient reserve balance" | |
| "Txn validation failed: Emptying txn cannot be accepted due to insufficient balance" |
|
|
||
| let Some(account_balance) = maybe_account_balance else { | ||
| warn!( | ||
| debug!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should be a warn - if this happens, it's likely there is a bug in our code (i.e. account balances for tx signers should always be populated)
No description provided.