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[bot] merged 1 commit intosolana-labs:masterfrom
mvines:no0
Oct 10, 2020
Merged

bank: Don't bother paying 0 rent#12791
mergify[bot] merged 1 commit intosolana-labs:masterfrom
mvines:no0

Conversation

@mvines
Copy link
Copy Markdown
Contributor

@mvines mvines commented Oct 10, 2020

No description provided.

@mvines mvines added v1.3 automerge Merge this Pull Request automatically once CI passes labels Oct 10, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 10, 2020

Codecov Report

Merging #12791 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #12791   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         360      360           
  Lines       84776    84778    +2     
=======================================
+ Hits        69461    69465    +4     
+ Misses      15315    15313    -2     

@mergify mergify Bot merged commit 1fc7c1e into solana-labs:master Oct 10, 2020
Comment thread runtime/src/bank.rs
post_balance: account.lamports,
},
));
if rent_to_be_paid > 0 {
Copy link
Copy Markdown
Contributor

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

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

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

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

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

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants