Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIMD-0085: Additional Fee Collector Constraints #85

Merged
merged 7 commits into from
Dec 27, 2023
Merged

SIMD-0085: Additional Fee Collector Constraints #85

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f20f7d4
SIMD-0085: Additional Fee Collector Constraints
jstarry Nov 5, 2023
a217281
Add more context and address feedback
jstarry Nov 16, 2023
8271c5c
use consistent terminology
jstarry Nov 16, 2023
c211574
explain why the validator node account alternative isnt used
jstarry Nov 16, 2023
09d5d00
Update proposals/0085-additional-fee-collector-constraints.md
jstarry Dec 19, 2023
fb0387b
Update proposals/0085-additional-fee-collector-constraints.md
jstarry Dec 19, 2023
805179e
feedback
jstarry Dec 19, 2023
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
94 changes: 94 additions & 0 deletions proposals/0085-additional-fee-collector-constraints.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
---
simd: "0085"
title: Additional Fee-Collector Constraints
authors:
- Justin Starry
category: Standard
type: Core
status: Draft
created: 2023-11-05
feature: https://github.com/solana-labs/solana/issues/33888
---

## Summary

Every validator defines a node id which is also used as the validator's
fee-collector account for collecting earned protocol fees. After implementing
this proposal, validator implementations will burn fees if they would otherwise
be distributed into a fee-collector account that violates one of the following
constraints:

1. Must be a system owned account
Copy link

Choose a reason for hiding this comment

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

Why system owned? This restricts the use of multisig wallet for this for example

Copy link

Choose a reason for hiding this comment

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

I just want to understand why non-system owned are bad if all other constraints are met?

Copy link

Choose a reason for hiding this comment

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

I read the comments on the PR too, but there are no explanations there either as to why the system owned restriction is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that isn't ideal, the main reason is that it limits the number of edge cases that core protocol devs need to consider when thinking about the ways that accounts could change. You could use a nonce account as the fee collector still and set the authority to a multisig. It's kinda hacky and not ideal, though. What do you think?

Copy link

Choose a reason for hiding this comment

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

I’m really sorry, but I have to disagree. The problem of forgetting to make a check is more an issue of implementation rather than the model itself. If, in the implementation, for example, an account can self-validate against declarative rules before saving, then this problem would not exist. The account wouldn't be recorded, the block wouldn't be created, and nothing bad would happen. I understand that such an implementation is the most expensive, but it remains a problem of implementation. And the check here is no more than after a regular transfer (any account can receive lamports). In my experience, artificially imposed constraints have led to further constraints in the future (like the random example about multisig; a nonce account is not quite the same, the signature of the account receiving fees is not needed on the node).

To be honest, for a complete picture, in my opinion, the ability to create non-rent exempt accounts was a very cool feature. But in that case, as I understand, there was a real reason why they needed to be phased out.

In this case, I still don’t see any real reasons. Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that the hypothetical alternative of having a specific fee collector program account is still a possible path to take in the future. For now, limiting to system program seems appropriate to limit edge cases. Separation of duties is a nice to have for sure, my argument was that it's not a "must" right now. And also note that nonce account authorities can already be PDAs so you can use an on-chain program to withdraw fees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that the hypothetical alternative of having a specific fee collector program account is still a possible path to take in the future. For now, limiting to system program seems appropriate to limit edge cases.

All right, I think a two-phase approach is appropriate. Once anyone gets on the nonce method, though, I don't really see a reason why anybody would start up any dialogue about an official program for fee collection 😅

Copy link

Choose a reason for hiding this comment

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

Hello,

I've given this a lot of thought. I've revisited it many times but still haven't made a final decision. However, I want to share my thoughts.

On one hand, I understand the issue you're trying to solve. If we think on the level of transaction abstractions, our thinking breaks down, and it seems right to do what you suggest. Because (for me) in this case, we are (adding these rewards) somewhere in the on_write of a transaction (in this case a block, but we can abstract a block as a big transaction), and the necessary checks for modified accounts should be in on_write, putting us in a dilemma (since we have to do everything we actually want to postpone and have done for us).
So, looking at the model as I've tried to explain (not sure if I've made it clear), I want to minimize risks and keep it as simple (reliable) as possible.
However, the paradox is that, thinking this way, I always concluded that thinking at the transaction abstraction level here is wrong, not that it should be done this way.
That is, awarding rewards is a thing in itself and should be limited exclusively by the protocol's rules regarding accounts.
And at this point, it seems illogical not to be able to apply all protocol rules. Yes, I realize that these rules may change, but in any case, there should be a place where they are all collected in one place in the code (i.e., there should be a function capable of doing this). And the number of places where this function is called seems to be quite small because it's the lowest level. Thus, any change in protocol rules that leads to changing such a function simply entails reviewing its use.
So, the real risks are not substantial.

