-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7396 +/- ##
=========================================
+ Coverage 68.4% 80.6% +12.2%
=========================================
Files 244 244
Lines 56475 47779 -8696
=========================================
- Hits 38658 38548 -110
+ Misses 17817 9231 -8586 |
}); | ||
// Sort first by stake and then by pubkey for determinism | ||
node_stakes.sort_by( | ||
|(pubkey1, staked1), (pubkey2, staked2)| match staked2.cmp(staked1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sort doesn't need to be stable, does it? you should be safe sorting merely by stake...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-solana If I sort by stake only, it could lead to indeterminism, as the order might not be guaranteed for node with same stake amount. Which could result in rare cases, different distribution of leftover lamports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you testing for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-solana not specifically. Let me add a separate test scenario for this, just for my assurance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-solana Added a scenario in test-case.
runtime/src/bank.rs
Outdated
let node_stakes_and_rent = node_stakes | ||
.iter() | ||
.map(|(pubkey, staked)| { | ||
let rent_in_initial_round = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rent_to_be_paid was just as good a name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also be rent_share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, nits are optional
Pull request has been modified.
Problem
Currently, rent payment to validator is as a fraction of collected rent in accordance with their stake weight. Since lamports are atomic, the calculation results in leftover lamports (for N validators, maximum N lamports), which are then lost.
Proposed Solution
Distribute those leftover lamports among the validators from descending order of stake weight.
Each validator gets 1 lamport from the leftover lamports, until all leftover lamports are exhausted.
Fixes #7381