Burn fees collected into invalid accounts #33887

jstarry · 2023-10-27T02:04:36Z

Problem

Validators can currently collect transaction and rent fees into any account type they wish. This is unnecessarily permissive and fees should only be allowed to be collected into system owned accounts that are rent exempt.

Summary of Changes

Refactor: move fee distribution code into new bank::fee_distribution module to avoid increasing the size of bank.rs
Prevent collecting transaction and rent fees into non system owned accounts
Prevent the creation of new rent paying accounts via transaction fee collection
Move Bank::deposit into test_utils module to discourage its usage

Feature Gate Issue: #33888

codecov · 2023-10-27T03:19:14Z

Codecov Report

Merging #33887 (9978d32) into master (ba112a0) will increase coverage by 0.0%.
Report is 29 commits behind head on master.
The diff coverage is 97.8%.

@@           Coverage Diff            @@
##           master   #33887    +/-   ##
========================================
  Coverage    81.8%    81.9%            
========================================
  Files         810      811     +1     
  Lines      218148   218368   +220     
========================================
+ Hits       178650   178952   +302     
+ Misses      39498    39416    -82

CriesofCarrots

Just a couple questions and nits.
I only commented on a couple, but I'd love to see all the test match results cleaned up for readability.

runtime/src/bank/fee_distribution.rs

runtime/src/bank.rs

runtime/src/bank/fee_distribution.rs

CriesofCarrots · 2023-10-27T19:52:14Z

runtime/src/bank/fee_distribution.rs

-                        warn!(
-                            "Rent distribution of {rent_to_be_paid} to {pubkey} results in \
-                            invalid RentState: {recipient_post_rent_state:?}"
-                        );
-                        datapoint_warn!(
-                            "bank-rent_distribution_invalid_state",
-                            ("slot", self.slot(), i64),
-                            ("pubkey", pubkey.to_string(), String),
-                            ("rent_to_be_paid", rent_to_be_paid, i64)
-                        );


Just calling out that we do lose this warning and datapoint in the new deposit_fees. Hopefully, prevent_rent_paying_rent_recipients() will be active before this PR is released, so we'll no longer care about tracking that specific case so carefully.

Yeah I was curious if you wanted to keep that or not, I can keep it in if you want!

@jeffwashington , @brooksprumo , I think you're the main users of this datapoint currently. Thoughts on removing it?

The latest patch of this PR removes this particular datapoint_warn but has two datapoint_warn's that track how many lamports were burned during both tx and rent fee distribution

datapoint_warn!( "bank-burned_rent", ("slot", self.slot(), i64), ("num_lamports", rent_to_burn, i64) ); datapoint_warn!( "bank-burned_fee", ("slot", self.slot(), i64), ("num_lamports", deposit, i64), ("error", err.to_string(), String), );

Wdyt about adding the pubkey back into the bank-burned_fee datapoint?

I know that putting text/pubkeys into metrics isn't great, but it may be useful for any debugging. Since this is an exceptional case, I would expect this datapoint to not trigger often.

For bank-burned_fee we track the slot which allows us to infer who the leader was that failed to collect tx fees

Works for me, thanks!

CriesofCarrots · 2023-10-27T19:55:23Z

runtime/src/bank/fee_distribution.rs

+                            error!(
+                                "Burned {} rent lamports instead of sending to {}",
+                                rent_to_be_paid, pubkey
+                            );
+                            datapoint_error!(
+                                "bank-burned_rent",
+                                ("slot", self.slot(), i64),
+                                ("num_lamports", rent_to_be_paid, i64)
+                            );


Do we maybe want to back down these, or at least the log? It's not really the kind of thing a node operator would interpret as an error.

Yeah good idea. I think that if a validator is burning its own fees it should see an error but it won't care about other validators. Do you care if I remove the datapoint?

I don't know of a need for the datapoint and am personally fine with its removal, but that might be a good question for @jeffwashington or @HaoranYi

I think a datapoint_info! will be enough. I think it will be better if we accumulate all the burned fees and submit one data point for it.

@HaoranYi is there a specific reason why you would like to track this datapoint? I can't think of any reason personally.

It gives us the visibility of how much fees are burned in the network. The amount of the fees burned should be very small. To me, being able to run such a query and verify the amount of fees burned, could be a useful sanity check.

Good point, I'll add it back in, thanks

I added the datapoint back in but downgraded it to "warn"

runtime/src/bank/fee_distribution.rs

HaoranYi · 2023-10-31T16:01:50Z

runtime/src/bank/fee_distribution.rs

+    }
+
+    // Deposits fees into a specified account and if successful, returns the new balance of that account
+    fn deposit_fees(


Can we split this fu into two fns? one for transaction fee specificially and one for rent specificially?
For transaction fee, we don't need to check for rent state transition. Right?

We need the same checks (including rent state transition) in both cases, that's why I created this function. If we collect transaction fees into an account whose post balance is not greater than or equal to the rent exempt balance, we need to skip the fee deposit because otherwise we allow creating rent paying accounts. Make sense?

Yeah.
It seems that the current code still allows transaction fees to be collected into an account whose post balance is not greater than or equal to the rent exempt balance. Should we create a separate PR to fix this for current code and backport it to v1.17?

I'm not sure what you mean, can you elaborate more? By the "current code" you mean the code in this PR right? And under what conditions are you saying that the code "still allows transaction fees to be collected into an account whose post balance is not greater than or equal to the rent exempt balance"?

By "current code", I mean master.

In master, we don't validate the post balance of the account that transaction fees are deposit to. If the balance of the account pre-transaction fee was zero, then after the transaction fee deposit, we introduce a "new" rent paying account, right?

The new code (i.e. deposit_fee fn) in the PR will prevent this from happening. What I mean is that we pull this piece out in a new PR to patch master separately.

I see, I'd rather not break up apart this PR unless we have a good reason to. I purposely setup this PR to introduce the shared deposit_fee function to ensure that all invalid cases are handled for each type of fee collection. Before, without a common function, we missed the rent paying edge case for collected transaction fees.

OK. I think we might want to port this feature back to 1.17 to close those edge cases as soon as possible.

runtime/src/bank/fee_distribution.rs

HaoranYi

Nice work. Just a few nits and suggestions.

CriesofCarrots

Lgtm. It just occurred to me that, since this adds a feature gate, it really should have been a SIMD first.

CriesofCarrots · 2023-11-03T19:41:36Z

runtime/src/bank/fee_distribution.rs

                        }
                    }
                }
            });
        self.rewards.write().unwrap().append(&mut rewards);

