refactor: standardize errors #611

Mirko-von-Leipzig · 2025-01-13T11:40:29Z

Updates errors to follow the guidelines in 0xPolygonMiden/miden-base#966.

This PR also removes the wildcard route option from the faucet. Currently the faucet allows visiting arbitrary paths (e.g. https://testnet.miden.io/asdas). Clearly this isn't behaving as intended and since we only have the single static file I've removed the wildcard entirely.

I looked into the axum defaults relating to #499 (comment) and decided the defaults of Error for failures, Debug for success are fine - though maybe we want to log the latter as info? I've also enabled the built-in axum rejects by setting axum::rejections=trace by default.

Closes #484, #499 and #581.

I do feel we could simplify our error handling strategy but that can wait for another day. In general we have many error variants, but we never actually do anything with them except bubble them up. Or put differently, aside from differentiating between internal/user-facing errors, we don't usually care about the errors.

Fixup graph errors

crates/utils/src/logging.rs

igamigo

Looks good overall, although I left a comment with a question. Basically not sure why we took out the #[from] and the error messages from the display messages.

Also, regarding this:

I looked into the axum defaults relating to #499 (comment) and decided the defaults of Error for failures, Debug for success are fine - though maybe we want to log the latter as info? I've also enabled the built-in axum rejects by setting axum::rejections=trace by default.

These defaults sound sensible to me

igamigo · 2025-01-13T20:59:25Z

crates/block-producer/src/errors.rs

+    #[error("failed to retrieve transaction inputs from the store")]
    StoreConnectionFailed(#[from] StoreError),


Should these (this and other occurrences) be annotated with #[source] instead? If #[from] is kept, we should probably include the inner error message. Seems like we would lose information on display otherwise, but maybe there's something I'm missing.

#[from] is a superset of #[source], quoting from the thiserror docs (emphasis mine):

The Error trait’s source() method is implemented to return whichever field has a #[source] attribute or is named source, if any. This is for identifying the underlying lower level error that caused your error.

The #[from] attribute always implies that the same field is #[source], so you don’t ever need to specify both attributes.

Any error type that implements std::error::Error or dereferences to dyn std::error::Error will work as a source.

This does mean that Display won't show the full backtrace; as described in the XOR Source section of the discussion in 0xPolygonMiden/miden-base#966.

We should go through all the "exit" routes of our tasks and main's to ensure they're doing the correct thing and returning a full error report.

igamigo · 2025-01-13T21:32:47Z

crates/store/src/errors.rs

    AccountsNotFoundInDb(Vec<AccountId>),
-    #[error("Account {0} is not on the chain")]
+    #[error("account {0} is not on the chain")]
    AccountNotOnChain(AccountId),


This should likely be renamed to AccountNotPublic

Thank you that makes much more sense :)

That said it doesn't appear to be used anywhere?

Fix default tracing level

bobbinth

Looks good! Thank you! This is not a super deep review from my end, but upon a quick scan, I didn't see any issues.

I do feel we could simplify our error handling strategy but that can wait for another day. In general we have many error variants, but we never actually do anything with them except bubble them up. Or put differently, aside from differentiating between internal/user-facing errors, we don't usually care about the errors.

This could be a good idea, but I'd keep for another PR. Let's create an issue for it?

PhilippGackstatter

Looks good to me. Thanks for working on this! If I understand correctly, the goal of this PR was to make errors fall in line with the guidelines and it does that so I will approve.

I left comments about where I think errors could be more concrete or add more context in their messages by refactoring them. Generally, I'm not a fan of using #[from] as it usually leads to generic error variants which cover multiple concrete cases, which, imho, should have their own concrete variants to ultimately result in better errors. I think #[from] is appropriate in places like ConversionError, where no more context can be added by introducing concrete variants so having to map_err is actually unnecessary.

This is also a matter of opinion whether applying my suggestions is necessary, so feel free to apply or not apply what you think makes sense for this codebase.

PhilippGackstatter · 2025-01-14T06:54:42Z

bin/faucet/src/handlers.rs

@@ -56,7 +56,7 @@ pub async fn get_tokens(

    // Check that the amount is in the asset amount options
    if !state.config.asset_amount_options.contains(&req.asset_amount) {
-        return Err(HandlerError::BadRequest("Invalid asset amount".to_string()));
+        return Err(HandlerError::BadRequest("invalid asset amount".to_string()));


Suggested change

return Err(HandlerError::BadRequest("invalid asset amount".to_string()));

return Err(HandlerError::BadRequest(format!(

"requested asset amount {} is not in list of allowed request amounts {:?}",

req.asset_amount, state.config.asset_amount_options

)));

Maybe this would be more helpful.

PhilippGackstatter · 2025-01-14T07:03:53Z

crates/block-producer/src/errors.rs

+    #[error("transaction input error")]
    TransactionInputError(#[from] TransactionInputError),


This variant is unused currently, can we remove it?

PhilippGackstatter · 2025-01-14T07:06:53Z

crates/block-producer/src/errors.rs

@@ -80,19 +82,19 @@ pub enum VerifyTxError {

 #[derive(Debug, Error)]
 pub enum AddTransactionError {
-    #[error("Transaction verification failed: {0}")]
+    #[error("transaction verification failed")]


Suggested change

#[error("transaction verification failed")]

#[error("to-be-added transaction failed verification")]

Nit: Could this be more specific, i.e. make it clearer which transaction failed (the one to be added)?

PhilippGackstatter · 2025-01-14T07:12:58Z

crates/block-producer/src/errors.rs

    VerificationFailed(#[from] VerifyTxError),

-    #[error("Transaction input data is stale. Required data from {stale_limit} or newer, but inputs are from {input_block}.")]
+    #[error("transaction input data is stale. Required data from {stale_limit} or newer, but inputs are from {input_block}")]


I think we should probably also avoid . within error messages (and add this to the guidelines?) and first try to make a single sentence out of it, and if that's not possible, use ; instead.

So maybe something like:

Suggested change

#[error("transaction input data is stale. Required data from {stale_limit} or newer, but inputs are from {input_block}")]

#[error("transaction input data from block height {input_block} is stale because the oldest allowed block height is {stale_limit}")]

PhilippGackstatter · 2025-01-14T07:14:13Z

crates/block-producer/src/errors.rs

+    #[error("deserialization failed: {0}")]
    DeserializationError(String),


We should be able to make the inner error #[source] DeserializationError directly instead of String (and then not include it in the error message).

This variant is only used for ProvenTransaction deserialization. Can we then make this ProvenTransactionDeserializationFailed(#[source] DeserializationError) and adjust the error message accordingly? Alternatively just TransactionDeserializationFailed if we think it should be more generic. But I think having just DeserializationError is too generic because it adds too little context.

PhilippGackstatter · 2025-01-14T08:23:42Z

crates/store/src/errors.rs

@@ -40,71 +40,65 @@ pub enum NullifierTreeError {
 pub enum DatabaseError {


I think this error should be refactored to not have generic variants for each sub error, because they add very little context. Similar to a previous comment about the NullifierTree, if we apply a delta to an account and it fails, we would maybe get "account error: storage slot index is 10 but the slots length is 5", which could be more helpful by making it "failed to apply account delta to account 0x123: storage slot index is 10 but the slots length is 5". Especially if this error is part of a larger error chain, this intermediate context is important for locating what actually went wrong.

Not for this PR, but a larger question is whether this error should contain all of these variants in the first place (see also "kitchen-sink enum" in https://matklad.github.io/2020/10/15/study-of-std-io-error.html). I don't have a perfect solution for this, but perhaps we could think about whether DB connection errors like MissingDbConnection or SqliteError should be part of this error or if they are actually unrecoverable and should lead to a panic at the call site. But I don't know a lot about how the node works, so consider this just a question and not a request.

PhilippGackstatter · 2025-01-14T08:38:23Z

crates/store/src/errors.rs

 pub enum DatabaseSetupError {
-    #[error("I/O error: {0}")]
+    #[error("I/O error")]
    IoError(#[from] io::Error),
-    #[error("Database error: {0}")]
+    #[error("database error")]
    DatabaseError(#[from] DatabaseError),
-    #[error("Genesis block error: {0}")]
+    #[error("genesis block error")]
    GenesisBlockError(#[from] GenesisError),
-    #[error("Pool build error: {0}")]
+    #[error("pool build error")]
    PoolBuildError(#[from] deadpool_sqlite::BuildError),
-    #[error("SQLite migration error: {0}")]
+    #[error("SQLite migration error")]
    SqliteMigrationError(#[from] rusqlite_migration::Error),
 }

 #[derive(Debug, Error)]
 pub enum GenesisError {
    // ERRORS WITH AUTOMATIC CONVERSIONS FROM NESTED ERROR TYPES
    // ---------------------------------------------------------------------------------------------
-    #[error("Database error: {0}")]
+    #[error("database error")]
    DatabaseError(#[from] DatabaseError),
-    #[error("Block error: {0}")]
+    #[error("block error")]
    BlockError(#[from] BlockError),
-    #[error("Merkle error: {0}")]
+    #[error("merkle error")]
    MerkleError(#[from] MerkleError),


Might be worthwhile to check if any of these could be made more concrete as well.

PhilippGackstatter · 2025-01-14T08:39:58Z

crates/store/src/errors.rs

-    #[error("Apply block failed: {0}")]
+    #[error("apply block failed: {0}")]
    ApplyBlockFailed(String),


Should we make this a #[source] error instead and make the error message say "genesis block"?

PhilippGackstatter · 2025-01-14T08:40:56Z

crates/store/src/errors.rs

+    #[error("failed to read genesis file \"{genesis_filepath}\": {error}")]
    FailedToReadGenesisFile {
        genesis_filepath: String,
        error: io::Error,
    },


Suggested change

#[error("failed to read genesis file \"{genesis_filepath}\": {error}")]

FailedToReadGenesisFile {

genesis_filepath: String,

error: io::Error,

},

#[error("failed to read genesis file \"{genesis_filepath}\"")]

FailedToReadGenesisFile {

genesis_filepath: String,

source: io::Error,

},

PhilippGackstatter · 2025-01-14T08:44:00Z

crates/store/src/errors.rs

+    #[error("invalid tx hash: expected {expected}, but got {actual}")]
    InvalidTxHash { expected: RpoDigest, actual: RpoDigest },


Suggested change

#[error("invalid tx hash: expected {expected}, but got {actual}")]

InvalidTxHash { expected: RpoDigest, actual: RpoDigest },

#[error("invalid block tx hash: expected {expected}, but got {actual}")]

InvalidBlockTxHash { expected: RpoDigest, actual: RpoDigest },

Nit: This seems to be specific to the block tx hash.

Mirko-von-Leipzig added 6 commits January 13, 2025 12:41

Fix faucet errors

1de2857

Fixup store errors

f8b1758

Fixup error utils

f91fecb

Fixup block producer errors

4c0cbff

Fixup graph errors

fb76ef8

Log axum rejections

ca03226

Fixup graph errors

Mirko-von-Leipzig added the no changelog label Jan 13, 2025

Mirko-von-Leipzig requested a review from PhilippGackstatter January 13, 2025 11:40

Mirko-von-Leipzig commented Jan 13, 2025

View reviewed changes

crates/utils/src/logging.rs Outdated Show resolved Hide resolved

bobbinth requested a review from igamigo January 13, 2025 17:15

igamigo reviewed Jan 13, 2025

View reviewed changes

Mirko-von-Leipzig added 2 commits January 14, 2025 08:46

Fix default tracing level

802657f

Rename AccountNotOnChain to AccountNotPublic

7e84e37

Fix default tracing level

Mirko-von-Leipzig force-pushed the mirko/feat/standardize-errors branch from 4fa6f06 to 7e84e37 Compare January 14, 2025 06:47

bobbinth approved these changes Jan 14, 2025

View reviewed changes

PhilippGackstatter approved these changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: standardize errors #611

refactor: standardize errors #611

Mirko-von-Leipzig commented Jan 13, 2025 •

edited

Loading

igamigo left a comment

igamigo Jan 13, 2025

Mirko-von-Leipzig Jan 14, 2025

Mirko-von-Leipzig Jan 14, 2025

igamigo Jan 13, 2025

Mirko-von-Leipzig Jan 14, 2025

Mirko-von-Leipzig Jan 14, 2025

bobbinth left a comment

PhilippGackstatter left a comment

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

PhilippGackstatter Jan 14, 2025

		#[error("failed to retrieve transaction inputs from the store")]
		StoreConnectionFailed(#[from] StoreError),

-        return Err(HandlerError::BadRequest("invalid asset amount".to_string()));
+        return Err(HandlerError::BadRequest(format!(
+            "requested asset amount {} is not in list of allowed request amounts {:?}",
+            req.asset_amount, state.config.asset_amount_options
+        )));

		#[error("transaction input error")]
		TransactionInputError(#[from] TransactionInputError),

	#[error("transaction verification failed")]
	#[error("to-be-added transaction failed verification")]

	#[error("transaction input data is stale. Required data from {stale_limit} or newer, but inputs are from {input_block}")]
	#[error("transaction input data from block height {input_block} is stale because the oldest allowed block height is {stale_limit}")]

		#[error("deserialization failed: {0}")]
		DeserializationError(String),

		@@ -40,71 +40,65 @@ pub enum NullifierTreeError {
		pub enum DatabaseError {

		#[error("invalid tx hash: expected {expected}, but got {actual}")]
		InvalidTxHash { expected: RpoDigest, actual: RpoDigest },

refactor: standardize errors #611

Are you sure you want to change the base?

refactor: standardize errors #611

Conversation

Mirko-von-Leipzig commented Jan 13, 2025 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mirko-von-Leipzig commented Jan 13, 2025 •

edited

Loading