Much quieter replays #1713
Much quieter replays #1713
Conversation
|
Test failures on no-freeze-file builds are caused by new hashable, which changes the hash values. Updating the freeze file and |
This reverts commit 166adac.
|
Reverting in case this causes a problem. |
|
Arrg, updating |
| @@ -1155,7 +1155,7 @@ fatal e = do | |||
| rk <- view txRequestKey | |||
|
|
|||
| logError_ l | |||
| $! "critical transaction failure: " | |||
| $ "critical transaction failure: " | |||
There was a problem hiding this comment.
What's the reason for undoing the bangs
There was a problem hiding this comment.
Loggers are always strict, so the bangs aren't necessary... except that they're lazy when they're filtered such that the log message never makes it to the queue. I've seen some pretty big log messages here, so I'd like those not constructed in case that can raise throughput.
There was a problem hiding this comment.
yeah, log messages are deep forced before added to the queue.
Avoiding construction when their log level is disabled is a good point.
| -- Validate hashes if not doing a replay. no need to re-validate hashes | ||
| -- during a replay, because these blocks have already been validated | ||
| view psReplaying >>= \x -> unless x $ |
There was a problem hiding this comment.
Is this correct? During replay we need to compare the payload hash from the pact validation with the payload hash that is stored in the block header.
I guess the best would be to always check. IIRC it means to do one additional rocksdb lookup (which most likely is cached already). During replay we have to do it anyways. And in normal consensus operation the overhead would be minimal. So, it should be fine.
There was a problem hiding this comment.
I'm probably just confused here. I'll just make it always check.
| withSilentLoggerForReplays f = do | ||
| replaying <- view psReplaying | ||
| logger <- view psLogger | ||
| if replaying then L.withLoggerLevel L.Error logger f |
There was a problem hiding this comment.
Not sure if this is too broad. This would also suppress unrelated legitimate warnings logs -- assuming that those could exist.
There was a problem hiding this comment.
It's possible. Not sure. I think with replays we only end up actually caring about errors. We haven't even really seen warnings, if they are there, because they're buried by the Pact warnings.
| -- this can't go in Chainweb.Version.Guards because it causes an import cycle | ||
| -- it uses genesisHeight which is from BlockHeader which imports Guards |
There was a problem hiding this comment.
Would it make sense to move genesisHeight to a different place instead?
There was a problem hiding this comment.
Probably would cause a cycle. I tried it during recordification.
…nd/silent-replays
|
Tested on personal node and CI, it's much quieter. |
| either throwM (void . return) $ | ||
| validateHashes currHeader plData miner results |
* silent replays.... shhhhh * Lazier logging * Enable psReplaying flag when we replay, not when we don't * hashable 1.4.3.0 * Revert "hashable 1.4.3.0" This reverts commit 166adac. * Always validate hashes --------- Co-authored-by: chessai <[email protected]>
Silences non-Error messages from Pact during replays and lets the module cache stop cleaning itself up when it's already clean.