[Enhancement] Bail on database lookup errors. #3137
Conversation
ruseinov
requested review from
elmattic,
lemmih,
LesnyRumcajs,
jdjaustin,
hanabi1224,
creativcoder,
sudo-shashank and
aatifsyed
as code owners
src/interpreter/fvm.rs
Outdated
| Err(err) | ||
| } | ||
| // invalid consensus fault: cannot verify block header signature | ||
| Error::Signature(_) => Ok((None, total_gas)), |
There was a problem hiding this comment.
Do we know why is it okay to discard this error? I get that this logic was there before, I'm just curious.
There was a problem hiding this comment.
Yes, AFAIK when the signature is not correct - we ignore the consensus fault, but we need to report the total gas used for the lookup.
Co-authored-by: Hubert <hubert@chainsafe.io>
Co-authored-by: Hubert <hubert@chainsafe.io>
Co-authored-by: Hubert <hubert@chainsafe.io>
Co-authored-by: Hubert <hubert@chainsafe.io>
src/interpreter/fvm.rs
Outdated
| if let Ok(gas_used) = self.verify_block_signature(&bh_1) { | ||
| total_gas += gas_used; | ||
| if let Ok(gas_used) = self.verify_block_signature(&bh_2) { | ||
| let res = self.verify_block_signature(&bh_1); |
There was a problem hiding this comment.
verify_block_signature has three possible outputs: fatal error, non-fatal error, ok(gas_used). We can handle these three branches without any nesting. Not a big deal, but it would make the code easier to read for me.
There was a problem hiding this comment.
I was juggling with possible options to make it more readable, the trick here is that a non-fatal error still requires the gas to be passed around so the eloquence suffers there. Let me see what I can do here.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #3126
Other information and links
Change checklist