Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

bank: Don't bother paying 0 rent #12791

Merged
mergify merged 1 commit into from
Oct 10, 2020
Merged

bank: Don't bother paying 0 rent #12791

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2676,17 +2676,19 @@ impl Bank {
} else {
rent_share
};
let mut account = self.get_account(&pubkey).unwrap_or_default();
account.lamports += rent_to_be_paid;
self.store_account(&pubkey, &account);
rewards.push((
pubkey,
RewardInfo {
reward_type: RewardType::Rent,
lamports: rent_to_be_paid as i64,
post_balance: account.lamports,
},
));
if rent_to_be_paid > 0 {
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Oct 12, 2020

Choose a reason for hiding this comment

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

@mvines I'm certain this will create testnet cluster crash because this changes some accounts are written or not, depending on validator updated or not.. testing on #12175. Also, I'll just make enforce_fix cover this new behavior.

Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Oct 12, 2020

Choose a reason for hiding this comment

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

I mean, this ultimately produces different bank hashes.

Copy link
Copy Markdown
Contributor Author

@mvines mvines Oct 12, 2020

Choose a reason for hiding this comment

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

Oh really? Wow. Because self.store_account(&pubkey, &account); was skipped here? My main goal was to skip rewards.push(...), but then didn't see a reason to not skip the entire block and no test told me otherwise!

Mainnet-beta should be fine since rent is still 100% burned. There's been rent payments on testnet though since this PR is in 1.3.16, testnet hasn't blown apart yet. Have we just been lucky?

Copy link
Copy Markdown
Contributor Author

@mvines mvines Oct 12, 2020

Choose a reason for hiding this comment

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

Oh my bad. This PR missed v1.3.16 so it's not live on testnet/mainnet-beta yet 😅

Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Oct 12, 2020

Choose a reason for hiding this comment

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

Because self.store_account(&pubkey, &account); was skipped here?

Yeah, I think so. Just preparing to test this locally as well.

Oh my bad. This PR missed v1.3.16 so it's not live on testnet/mainnet-beta yet sweat_smile

😅

Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Oct 12, 2020

Choose a reason for hiding this comment

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

@mvines I've confirmed this locally and on CI and created a fix for this: #12804.

let mut account = self.get_account(&pubkey).unwrap_or_default();
account.lamports += rent_to_be_paid;
self.store_account(&pubkey, &account);
rewards.push((
pubkey,
RewardInfo {
reward_type: RewardType::Rent,
lamports: rent_to_be_paid as i64,
post_balance: account.lamports,
},
));
}
});
self.rewards.write().unwrap().append(&mut rewards);

Expand Down