Skip to content

Improve error handling and log error context#15

Open
dangeross wants to merge 1 commit into
RCasatta:masterfrom
breez:savage-breez-fix-error-handling
Open

Improve error handling and log error context#15
dangeross wants to merge 1 commit into
RCasatta:masterfrom
breez:savage-breez-fix-error-handling

Conversation

@dangeross
Copy link
Copy Markdown
Contributor

We've observed an error in the blocks thread causing the blocks updates to stall because the index fn exits:

May 15 04:04:17 elements waterfalls[691968]: [2025-05-15T02:04:17Z INFO  waterfalls::server::route] returning: 6 elements, elapsed: 0ms
May 15 04:04:17 elements waterfalls[691968]: [2025-05-15T02:04:17Z INFO  waterfalls::threads::mempool] removed txs from mempool [c49a7e07945e75b1fbd9149fdbf1fab72901633815893ac665e3b749f2588ef3, 34a7ae0465af3d5c0a1b0b412f9b4ca9d3d509d89735b44da3c9f35fa100f4f0, 99f5914195b91091e86f6c22ec4d08725eb6a03b07d835431974f78c13919935], tip: Some(3375085)
May 15 04:04:17 elements waterfalls[691968]: [2025-05-15T02:04:17Z ERROR waterfalls::threads::blocks] Other

This PR:

  • Updates the index fn to log the error source
  • Calls set_hash_ts fn only after successfully calling db.update fn
  • Updates blocks_infallible fn to handle the error in a loop
  • Adds a shutdown handler to gracefully exit the blocks_infallible loop

@RCasatta
Copy link
Copy Markdown
Owner

That seems a systemd error log, note that you can set RUST_LOG_STYLE=systemd to have a much nicer output (no double date and also log levels such that for example journalct -p 4 -u waterfalls shows only warning and errors

I am reviewing this PR now, everything seems good but having the commit splitted like in your description would have been better

@RCasatta
Copy link
Copy Markdown
Owner

And sorry for the terrible Other error

Comment thread src/threads/blocks.rs
db.update(&meta, utxo_spent, history_map, utxo_created)
.inspect_err(|e| log::error!("Error updating store: {e:?}"))
.map_err(|_| Error::Other)?; // TODO
state.set_hash_ts(&meta).await;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it seems it's called internally in update() also, should we remove it from here?

Comment thread src/threads/blocks.rs
let meta = BlockMeta::new(block_height, block.block_hash(), block.header.time);
state.set_hash_ts(&meta).await;
db.update(&meta, utxo_spent, history_map, utxo_created)
.inspect_err(|e| log::error!("Error updating store: {e:?}"))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

should we use with_context like in other places?

Comment thread src/threads/blocks.rs
pub(crate) async fn blocks_infallible(
shared_state: Arc<State>,
client: Client,
shutdown_signal: Receiver<()>,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

do we need this also in the mempool thread? (not needed in this MR just wondering)

@RCasatta
Copy link
Copy Markdown
Owner

Do you discover what was the specific cause of the initial Other error?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants