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

fix!(calc): fee calculation #974

Merged
merged 22 commits into from
Aug 11, 2022
Merged

fix!(calc): fee calculation #974

merged 22 commits into from
Aug 11, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Jun 28, 2022

This PR will be followed up by a release of @substrate/calc, then a updated PR to implement the changes provided by the wasm_bindgen

Summary (PR Co-authored by @jsdw, and Summary written by @jsdw)

Uses the following formula to calculate an extrinsics partial_fee (ie the total fee minus any tip).

partial_fee = base_fee + len_fee + ((adjusted_weight_fee/estimated_weight)*actual_weight)

Where:

  • base_fee is a fixed base fee to include some transaction in a block. It accounts
    for the work needed to verify the signature and such and is constant for any tx.
  • len_fee is a fee paid based on the size (length in bytes) of the transaction. Longer
    transactions require more storage.
  • adjusted_weight_fee is a fee that is itself estimated_weight * targeted_fee_adjustment.
    targeted_fee_adjustment is some adjustment made based on the network load and such, and is
    an opaque internal value we have no access to.
  • estimated_weight is the "pre-dispatch" weight of the transaction. It's set based on the cost of processing the transaction on reference hardware.
  • actual_weight is the weight that is found in the ExtrinsicSuccess event for the extrinsic in
    a block (it's just called weight in the event), and is often the same as estimated_weight,
    but the node has the opportunity to change it to whatever it likes, I think.

The RPC endpoint payment_queryFeeDetails returns base_fee, len_fee and adjusted_weight_fee/
The RPC endpoint payment_queryInfo returns estimated_weight (called weight in the response), and
a partialFee value, which is our best guess at the inclusion fee for the tx without actually submitting
it and seeing whether the node changes the weight/decides not to take a fee at all.

To get the correct values for some extrinsic from both endpoints, provide the extrinsic bytes, and the
block number one before the block it made it into (eg if the extrinsic was in block 100, you'd use
block 99 as an argument). This is very important.

Once you've called these endpoints, access the ExtrinsicSuccess event to find the actual_weight, but
also a paysFee value which signals whether the extrinsic actually incurred a fee at all or not (a node
has the opportunity to refund the fee entirely if it likes by setting this).

With all of those values to hand, the equation above calculates the correct Fee. Why? Well, the basic
way to calculate a pre-dispatch fee is:

partial_fee = base_fee + len_fee + adjusted_weight_fee

We can do this from just the RPC methods. But then once it's in a block, we need to swap out the weight used
to calculate that adjusted_weight_fee with the actual weight that was used from the ExtrinsicSuccess event.
In the end, the maths is simple and gathering the details needed is the main difficulty. We do this all in
Rust simply to limit any precision loss.

Note

This PR removes legacy calc_fee

@TarikGul TarikGul requested a review from a team as a code owner June 28, 2022 14:25
@TarikGul TarikGul added A3 - In Progress PR in progress, not ready for review P2 - ASAP Get fixed ASAP labels Jun 29, 2022
@TarikGul TarikGul self-assigned this Jun 29, 2022
@TarikGul TarikGul removed the P2 - ASAP Get fixed ASAP label Jul 5, 2022
@TarikGul
Copy link
Member Author

@athei I was wondering if you had any pointers here for using Fixed precision with sp_arithmetic in order to do the above calculations as accurately as possible.

@athei
Copy link
Member

athei commented Jul 18, 2022

I thought no calculation is necessary anymore since we can get all of that from the node via RPC?

@TarikGul
Copy link
Member Author

I thought no calculation is necessary anymore since we can get all of that from the node via RPC?

So yes it is not necessary for chains specific to parity (ie. Polkadot, Kusama, Westend etc.) but for parachains query_info doesnt actually give accurate fee calculations. Therefore we are trying to implement the same algorithm subscan uses for all chains.

@kianenigma
Copy link

If you just want to know the fee in a retrospective way, we can kiss and goodbye this whole wasm calculation because we added paritytech/substrate#11618

@jsdw
Copy link
Collaborator

jsdw commented Aug 4, 2022

Just to add a brief history from chatting with @TarikGul:

  • We created the rust calc package which calculated extrinsic fees back in the day. This relied on some "extrinsic base weight" which used to be a constant exposed on a node, but isn't any more (see Introduce WeightToFee trait instead of WeightToFeePolynomial and make WeightToFeePolynomial implement it instead substrate#11415). Without that, calc as it stands is no longer able to do fee calculation (see Rewrite Fee Calculation in sidecar (Potentially phase out of @substrate/calc) #935)

  • So now we are trying to take the following approach:

    • Call the payment.queryFeeDetails method, which takes the signed extrinsic bytes and returns base_fee, len_fee and adjusted_wight_fee.
    • Call the payment.queryInfo method, which takes in the signed extrinsic bytes and returns a weight we'll call estimated_weight. This might not be the exact weight the extrinsic would actually have been submitted using, so it's just an estimate. (NB this call also returns the partialFee, also an estimate for the same reasons, but we don't use this).
    • Look for the ExtrinsicSuccess event for the transaction we want to calculate the weight for. This contains a weight we'll call actual_weight.
    • Apply the calculation provided by SubScan (see above) to try to calculate the partial fee for the given transaction (base_fee + len_fee)+((adjusted_weight_fee/estimated_weight)*actual_weight).

But the results we get don't quite match up to the real known fees for some transactions.

Notes:

  • We can't rely solely on things like a new fee payment event, since sidecar needs to be able to provide fee info for transactions in historic blocks, too.

And now some questions:

  • Why are our calculations off by a small amount? (is it a precision related issue to do with dividing and re-multiplying or is our approach above actually wrong?)
  • Is there a better way to approach this (eg should we add something to Substrate to help us calculate/return accurate historic fee info)?

@kianenigma
Copy link

Why are our calculations off by a small amount? (is it a precision related issue to do with dividing and re-multiplying or is our approach above actually wrong?)

Could be, I recall that during the creation of calc we realized that there are some differences between how the rust types behave.


Reading all of this, I really hope that we can solve this in a fundamentally new way, using the runtime as the source of truth and not some opaque formula by subscan that may or may not be respected in any given runtime.

So as far as I know, subscan has the machinery to decode the tx-fee of most of the ranges of historical blocks, right? and from now on, it should rely on this event. Isn't this more or less "problem solved"?

@TarikGul
Copy link
Member Author

TarikGul commented Aug 8, 2022

So as far as I know, subscan has the machinery to decode the tx-fee of most of the ranges of historical blocks, right? and from now on, it should rely on this event. Isn't this more or less "problem solved"?

So it's mostly/partially problem solved. Moving forward we would be able to use the new event as a source of truth for future blocks. For historical blocks there is a solution in place currently that uses events to collect fee information, but I dont think the solution is the be all end all for sidecar. In addition, Sidecar is not able to get any information from Subscan, so if they have a solution in place we would have to adopt it to retrieve any of that fee accuracy.

My main concern (and this concern is also shared across exchanges) is that Sidecar needs a guarantee that every block historically has the correct fees. For Polkadot, Kusama, and Westend this is true so on that front Sidecar is great. But for parachains is a bit more tricky. I briefly mentioned it above, but I added a query param called feeByEvent which allows fee abstraction from events. It uses a shallow comparison between the partialFee of queryInfo and the events actualFee (partialFee + tip), and then collects the fee from there. I think it's a good solution for now for parachains, but hopefully we can make it better.

Reading all of this, I really hope that we can solve this in a fundamentally new way, using the runtime as the source of truth and not some opaque formula by subscan that may or may not be respected in any given runtime.

I definitely agree on this. Having the runtime be the source of truth is definitely the route I hope is taken. It's always better especially for anything downstream because it's usually easier to handle or deal with if there are any changes.

@TarikGul TarikGul changed the title [WIP] fix(fee): fee calculation fix(fee): fee calculation Aug 10, 2022
@TarikGul TarikGul changed the title fix(fee): fee calculation fix!(fee): fee calculation Aug 10, 2022
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Just to check; is the partial_fee_as_string method all you'll need on the sidecar side? I think so, but worth being sure before you do a release and then find you need another release to add some missing thing :)

@TarikGul
Copy link
Member Author

@jsdw So I checked everything out and ran it locally. I like the current API we are exposing. 👍 from me. All checked out as well

@TarikGul TarikGul added A0 - Please Review PR is ready for review R1 - Rust related issue Pull requests that update Rust code and removed A3 - In Progress PR in progress, not ready for review labels Aug 10, 2022
@TarikGul TarikGul changed the title fix!(fee): fee calculation fix!(calc): fee calculation Aug 11, 2022
@TarikGul TarikGul merged commit b4a8781 into master Aug 11, 2022
@TarikGul TarikGul deleted the tarik-fix-fee branch August 11, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0 - Please Review PR is ready for review R1 - Rust related issue Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants