paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Partial Fee returns invalid result for operational extrinsics #11106

Open crystalin opened 2 years ago

crystalin commented 2 years ago

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

Description of bug

When querying the fees through api.rpc.payment.queryInfo the value returns is different from what is expected. The following program computes the fees using this rpc method and compares it to the treasury deposit event in that same extrinsic.

Running it with ts-node block-fees.tx 9468639 shows:

[  ok  ] timestamp.set
    -    fees:              0 (Mandatory)
    -     80%:              0
    - deposit:              0
[  ok  ] paraInherent.enter
    -    fees:              0 (Mandatory)
    -     80%:              0
    - deposit:              0
[  ok  ] council.propose
    -    fees:      123000046 (Operational)
    -     80%:       98400036
    - deposit:       98400034
[  ok  ] balances.transfer
    -    fees:      159000012 (Normal)
    -     80%:      127200009
    - deposit:      127200009
[  ok  ] balances.transfer
    -    fees:      159000012 (Normal)
    -     80%:      127200009
    - deposit:      127200009
[  ok  ] balances.transfer
    -    fees:      159000012 (Normal)
    -     80%:      127200009
    - deposit:      127200009
[  ok  ] staking.bondExtra
    -    fees:      123000079 (Normal)
    -     80%:       98400063
    - deposit:       98400063

As you can see the 80% of the council.propose fees don't match the deposit to the treasury (but it works for all the other extrinsics).

I've verified it with other Operational and most (if not all) are failing to report the right value. Except when the extrinsic fails, in that case it has the correct value. I've also verified it with parachains and it is the same error.

Steps to reproduce

Code to check the block fees:

import { ApiPromise, WsProvider } from "@polkadot/api";
import "@polkadot/api-augment/polkadot";

const main = async () => {
  // Instantiate Api
  const api = await ApiPromise.create({
    provider: new WsProvider("wss://rpc.polkadot.io"),
  });
  await api.isReady;

  const args = process.argv.slice(2);
  const blockNumber =
    args.length > 0 ? parseInt(args[0]) : (await api.rpc.chain.getHeader()).number.toNumber();

  const blockHash = await api.rpc.chain.getBlockHash(blockNumber);
  console.log(`Reading data for #${blockNumber} (${blockHash.toString()})`);
  const apiAt = await api.at(blockHash);

  const { block } = await api.rpc.chain.getBlock(blockHash);
  const records = await apiAt.query.system.events();

  await Promise.all(
    block.extrinsics.map(async (extrinsic, index) => {
      const txRecords = records.filter(
        ({ phase }) => phase.isApplyExtrinsic && phase.asApplyExtrinsic.eq(index)
      );
      const treasuryDeposit = txRecords
        .filter(({ event }) => event.section == "treasury" && event.method == "Deposit")
        .reduce((p, { event }) => p + (event.data[0] as any).toBigInt(), 0n);

      const fees = await api.rpc.payment.queryInfo(extrinsic.toHex(), blockHash);

      const isSuccess = !!txRecords.find(
        ({ event }) => event.section == "system" && event.method == "ExtrinsicSuccess"
      );

      console.log(`[${isSuccess ? "  ok  " : "failed"}] ${extrinsic.method.section}.${
        extrinsic.method.method
      } 
    -    fees: ${fees.partialFee.toBigInt().toString().padStart(14)} (${fees.class.toString()})
    -     80%: ${((fees.partialFee.toBigInt() * 80n) / 100n).toString().padStart(14)}
    - deposit: ${treasuryDeposit.toString().padStart(14)}`);
    })
  );
  await api.disconnect();
};

main();
crystalin commented 2 years ago

cc @jacogr as he might know better how to compute/handle the fees in polkadotjs

jacogr commented 2 years ago

As far as I understand the queryInfo actually executes the block and then returns the fees based on that - so it should be 100%.

Where it does fall short is where refunds come into play. (I have a mental note of this somewhere, sometime ago to look at the actual Rust code in these cases, but sadly have not had a gap since I made that note)

crystalin commented 2 years ago

Thank you @jacogr that was my understanding. I can't understand why it is different :(

notlesh commented 2 years ago

After some more investigation, it looks like this affects extrinsics which return a DispatchResultWithPostInfo with a different weight than their declared #[pallet::weight(..)] fn.

Here are some we've noticed:

councilCollective.close
councilCollective.propose
identity.clearIdentity
identity.setIdentity
identity.setSubs
techCommitteeCollective.close
techCommitteeCollective.propose
utility.batch
utility.batchAll
notlesh commented 2 years ago

I believe the problem is simply that pallet_transaction_payment's query_info only queries the extrinsic's get_dispatch_info without actually executing it:

https://github.com/paritytech/substrate/blob/127536132f52b0a1ef0693a4adb5399a175a4aa1/frame/transaction-payment/src/lib.rs#L419

bkchr commented 2 years ago

Yeah that makes sense. Executing them would be way too costly.