Verify frozen bank from snapshot by hashing#8184
Conversation
| let calculated_hash = self.hash_internal_state(); | ||
| let expected_hash = self.hash(); | ||
|
|
||
| if calculated_hash == expected_hash { |
There was a problem hiding this comment.
I'm being conservative here. I'm ballooning calculated_hash == expected_hash into a complete if now a days hard to debug hash mismatch errors are common. ;)
Codecov Report
@@ Coverage Diff @@
## master #8184 +/- ##
========================================
+ Coverage 81.3% 81.3% +<.1%
========================================
Files 251 251
Lines 53962 53986 +24
========================================
+ Hits 43876 43903 +27
+ Misses 10086 10083 -3 |
ryoqun
force-pushed
the
verify-frozen-bank
branch
from
089f3e7 to
1fe6a90
Compare
mergify
Bot
dismissed
sakridge’s stale review
Pull request has been modified.
| #[must_use] | ||
| fn verify_hash(&self) -> bool { | ||
| assert!(self.is_frozen()); | ||
| let calculated_hash = self.hash_internal_state(); |
There was a problem hiding this comment.
Just for the record: I'm little concerned about the verification failure for the restarted (hard-forked) cluster, but it should not be a problem because the renewed hard_fork information itself should be included in the bank of updated snapshot. Current ops doesn't use solana-validator's --hard-fork.
There was a problem hiding this comment.
solana-validator's --hard-fork is only there for a validator that never wants to consume a snapshot, for example a node that wants to archive the entire ledger to long-term storage for posterity.
There was a problem hiding this comment.
Oh, I see. Then, necessitated manual restarts (with command argument adjustment) for the hard-fork should be acceptable for such a scenario. Thanks for clarification!
3 participants
Problem
The actually recorded frozen hash of the bank in a snapshot isn't verified. It seems that it's lacking simply because of unintentional omission like #7559.
Summary of Changes
Just start to verify it.
Also, this is the base of the upcoming PR of more exhaustive protection for various bank fields. Currently, there are numerous unprotected fields, which could result in accepting tampered snapshot with unpredictable future behavior.
Part of #7167