Skip to content

Updates test_bank_hash_internal_state_same_account_different_fork()#7005

Merged
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:test_bank_hash_internal_state_same_account_different_fork
Jul 17, 2025
Merged

Updates test_bank_hash_internal_state_same_account_different_fork()#7005
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:test_bank_hash_internal_state_same_account_different_fork

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Jul 16, 2025

Problem

test_bank_hash_internal_state_same_account_different_fork() is broken when we remove accounts delta hash, blocking #7006.

Summary of Changes

Ensure the banks are frozen first, which ensures the accounts lt hash is updated, and the bank hash is updated.

Commentary

Note that the impl in this PR maintains similar behavior. Calling freeze() ensures we update the accounts lt hash, making it visible within hash_internal_state(). And at that point, we might as well get the bank hash via Bank::hash() instead.

The interesting part, imo, is thinking about how/what this is actually testing. If we actually had the same accounts, their accounts hashes would be the same. For the bank hash, we can have differences in the 'last blockhash', which will produce a different bank hash. I'm not sure if those were ticking at all though (since if it was, then the test wouldn't be failing). Otherwise, I think we do have different accounts (maybe due to sysvar updates?), which I think is why this tests passes. So I think we want to ensure the bank hashes are different, which we maintain with this PR.

Here's the PR that added this test originally: solana-labs#5573.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (4cb7c4a) to head (8b1a4c6).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7005     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      375383   375394     +11     
=========================================
  Hits       312489   312489             
- Misses      62894    62905     +11     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@brooksprumo brooksprumo marked this pull request as ready for review July 16, 2025 21:48
@brooksprumo brooksprumo requested a review from jstarry July 16, 2025 21:48
@jstarry
Copy link
Copy Markdown

jstarry commented Jul 17, 2025

Idk I think this test is pretty worthless because it's currently impossible that two blocks at different slots have the same accounts due to the updated clock sysvar. It seems like the original intent was more or less to make sure that the last blockhash changed between the two forks and I think we have sufficient test coverage for that. So I'd be more in favor of removing the test than "fixing" it. But your changes look fine too

@jstarry
Copy link
Copy Markdown

jstarry commented Jul 17, 2025

Reread your commentary again and I think it's fine to keep this as a sanity test to ensure that the accounts hash is actually being updated and actually being used in the bank hash.

Comment thread runtime/src/bank/tests.rs
Comment on lines 2298 to +2300
// Test that two bank forks with the same accounts should not hash to the same value.
#[test]
fn test_bank_hash_internal_state_same_account_different_fork() {
fn test_bank_hash_same_account_different_fork() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// Test that two bank forks with the same accounts should not hash to the same value.
#[test]
fn test_bank_hash_internal_state_same_account_different_fork() {
fn test_bank_hash_same_account_different_fork() {
// Test that two bank forks with the same transactions should not hash to the same value.
#[test]
fn test_bank_hash_same_transactions_different_fork() {

I think this is more accurate considering that the accounts are actually different (because of the clock sysvar)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm going to merge the PR as-is, and will address the rename in a follow up. This let's me unblock #7006.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

PR to rename the fn is up here: #7017.

@brooksprumo brooksprumo merged commit b4ba976 into anza-xyz:master Jul 17, 2025
41 checks passed
@brooksprumo brooksprumo deleted the test_bank_hash_internal_state_same_account_different_fork branch July 17, 2025 13:33
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.

3 participants