separate functions for different signature types #4354
Conversation
fee/src/lib.rs
Outdated
| .num_transaction_signatures() | ||
| .saturating_add(message.num_ed25519_signatures()) | ||
| .saturating_add(message.num_secp256k1_signatures()) | ||
| .saturating_add(message.num_secp256r1_signatures())) |
There was a problem hiding this comment.
wdyt adding a helper function to do all these saturating_adds, add comment to remind that this function need to be updated if signature types are removed/added in future, and feature gate is necessary?
There was a problem hiding this comment.
I'd kind of rather not, and would rather that all these signatures get removed from fees entirely w/ only the base num_transaction_signatures counting as "signatures" for fee calculations.
With 9ypxGLzkMxi89eDerRKXWDXe44UY2z4hBig4mDhNq5Dp we're already moving towards precompiles being treated more similarly to other programs. I think it's natural we remove the special-case in fee to reflect that. Any "cost" to the validator should be reflected in additional CUs and require priority to make them worth processing.
There was a problem hiding this comment.
In shorter term, I am on the fence of removing SVMMesage from this interface entirely and having a struct FeeSignatures {} as an argument to this (defined in this crate).
Makes it much more clear exactly what is going into fee calculation.
There was a problem hiding this comment.
^ actually.. typing it out made it more clear to me. We should 100% make this change. It'd make it impossible for unexpected stuff from the much more generic SVMMessage interface to creep into the fee-calculation.
keeping that as a follow-up change. right now just changing the SVMMessage interface so we can deprecate the structure from sdk.
There was a problem hiding this comment.
yea, I agree. Was just thinking if putting these adds in one separate place would make it easier to change when precompiles become normal programs. Your suggestion of removing SVMMessage would serve same purpose.
apfitzge
force-pushed
the
separate_signature_functions
branch
from
56ee2aa to
52cfd85
Compare
fee/src/lib.rs
Outdated
| } | ||
|
|
||
| /// This function has the actual fee calculation. | ||
| fn calculate_fee_details_internal( |
There was a problem hiding this comment.
I added this function as an additional internal level of abstraction.
It has only very simple argument types that are all explicitly typed. No traits hide the behavior; nearly any change to how the fee is calculated is immediately obvious from this code.
Not super satisfied with prioritization_fee being just passed in like it is now. Code for determining the prioritization fee should imo be inside this fee crate. But it's outside scope of this PR which is focusing on changing how we were using signature counts in the fee-calculation.
There was a problem hiding this comment.
For now can we just have an internal fn that calculates the signature fee? I don't love that this function otherwise just wraps the FeeDetails constructor and we have all these unit tests which are really only testing signature fees anyways.
|
|
||
| FeeDetails::new( | ||
| signature_fee, | ||
| prioritization_fee, | ||
| remove_rounding_in_fee_calculation, | ||
| ) | ||
| } | ||
|
|
||
| struct SignatureCounts { |
There was a problem hiding this comment.
see follow-up PR in #4472 which adds the secpr1 signature count behind a feature-gate.
fee/src/lib.rs
Outdated
| } | ||
|
|
||
| /// This function has the actual fee calculation. | ||
| fn calculate_fee_details_internal( |
There was a problem hiding this comment.
For now can we just have an internal fn that calculates the signature fee? I don't love that this function otherwise just wraps the FeeDetails constructor and we have all these unit tests which are really only testing signature fees anyways.
apfitzge
force-pushed
the
separate_signature_functions
branch
from
84efc30 to
10e3213
Compare
There was a problem hiding this comment.
@jstarry thinking to make the fee functions above take something where SignatureCounts: From<&T>.
This gives the functions a bit more flexibility, since we should be able to use this function for types that do not implement SVMMessage. Ideally we'd use this same function for estimating priority before we resolve ALTs (which SVMMessage requires from us); and could just implement the correct From here.
If fee model changed and we needed additional information from the type, this also gives a bit more flexibility than just taking SignatureCounts as the argument directly, as we can just add additional bounds:
pub fn calculate_fee_details<'a, T>(message:T,...) -> FeeDetails
where
SignatureCounts: From<&'a T>,
OtherInformation: From<&'a T>,
{}I think the alternative is basically just us doing this a few times for different types we want to calculate fees for, which seems worse to me than using From traits. let me know what you think of the approach.
There was a problem hiding this comment.
Maybe in a follow up? I'm happy with the current PR. I'm just not sure how useful it is to have priority before we resolve ALTs in the scheduler..
There was a problem hiding this comment.
It's useful because if below the current minimum priority, I won't resolve at all.
Less useful in scheduler, more useful in forwarding stage (wip) where the buffer is significantly smaller
There was a problem hiding this comment.
also just to expand on this, the medium-term plan is to move prioritization into sigverify stage. but we don't want SV reaching into accounts-db at all, obviously. so we'd want a way to calculate fees just from an unresolved view (essentially to what i mentioned for forwarding stage now).
|
@anza-xyz/svm can I get a review on this change? SVM-related change is iin the |
Problem
TransactionSignatureDetailsfromsdk. it should have never been a public typesvm-messageSummary of Changes
TransactionSignatureDetailsand refactoring feesFixes #