+        if rent_to_burn > 0 {
+            self.capitalization.fetch_sub(rent_to_burn, Relaxed);


This change to a single subtraction per loop looks fine, but since it was potentially substantive, it would have been nice if called out in its own commit.

runtime/src/bank.rs

brooksprumo · 2023-11-04T10:44:21Z

runtime/src/bank/fee_distribution.rs

-                        warn!(
-                            "Rent distribution of {rent_to_be_paid} to {pubkey} results in \
-                            invalid RentState: {recipient_post_rent_state:?}"
-                        );
-                        datapoint_warn!(
-                            "bank-rent_distribution_invalid_state",
-                            ("slot", self.slot(), i64),
-                            ("pubkey", pubkey.to_string(), String),
-                            ("rent_to_be_paid", rent_to_be_paid, i64)
-                        );


Wdyt about adding the pubkey back into the bank-burned_fee datapoint?

I know that putting text/pubkeys into metrics isn't great, but it may be useful for any debugging. Since this is an exceptional case, I would expect this datapoint to not trigger often.

jstarry · 2023-11-05T07:50:35Z

Lgtm. It just occurred to me that, since this adds a feature gate, it really should have been a SIMD first.

Good point, I created a SIMD to improve visibility and explain this change in more detail: solana-foundation/solana-improvement-documents#85

HaoranYi

lgtm

* refactor: create bank::fee_distribution module * feature: add checks to fee distribution * refactor: move Bank::deposit fn into test_utils * feedback * feedback 2 * add datapoints * change to datapoint_warn * typo (cherry picked from commit ebe8afb) # Conflicts: # runtime/src/bank.rs # runtime/src/bank/tests.rs

…#33951) * Burn fees collected into invalid accounts (#33887) * refactor: create bank::fee_distribution module * feature: add checks to fee distribution * refactor: move Bank::deposit fn into test_utils * feedback * feedback 2 * add datapoints * change to datapoint_warn * typo (cherry picked from commit ebe8afb) # Conflicts: # runtime/src/bank.rs # runtime/src/bank/tests.rs * resolve conflicts --------- Co-authored-by: Justin Starry <justin.starry@icloud.com> Co-authored-by: Justin Starry <justin@solana.com>

t-nelson

that the refactors have been squashed into the logical changes is pretty not great. this should have been at least two PRs, probably more.

have we checked how many mb validators will be effected by this change?

do we intend to prevent assignment of invalid fee-collection accounts?

t-nelson · 2023-11-15T20:01:40Z

runtime/src/bank.rs

@@ -3805,7 +3804,7 @@ impl Bank {
            .stakes_cache
            .stakes()
            .highest_staked_node()
-            .unwrap_or_default();
+            .unwrap_or_else(Pubkey::new_unique);


this method should absolutely not be used outside of tests under any circumstances. can we just fail/panic instead?

It's only used in tests right now... using the system program (default pubkey) as the highest staked node should also not happen outside of tests in any circumstances. I'll clean it up in a follow up change though to be sure

PR fix is being worked on here: #34135

t-nelson · 2023-11-15T20:05:55Z

runtime/src/bank/fee_distribution.rs

+        fees: u64,
+        options: DepositFeeOptions,
+    ) -> Result<u64, DepositFeeError> {
+        let mut account = self.get_account_with_fixed_root(pubkey).unwrap_or_default();


why not fail directly here rather than do the whole footgun-prone unwrap_or_default() thing?

Because it's fine if the account doesn't exist as long as the fees top up the balance to at least rent exempt.

jstarry · 2023-11-16T01:51:40Z

that the refactors have been squashed into the logical changes is pretty not great. this should have been at least two PRs, probably more.

Noted, will do that next time.

have we checked how many mb validators will be effected by this change?

Right now the validator implementation forces nodes to use the fee collector account (also known as node id account) as the fee payer for their votes (therefore must already be a valid system account). And tpu votes also checks that the node id is the payer for the votes and uses that to weight vote transactions for block inclusion. So no one will be impacted.

do we intend to prevent assignment of invalid fee-collection accounts?

Not now.. it isn't possible to enforce that a fee-collection account stays valid. Its owner can change at any time and it can be withdrawn to zero lamports at any time. Best thing I think we could do in the future is to create an enshrined "NodeAccount" or "FeeCollectorAccount" that must always be valid and used separately from fee payments. But that's a big lift and probably not necessary. If validators want fees, they won't create funky fee collection accounts.

…labs#33887) (solana-labs#33951) * Burn fees collected into invalid accounts (solana-labs#33887) * refactor: create bank::fee_distribution module * feature: add checks to fee distribution * refactor: move Bank::deposit fn into test_utils * feedback * feedback 2 * add datapoints * change to datapoint_warn * typo (cherry picked from commit ebe8afb) # Conflicts: # runtime/src/bank.rs # runtime/src/bank/tests.rs * resolve conflicts --------- Co-authored-by: Justin Starry <justin.starry@icloud.com> Co-authored-by: Justin Starry <justin@solana.com>

t-nelson · 2023-11-16T17:41:41Z

this was way too big to backport. cc/ @willhickey

if we're developing a fix with intention to backport, it must be done in the smallest change set possible. now we have a mass of refactor to sift through if we need to debug compatibility issues (which we are ATM)

…labs#33887) (solana-labs#33951) * Burn fees collected into invalid accounts (solana-labs#33887) * refactor: create bank::fee_distribution module * feature: add checks to fee distribution * refactor: move Bank::deposit fn into test_utils * feedback * feedback 2 * add datapoints * change to datapoint_warn * typo (cherry picked from commit ebe8afb) # Conflicts: # runtime/src/bank.rs # runtime/src/bank/tests.rs * resolve conflicts --------- Co-authored-by: Justin Starry <justin.starry@icloud.com> Co-authored-by: Justin Starry <justin@solana.com>

jstarry added 3 commits October 27, 2023 10:01

refactor: create bank::fee_distribution module

69827fa

feature: add checks to fee distribution

ea1f7f3

refactor: move Bank::deposit fn into test_utils

7d204a2

jstarry requested review from HaoranYi, t-nelson and CriesofCarrots October 27, 2023 02:04

jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Oct 27, 2023

CriesofCarrots reviewed Oct 27, 2023

View reviewed changes

feedback

4cb3bca

jstarry force-pushed the feature/check-fee-distribution branch from fbeea2e to 4cb3bca Compare October 30, 2023 13:07

HaoranYi reviewed Oct 31, 2023

View reviewed changes

runtime/src/bank/fee_distribution.rs Outdated Show resolved Hide resolved

HaoranYi reviewed Oct 31, 2023

View reviewed changes

runtime/src/bank/fee_distribution.rs Outdated Show resolved Hide resolved

HaoranYi reviewed Oct 31, 2023

View reviewed changes

jstarry added 3 commits November 1, 2023 19:05

feedback 2

a5c8660

add datapoints

4d192ce

change to datapoint_warn

cf93883

CriesofCarrots previously approved these changes Nov 3, 2023

View reviewed changes

brooksprumo reviewed Nov 4, 2023

View reviewed changes

typo

9978d32

jstarry dismissed CriesofCarrots’s stale review via 9978d32 November 5, 2023 07:13

HaoranYi approved these changes Nov 5, 2023

View reviewed changes

jstarry merged commit ebe8afb into solana-labs:master Nov 6, 2023
42 checks passed

jstarry deleted the feature/check-fee-distribution branch November 6, 2023 02:02

jstarry added the v1.17 PRs that should be backported to v1.17 label Nov 6, 2023

mergify bot mentioned this pull request Nov 6, 2023

v1.17: Burn fees collected into invalid accounts (backport of #33887) #33951

Merged

t-nelson reviewed Nov 15, 2023

View reviewed changes

jstarry mentioned this pull request Nov 17, 2023

Update genesis processing to have a fallback collector id for tests #34135

Merged

HaoranYi mentioned this pull request Apr 8, 2024

pr634 1.17 new filter anza-xyz/agave#657

Closed

Burn fees collected into invalid accounts #33887

Burn fees collected into invalid accounts #33887

Conversation

jstarry commented Oct 27, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi left a comment

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Nov 5, 2023

HaoranYi left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Nov 16, 2023 • edited Loading

t-nelson commented Nov 16, 2023

jstarry commented Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

jstarry commented Nov 16, 2023 •

edited

Loading