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

secp256r1 signature fees #4472

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

apfitzge
Copy link

Problem

  • ed25519 and secp256k1 signatatures count as signatures for fees
  • newly added secp256r1 signatures do not yet count

Summary of Changes

Built on top of #4354

  • Add FeeFeatures to hold bools relevant to feature activations used in fee calculation
  • Include secp256r1 signatures in fee calculation behind a feature gate

Fixes #

@apfitzge apfitzge added the noCI Suppress CI on this Pull Request label Jan 15, 2025
fee/src/lib.rs Outdated
Comment on lines 80 to 81
u64::from(fee_features.enable_secp256r1_precompile)
.wrapping_mul(num_secp256r1_signatures),
Copy link
Author

Choose a reason for hiding this comment

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

@samkim-crypto we need to block activation until the minimum version is whatever this PR lands in (2.2 hopefully?)

Choose a reason for hiding this comment

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

Yep, I will make sure double check before activating. It seems that you already put a note in https://github.com/anza-xyz/feature-gate-tracker/issues/65 as well.

@apfitzge apfitzge force-pushed the secp256r1_signature_fees branch from 2ba78b5 to 6dbf058 Compare January 16, 2025 19:09
@apfitzge apfitzge added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels Jan 16, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 16, 2025
/// Bools indicating the activation of features relevant
/// to the fee calculation.
#[derive(Copy, Clone)]
pub struct FeeFeatures {
Copy link
Author

Choose a reason for hiding this comment

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

Goal of this structure is so that we do not have a bunch of bool arguments that we always need, but also to not introduce the entire feature set which.
Keeping this makes it extremely obvious exactly which features affect the fee-calculation; and the FeeFeatures could even be re-used to avoid re-looking up in the feature-set!

Choose a reason for hiding this comment

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

since needed features come and go, there will be a time FeeFeatures becomes an empty struct. I would think to keep it empty then (perhaps with a note or something to prevent innocent "cleanup"), so to keep calculate_fee() signature unchanged. wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

yeah I agree with keeping it with a note, so that the design idea is not lost

Copy link
Author

Choose a reason for hiding this comment

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

@apfitzge apfitzge force-pushed the secp256r1_signature_fees branch from 6dbf058 to 460c76b Compare January 16, 2025 19:38
@apfitzge apfitzge force-pushed the secp256r1_signature_fees branch from 460c76b to e59bf5a Compare January 16, 2025 19:52
@apfitzge apfitzge marked this pull request as ready for review January 16, 2025 23:13
@apfitzge apfitzge requested review from a team as code owners January 16, 2025 23:13
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

goods good, just one nit

// in the future. Keeping this struct will help keep things organized.
#[derive(Copy, Clone)]
pub struct FeeFeatures {
pub remove_rounding_in_fee_calculation: bool,

Choose a reason for hiding this comment

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

do these need to be public, since only this model needs to know which feature(s) matter to fee calculation?

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.

4 participants