Skip to content

Conversation

@gzalz
Copy link
Contributor

@gzalz gzalz commented Jul 30, 2025

Problem
Current copy_priority_fee_distribution_account and compute_score instruction handlers don't consider all edge cases considering their permissionless nature.

  • There is no way to distinguish a PFDA that has not been copied vs. a PFDA that does not exist (no PF sharing opt-in from validator)
  • PFDAs can be copied before they are finalized
  • Scoring can be invoked before we can guarantee a PFDA has been copied potentially causing erroneous unstake conditions for a validator

Solution

  • Disallow calling copy_priority_fee_distribution_account until the epoch that account tracks is over
  • Allow copying PFDAs that do not exist - this is the only way we can distinguish DNE from not yet copied on-chain
  • Add new DNE merkle_root_upload_authority enum value
  • Disallow re-copying a specific PFDA to remove any possibility of copying a reclaimed account

IMPORTANT
This diff has been modified to have priority fee related score defaulted to 1.0. The intended logic has been commented out until the time of priority fee scoring launch.

@gzalz gzalz changed the base branch from master to feat/add-priority-fee-support July 30, 2025 04:42
epoch,
priority_fee_commission: commission,
priority_fee_tips,
merkle_root_upload_authority,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

great catch

Copy link
Collaborator

@ebatsell ebatsell Jul 30, 2025

Choose a reason for hiding this comment

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

for the record: this bug existed for the case where the copy_priority_fee_distribution ix is called on a validator as the first instruction before any other validator history ixs are called and there existed no entry yet for this validator

@gzalz gzalz marked this pull request as ready for review July 30, 2025 05:05
],
bump,
seeds::program = config.priority_fee_distribution_program.key(),
owner = config.priority_fee_distribution_program.key()
Copy link
Contributor

@mrmizz mrmizz Jul 30, 2025

Choose a reason for hiding this comment

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

I do think this check is redundant (bc we can the seeds and seeds::program constraints). So should we update the docstring above? It's specifically talking about the owner check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call-out. The big logic change here is that now we actually do want to allow a non-existent (still owned by system program) account to be passed in but it must abide by the PDA constraints. I will make sure this is reflected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh even better. Did not quite catch that. Nice work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was copying a non-existent account possible before? I think it wouldn't have been because of the owner constraint

}

/// Boolean representing if fields required for scoring have been set
pub fn can_score(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this method to is_finalized?

I do think we should lean into this concept of "finalizing" a validator history entry. If there is outstanding/missing data, is it okay that the steward can score without it?

The following PR will require the CopyStakeInfo instruction to run during the current epoch. While this CopyPriorityFeeDistribution is required to run after the current epoch. This means that history accounts will always be partially updated, up until being finalized.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a few other requirements for finalization, kind of baked into the StewardState::compute_score fn like the vote credits being updated in the current epoch and cluster history being updated

Open to putting those all in one method but we should keep scoring logic on the Steward side since validator history is supposed to be just the data store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropping this function as discussed offline. The process of "skipping" an epoch while scoring will look different and impact score rather than instruction success/failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I jive with this ^

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually name it something a little more verbose, has_copied_priority_fees()

In this case can_score() seems to vague, and is_finalized() is closer, but its meaning is lost.

Copy link
Contributor

@coachchucksol coachchucksol left a comment

Choose a reason for hiding this comment

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

This all seems to hit what we need!

  • Cannot call copy on N epoch
  • Cannot overwrite data once its been written
  • Added the DNE
  • Cannot score without a valid MRUA

Just a couple of comments

}

/// Boolean representing if fields required for scoring have been set
pub fn can_score(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I jive with this ^

}

/// Boolean representing if fields required for scoring have been set
pub fn can_score(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually name it something a little more verbose, has_copied_priority_fees()

In this case can_score() seems to vague, and is_finalized() is closer, but its meaning is lost.

Copy link
Contributor

@coachchucksol coachchucksol left a comment

Choose a reason for hiding this comment

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

Another thought

Copy link
Contributor

@coachchucksol coachchucksol left a comment

Choose a reason for hiding this comment

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

A couple more things

let distribution_account = PriorityFeeDistributionAccount::try_deserialize(&mut pdfa_data)
.unwrap_or(PriorityFeeDistributionAccount {
validator_vote_account: Pubkey::default(),
merkle_root_upload_authority: Pubkey::default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Set as DNE_AUTHORITY not Pubkey::default(),

&[
MerkleRootUploadAuthority::TipRouter,
MerkleRootUploadAuthority::TipRouter,
MerkleRootUploadAuthority::TipRouter,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any test cases with MerkleRootUploadAuthority::Other?

@gzalz gzalz merged commit 0f997a2 into feat/add-priority-fee-support Jul 31, 2025
6 checks passed
@gzalz gzalz deleted the gzalz/pf-scoring-fix branch July 31, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants