runtime: fix possible deadlock in bank

[BurtonQin]

Problem

There are two kinds of possible deadlock bugs:

1. locks in conflicting order:

e.g.

compare_bank	freeze
stakes.read()
	hash.write()
	collect_return_eagerly
	collect_rent_in_partition
	store_account
	stakes.write()//deadlock!
hash.read()//deadlock!

The fix is to swap the order of stakes and hash in compare_bank().

2. double read lock if comparing with itself

When compare_bank() compares itself, there are four cases where the first read lock is not released before the second lock. A deadlock may happen when the two read locks are interleaved by a write lock from another thread. The reason is that the priority policy of std::sync::RwLock is dependent on the underlying operating system's implementation. AFAIK, Windows and macOS use fair policy which leads to this kind of deadlock.

The fix is to add a shortcut by checking the ptr equality of self and dbank first.

Summary of Changes

1. Swap the order of stakes and hash in compare_bank().
2. Add a shortcut by checking the ptr equality of self and dbank in compare_bank().

[ryoqun]

This looks nice! Thanks for reporting and creating an actual pr! Did you find this by https://github.com/BurtonQin/rust-lock-bug-detector?

Also, do you mind to write a testcase? Considering the kind of this bug, it might be a bit hard?

[BurtonQin]

This looks nice! Thanks for reporting and creating an actual pr! Did you find this by https://github.com/BurtonQin/rust-lock-bug-detector?

Also, do you mind to write a testcase? Considering the kind of this bug, it might be a bit hard?

Yes, and I am still improving this detector.
I do not think it is easy to write a test case because of the uncertainty of the execution sequences during run-time checking. So I choose to statically rule out every possible deadlock condition.

[codecov]

Codecov Report

Merging #10466 into master will decrease coverage by 0.0%.
The diff coverage is 94.4%.

@@            Coverage Diff            @@
##           master   #10466     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         296      296             
  Lines       69320    69326      +6     
=========================================
- Hits        56634    56614     -20     
- Misses      12686    12712     +26     

[ryoqun]

@BurtonQin nits: reordering is alone suffice to fix the deadlock as you said. Thanks for spotting this!

But for maintainability perspective, the current code is too fragile, relaying on the correct locking order. I think this method's each code blocks should also be surrounded with {} so that each locks aren't held for longer than needed.

[ryoqun]

nicely done!

@BurtonQin Could you work on the nit?

Pull request has been modified.

[BurtonQin]

nicely done!

@BurtonQin Could you work on the nit?

Sure. Pushed.

[solana-grimes]

💔 Unable to automerge due to CI failure