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.

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

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Nov 5, 2023

Add SIMD for adding fee collector constraints

@jstarry jstarry force-pushed the add-fee-collector-constraints branch from e66e3b9 to f20f7d4 Compare November 5, 2023 07:52
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

r+ nit and sign-off from other clients

Comment on lines 41 to 43
At the end of a block, validators MUST NOT distribute fees to accounts that are
not system owned and/or rent-exempt. Instead, they MUST burn the fees by not
distributing them to anyone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
At the end of a block, validators MUST NOT distribute fees to accounts that are
not system owned and/or rent-exempt. Instead, they MUST burn the fees by not
distributing them to anyone.
At the end of a block, validators MUST NOT distribute fees to accounts that are
rent-paying or owned by a non-system program. Instead, they MUST burn the fees by not
distributing them to anyone.

Nit. I thought this was a little easier to follow without one set of negatives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I flipped this to a set of positives which I hope is even clearer, thanks for pointing this out

one of the following constraints:

1. Must be a system owned account
2. Must be rent-exempt
Copy link
Contributor

@lheeger-jump lheeger-jump Nov 11, 2023

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.


## Motivation

1. Since fee collection occurs outside of the runtime, it's generally a good
Copy link
Contributor

Choose a reason for hiding this comment

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

How is it that fee collection occurs outside of the runtime? This seems like an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fees are deposited at the end of the block and must be done consistently across clients to maintain consensus. By outside the runtime, I meant outside of normal transaction processing.

Burns fees if they would otherwise be collected into an account that violates
one of the following constraints:

1. Must be a system owned account

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

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?

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?

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 😅

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

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.

@jstarry
Copy link
Contributor Author

jstarry commented Nov 16, 2023

I added more context to the motivations of this SIMD and added some arguments against other alternative solutions. Can everyone take another look and either approve or suggest changes? Thanks!

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Nice write-up @jstarry!

I think 2) is a given, but in my opinion 1) is worth a bit more discussion.

Sure you can totally accomplish a "multi-sig-esque" setup with System and nonce accounts, but that's not what they're designed for. I think it's at least worth exploring alternatives that may steer us away from overloading functionality/responsibility on one particular component.

Burns fees if they would otherwise be collected into an account that violates
one of the following constraints:

1. Must be a system owned account
Copy link
Contributor

Choose a reason for hiding this comment

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

My preferred solution is to accept that depositing fees should continue happening outside of any transactions and then constrain the fee collector account to limit the "nasty edge cases." If you take issue with limiting to system owned accounts, I think that having some concept of a fee collector program and fee collector accounts (which have configurable authorities) in the protocol would be ideal and flexible enough for any usecases. But I think they end up looking pretty similar to the system program + nonce accounts so I don't it's necessary to create such a program.

Would you suggest this hypothetical alternative would involve the non-transactional fee deposit to land in the "fee collector" program's account, and then the authority would have to withdraw?

I personally don't think it should be a deterrent to end up with something like "looks like" the system-nonce relationship, because with this suggestion, it sounds like we might get two things:

  • separation of duties, albeit maybe just in nomenclature
  • ability to withdraw with your signature of choice, not specifically a system-owned keypair
    • Think, for example, if I wanted to use my on-chain program to invoke-signed and withdraw from my fee collector account. This could be perfectly valid and still shave away the nasty edge cases mentioned.

@jstarry
Copy link
Contributor Author

jstarry commented Dec 18, 2023

Any other thoughts? Can I get an approval to merge this in @CriesofCarrots @lheeger-jump?

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Coming back to this with fresher eyes, I had additional thoughts about the writing/clarity. Nothing major, though; seems really close.

jstarry and others added 3 commits December 19, 2023 20:54
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for the polish! Lgtm

Copy link
Contributor

@lheeger-jump lheeger-jump left a comment

Choose a reason for hiding this comment

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

I am approving. This seems sufficiently well specified and, although I can imagine scenarios where you would want the fee collector to be a non-system account, it complicates the fee collection process. We can, and likely will, relax this constraint in the future but right now we need simplicity and clarity of design. This change tends towards that aim.

@jacobcreech
Copy link
Contributor

Looks like we got consensus from multiple clients. Thank you @jstarry!

@jacobcreech jacobcreech merged commit 4915952 into solana-foundation:main Dec 27, 2023
@jstarry jstarry deleted the add-fee-collector-constraints branch December 28, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: SIMDs
Development

Successfully merging this pull request may close these issues.

7 participants