Fix bad rent in Bank::deposit as if since epoch 0 #10468

ryoqun · 2020-06-09T08:33:03Z

Problem

When an account is created via Bank::deposit(), Bank charges too much rent on it: all the epochs since genesis if the account doesn't exist yet.... In other words, it collects rent for the number of epochs.

Also, the rent timing was wrongly delayed: #10468 (comment)

Mostly, this only affects tests. The only case which hits the production is collector_id's fee distribution because it uses Bank::deposit(). So the impact should be fairly small.

This should be a rolling update, otherwise this is a DOS vulnerability. These old/new behaviors are themselves consistent across validators.

Summary of Changes

~~Calculate rent for newly-funded accounts since the current epoch not epoch 0~~

I've changed mind: Disable rent collection entirely; for the reasoning, see the comments and document updates.

gating logic (ugh)
unit test

context

found while finishing #10099
FYI: @mvines (About rolling update).

ryoqun · 2020-06-09T08:39:04Z

runtime/src/accounts.rs

@@ -155,7 +155,8 @@ impl Accounts {
                            AccountsDB::load(storage, ancestors, accounts_index, key)
                                .map(|(mut account, _)| {
                                    if message.is_writable(i) && !account.executable {


Well, these inconsistent predicates are concerning me. Ideally, I want to put everything into RentCollector::update/RentCollector::from_existing_account(). But, to avoid any additional risks, I've punted it. ;)

ryoqun · 2020-06-09T08:39:49Z

runtime/src/accounts.rs

@@ -738,8 +739,7 @@ impl Accounts {
                );
                if message.is_writable(i) {
                    if account.rent_epoch == 0 {


Well, these inconsistent predicates are concerning me. Ideally, I want to put everything into RentCollector::update/RentCollector::collect_from_existing_account(). But, to avoid any additional risks, I've punted it. ;)

ryoqun · 2020-06-09T08:49:39Z

runtime/src/accounts.rs

@@ -154,8 +154,9 @@ impl Accounts {
                        let (account, rent) =
                            AccountsDB::load(storage, ancestors, accounts_index, key)
                                .map(|(mut account, _)| {
-                                    if message.is_writable(i) && !account.executable {
-                                        let rent_due = rent_collector.update(&key, &mut account);
+                                    if message.is_writable(i) {


I removed !account.executable because this one was fairly easy to prove it's redundant.

codecov · 2020-06-09T12:41:49Z

Codecov Report

Merging #10468 into master will increase coverage by 0.0%.
The diff coverage is 92.4%.

@@           Coverage Diff           @@
##           master   #10468   +/-   ##
=======================================
  Coverage    82.4%    82.4%           
=======================================
  Files         314      314           
  Lines       73713    73765   +52     
=======================================
+ Hits        60740    60795   +55     
+ Misses      12973    12970    -3

t-nelson

Nice catch!

These changes look good to me!

One nit, the from_ prefix felt like a conversion before rereading deeper. Maybe collect_ would read a little better

stale · 2020-06-16T21:18:31Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2020-06-23T21:20:17Z

This stale pull request has been automatically closed. Thank you for your contributions.

ryoqun · 2020-06-25T15:38:29Z

Now #10206 is landed, I'll work on this!!

stale · 2020-07-02T22:19:05Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2020-07-10T00:24:20Z

This stale pull request has been automatically closed. Thank you for your contributions.

stale · 2020-07-17T07:29:31Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Pull request has been modified.

ryoqun · 2020-07-29T15:27:13Z

One nit, the from_ prefix felt like a conversion before rereading deeper. Maybe collect_ would read a little better

Thanks for suggestion! I fixed that!

@t-nelson Could you review this pr? I've finished to work on this. And I think this is ready for merge. :)

Also, while writing tests, I found yet another bug, though: https://github.com/solana-labs/solana/pull/10468/files#r462385554

t-nelson

lgtm!

ryoqun · 2020-07-31T07:12:08Z

runtime/src/bank.rs

+        let account = self.get_account(pubkey);
+        let created = account.is_none();
+
+        let mut account = account.unwrap_or_default();
        account.lamports += lamports;


@t-nelson Ugh, I've found some concerns while testing against local/testnet/mainnet-beta....

Specifically, the order of deposit and rent is significant in a corner case when the account is to be reclaimed by rent, meaning there is too little lamports at the time of loading..

Also, there is some inconsistency for rent collection timing semantics with regards to #10426 (comment). Strictly abide by the rules, we would end up doing rent collection twice in deposit because we're both loading and storing at once. But I think the divergent behavior here is tolerable because bank::deposit() is fairly limited use.

I'm thinking for a good compromise... At bottom, I want to finish this without much complexities introduced and without any cluster consistency issue when updating.

Ok, my thinking is making progress with a fresh brain after weekend.

Come to think of it, update_rewards() doesn't care about rents at all.

So, maybe deposit() should follow the suit as well? So, the general idea would be that any kinds of protocol-level account manipulation is entirely exempt from rent manipulation. That will simplify the reasoning a lot.

Even if we do that, the accounts will be rent-collected at least once an epoch by eager rent collection. So skipping rents here shouldn't be an escape hatch.

I've checked the origin of rent in deposit(): https://github.com/solana-labs/solana/pull/6947/files#r347114185

As far as I understand, it seems that the reasoning is that all updates should be rent-collected. This is now realized by the eager rent collection now.

Also, I found another protocol-level account manipulation: collected rent distribution. That should be free of rent collection to avoid recursion..

So, I'm more inclined to the general consistent idea of no rent collection for protocol-level account manipulation.

Also, this would slightly alleviate rather hefty processing of update_rewards by avoiding rent collection there for the consistency of rent-collection coverage.

ryoqun · 2020-07-31T07:13:24Z

no merge please, I found some issues to be addressed after local testing..: https://github.com/solana-labs/solana/pull/10468/files#r463443390

Pull request has been modified.

ryoqun · 2020-08-03T03:40:49Z

@t-nelson I've completely turned around the solution after this course of thinking: #10468 (comment).

In short, this is my preferred new solution: https://github.com/solana-labs/solana/pull/10468/files#diff-80339caf6f22201fa3399a7761c91b88R17

How does this look for you? :)

t-nelson

This LGTM! Let's give @rwalker-com a few days to comment on the proposal changes, then go ahead and merge

t-nelson · 2020-08-07T16:33:08Z

docs/src/implemented-proposals/rent.md

+- The distribution of transaction fee at the end of every epoch
+
+Even if those processes are out of scope of rent collection, all of manipulated accounts will eventually be handled by the \(2\) mechanism.
+


This sounds good to me. It'd be great to get a comment from @rwalker-com to be sure we aren't missing anything. 🙏

lgtm, and this is consistent with what I intended, but the call flow to load and store accounts is pretty tortured, and has many paths. were the bank to document which APIs were for runtime access of accounts, it would go a long way to making these easier to reason about.

the call flow to load and store accounts is pretty tortured ...

~~@rwalker-com~~ (UPDATED) Yeah, I think so too... I'll address the documetation work along with #10426

Anyway, thanks for confirming this pr!

Err, wrong user @rwalker-apple...

rwalker-apple

lgtm

* Fix bad rent in Bank::deposit as if since epoch 0 * Remove redundant predicate * Rename * Start to add tests with some cleanup * Forgot to add refactor code... * Enchance test * Really fix rent timing in deposit with robust test * Simplify new behavior by disabling rent altogether (cherry picked from commit 6c242f3) # Conflicts: # runtime/src/accounts.rs # runtime/src/rent_collector.rs

* Fix bad rent in Bank::deposit as if since epoch 0 * Remove redundant predicate * Rename * Start to add tests with some cleanup * Forgot to add refactor code... * Enchance test * Really fix rent timing in deposit with robust test * Simplify new behavior by disabling rent altogether (cherry picked from commit 6c242f3)

ryoqun · 2020-08-11T15:38:37Z

todo

write local manual test steps for posterity

* Fix bad rent in Bank::deposit as if since epoch 0 * Remove redundant predicate * Rename * Start to add tests with some cleanup * Forgot to add refactor code... * Enchance test * Really fix rent timing in deposit with robust test * Simplify new behavior by disabling rent altogether (cherry picked from commit 6c242f3) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

* Fix bad rent in Bank::deposit as if since epoch 0 (#10468) * Fix bad rent in Bank::deposit as if since epoch 0 * Remove redundant predicate * Rename * Start to add tests with some cleanup * Forgot to add refactor code... * Enchance test * Really fix rent timing in deposit with robust test * Simplify new behavior by disabling rent altogether (cherry picked from commit 6c242f3) # Conflicts: # runtime/src/accounts.rs # runtime/src/rent_collector.rs * Fix conflict * Fix clippy Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun · 2020-08-17T05:55:43Z

[ ] write local manual test steps for posterity

First of all, the manual test for this fix needed to be rather complicated because voting validators cause its fee-paying accounts (= identity accounts) to be touched via the vote transactions by themselves. This causes rent collection, meaning Bank::deposit() becomes no-op with regards to the rent collection. Also, normal pay transactions doesn't touch Bank::deposit(), so...

In short, run a local cluster with two validators, which are both staked. One is running with unpatched version, the other with patched version. And intentionally make one of validator not to vote at all, meaning the validator is only producing blocks while collecting fees. (= which touches Bank::deposit()).

Specifically, like this:

# validator 1
# warmup is needed to accelerate new validator joining.
$ SOLANA_RUN_SH_GENESIS_ARGS="--enable-warmup-epochs" ./run.sh

# validator 2 (preps and actual running)
$ ./target/release/solana --url http://localhost:8899 airdrop 100000
$ ./target/release/solana --url http://localhost:8899 create-vote-account /tmp/vote.json /tmp/node.json
$ ./target/release/solana --url http://localhost:8899 pay /tmp/node.json 2
$ ./target/release/solana --url http://localhost:8899 create-stake-account /tmp/stake.json 0.04851334
# force is needed to stake to non-voting vote account.
$ ./target/release/solana --url http://localhost:8899 delegate-stake --force /tmp/stake.json /tmp/vote.json
# --no-voting didn't work? just don't give a voting-pubkey, so that ephemeral voting key effectively disables voting.
$ ./solana-validator{,-bad-rent} --ledger ledger{,bad-rent} --identity /tmp/node.json --entrypoint localhost:8001 --log - --rpc-port 8665 --snapshot-interval-slots 0

Then wait to see the cluster dies without or with mix of differing validators.

ryoqun added v1.1 labels Jun 9, 2020

ryoqun requested a review from t-nelson June 9, 2020 08:33

ryoqun commented Jun 9, 2020

View reviewed changes

t-nelson previously approved these changes Jun 9, 2020

View reviewed changes

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 16, 2020

stale bot closed this Jun 23, 2020

ryoqun reopened this Jun 25, 2020

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 25, 2020

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 2, 2020

stale bot closed this Jul 10, 2020

ryoqun reopened this Jul 10, 2020

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 10, 2020

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 17, 2020

ryoqun force-pushed the bad-rent-in-bank-deposit branch from d1b6b0a to 25adb08 Compare July 23, 2020 12:25

stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 23, 2020

ryoqun added 3 commits July 27, 2020 21:33

Fix bad rent in Bank::deposit as if since epoch 0

097c864

Remove redundant predicate

2df4e88

Rename

54ce39e

ryoqun requested a review from t-nelson July 29, 2020 15:24

t-nelson previously approved these changes Jul 30, 2020

View reviewed changes

ryoqun commented Jul 31, 2020

View reviewed changes

ryoqun changed the title ~~Fix bad rent in Bank::deposit as if since epoch 0~~ [no merge] Fix bad rent in Bank::deposit as if since epoch 0 Jul 31, 2020

ryoqun mentioned this pull request Aug 3, 2020

move rent collection to store_account in bank #7056

Closed

3 tasks

Simplify new behavior by disabling rent altogether

42fd2c2

ryoqun mentioned this pull request Aug 3, 2020

Possible resource exhausition attack for account storage with reasonable funds #11342

Closed

ryoqun requested a review from t-nelson August 3, 2020 03:38

ryoqun changed the title ~~[no merge] Fix bad rent in Bank::deposit as if since epoch 0~~ Fix bad rent in Bank::deposit as if since epoch 0 Aug 3, 2020

mvines added the v1.3 label Aug 6, 2020

t-nelson approved these changes Aug 7, 2020

View reviewed changes

rwalker-apple approved these changes Aug 7, 2020

View reviewed changes

ryoqun merged commit 6c242f3 into solana-labs:master Aug 11, 2020

mergify bot mentioned this pull request Aug 11, 2020

Fix bad rent in Bank::deposit as if since epoch 0 (bp #10468) #11538

Merged

mergify bot mentioned this pull request Aug 11, 2020

Fix bad rent in Bank::deposit as if since epoch 0 (bp #10468) #11539

Merged

This was referenced Aug 11, 2020

Rent timing is not consistant for writable/read-only access #10426

Closed

Re-do rent collection check on rent-exempt account #11349

Merged

This was referenced Aug 11, 2020

v1.3: Enable retirement of rent collect in Bank::deposit() on testnet #11543

Closed

v1.3: Enable retirement of rent collect in Bank::deposit() on mainnet-beta #11545

Closed

ryoqun mentioned this pull request Aug 12, 2020

Fix typo: epoch => slot... #11573

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad rent in Bank::deposit as if since epoch 0 #10468

Fix bad rent in Bank::deposit as if since epoch 0 #10468

ryoqun commented Jun 9, 2020 •

edited

Loading

ryoqun Jun 9, 2020 •

edited

Loading

ryoqun Jun 9, 2020 •

edited

Loading

ryoqun Jun 9, 2020

codecov bot commented Jun 9, 2020 •

edited

Loading

t-nelson left a comment

stale bot commented Jun 16, 2020

stale bot commented Jun 23, 2020

ryoqun commented Jun 25, 2020

stale bot commented Jul 2, 2020

stale bot commented Jul 10, 2020

stale bot commented Jul 17, 2020

ryoqun commented Jul 29, 2020

t-nelson left a comment

ryoqun Jul 31, 2020 •

edited

Loading

ryoqun Aug 3, 2020 •

edited

Loading

ryoqun Aug 3, 2020

ryoqun commented Jul 31, 2020

ryoqun commented Aug 3, 2020

t-nelson left a comment

t-nelson Aug 7, 2020

rwalker-apple Aug 7, 2020

ryoqun Aug 11, 2020 •

edited

Loading

ryoqun Aug 11, 2020

rwalker-apple left a comment

ryoqun commented Aug 11, 2020 •

edited

Loading

ryoqun commented Aug 17, 2020

		- The distribution of transaction fee at the end of every epoch

		Even if those processes are out of scope of rent collection, all of manipulated accounts will eventually be handled by the \(2\) mechanism.

Fix bad rent in Bank::deposit as if since epoch 0 #10468

Fix bad rent in Bank::deposit as if since epoch 0 #10468

Conversation

ryoqun commented Jun 9, 2020 • edited Loading

Problem

Summary of Changes

context

ryoqun Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Jun 9, 2020

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2020 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

stale bot commented Jun 16, 2020

stale bot commented Jun 23, 2020

ryoqun commented Jun 25, 2020

stale bot commented Jul 2, 2020

stale bot commented Jul 10, 2020

stale bot commented Jul 17, 2020

ryoqun commented Jul 29, 2020

t-nelson left a comment

Choose a reason for hiding this comment

ryoqun Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Aug 3, 2020

Choose a reason for hiding this comment

ryoqun commented Jul 31, 2020

ryoqun commented Aug 3, 2020

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Aug 7, 2020

Choose a reason for hiding this comment

rwalker-apple Aug 7, 2020

Choose a reason for hiding this comment

ryoqun Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

ryoqun Aug 11, 2020

Choose a reason for hiding this comment

rwalker-apple left a comment

Choose a reason for hiding this comment

ryoqun commented Aug 11, 2020 • edited Loading

ryoqun commented Aug 17, 2020

ryoqun commented Jun 9, 2020 •

edited

Loading

ryoqun Jun 9, 2020 •

edited

Loading

ryoqun Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 9, 2020 •

edited

Loading

ryoqun Jul 31, 2020 •

edited

Loading

ryoqun Aug 3, 2020 •

edited

Loading

ryoqun Aug 11, 2020 •

edited

Loading

ryoqun commented Aug 11, 2020 •

edited

Loading