Therefore, imposing restrictions on the owner still seems excessive to me. And although I understand its reasons, I cannot accept it wholeheartedly. But this can hardly be considered an argument.

Also, in my reflections, I came to the idea that there could be more than one reward (i.e., a validator might split rewards based on some principle, like automatically replenishing an account to pay for voting fees and putting the rest in the treasury), but while I'd really like such flexibility, here I'm more inclined to say no, because it could create an unaccounted CU expenditure (as a resource of the validator), i.e., a potential point for attack.

Furthermore, I am negative about creating a special program for accounting for rewards. I don't really believe in it, given that the number of clients is increasing. Introducing such a program in the future would require agreement by many and could be delayed indefinitely. And considering that nonce accounts can be managed by programs, it's likely to be rejected. So, changes to the current proposal are likely to be the last (as I see it).
Moreover, I thought that the development of such programs should be the business of validators. That is, any validator can develop their own program according to their needs (which may include factors like taxation, for example).
Simple steps could include, for instance, using a PDA for each epoch or, say, a calendar month (which is closer to tax issues).
But creating a universal program that satisfies everyone is unlikely to succeed. Or it would be something very simple, in which case why a new program? Why not just expand the vote program, adding an account to which to attribute rewards (with the requirement that its owner be the vote program).
But returning to the current discussion, having the ability to attribute rewards to any account in this case does not contradict all this. But managing the nonce account from an external program doesn't either. So this is an argument neither for nor against.

Something like that

Copy link

Choose a reason for hiding this comment

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

tldr I do not mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your thoughts @diman-io, I'm happy we agree that nonce accounts are flexible enough for now.

2. Must be rent-exempt
Copy link
Contributor

@lheeger-jump lheeger-jump Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

SIMD-0084 aims to disable rent. Does this affect that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to disable rent collection, all accounts must be enforced to be rent exempt when created. Fee collection was discovered to be an exception because it modifies accounts when depositing collected fees and didn't enforce that the new balance is rent exempt. So this exception needs to be patched to disable rent.


These constraints apply to both transaction fees and rent fees. Note that the
rent-exempt constraint was already added for rent fee collection in
[Feature 30151](https://github.com/solana-labs/solana/issues/30151).

## Motivation

1. Fee distribution occurs outside of the transaction runtime because the Solana
protocol mandates that fees are distributed to "fee-collector" accounts at the
end of each block. By restricting fee-collector accounts to be system owned, the
number of account modification edge cases as well as protocol complexity are
both reduced.
2. Prevent new rent-paying accounts from being created since rent collection is
planned to be disabled in SIMD-0084.

## Alternatives Considered

### Elide the system-owned constraint

Restricting fee-collector accounts to be system-owned is perhaps overly
restrictive and limits the amount of flexibility that validator operators have
when managing sensitive accounts with funds. However, the risk of having more
runtime edge cases is too high to allow any program-owned account to collect
fees. The Solana protocol should aim to limit the types of account modifications
that can occur outside of the transaction processor to avoid introducing
loopholes.

### Introduce an enshrined "validator-node" account

Rather than restricting fee-collector accounts to be system-owned, a new type of
"validator-node" account could be introduced. Currently, in normal validator
operations, the fee-collector account is also used as the node id as well as
the vote fee payer. Introducing a validator-node account that is owned by a
validator-node program which allows configuring a withdraw authority and
vote fee payer could help increase validator operation flexibility and
increase clarity in how validator keys are used in the protocol.

This approach requires a migration of all fee-collector accounts as well as
the development of a new on-chain program to manage the new validator-node
accounts. It will be a big effort compared to the proposed constraints in this
SIMD and should be discussed in a new SIMD if this approach is desired.
Furthermore, durable nonce accounts already have a configurable authority field
which can be used to manage fee-collector account funds in a more flexible way.

## New Terminology

Fee-Collector Account: The account that receives block and rent fees distributed
by validators.

## Detailed Design

At the end of a block, validators MUST ONLY distribute fees to accounts that are
both system owned and rent-exempt. If a fee-collector account does not satisfy
these constraints, the fees MUST be burned by not distributing them to anyone.

## Impact

New and existing validators must ensure that their fee-collector account is
rent-exempt and owned by the system program in order to receive fees. Since the
Solana Labs validator implementation currently requires the fee-collector
account to be same account as the fee payer for vote transactions, this is
unlikely to impact any validators unless they run a custom implementation.

Validators will still be able to collect fees into durable nonce accounts if
they wish. If a validator does not wish to use a hot wallet to have custody
over collected fees, they may use durable nonce accounts which have a
configurable authority address.

## Security Considerations

Note that durable nonce accounts are system owned and rent exempt and can
therefore continue to be used for fee collection.