Skip to content

Conversation

@notlesh
Copy link
Contributor

@notlesh notlesh commented Jan 24, 2022

This PR is essentially a revival of #730, which means:

  1. Writing a custom BlockWeights impl which specifies a value for base_extrinsic (this should account for more expensive ECDSA signature verification)
  2. Replacing WeightToFee config with a custom impl which is implemented as a flat modifier, but uses 1_000_000 instead of 1. This reflects MOVR/GLMR having 18 decimal places whereas KSM/DOT have 12.
  3. Tweaking per-byte fees.

Note that there is currently a problem with tweaking per-byte fees (described below) where it's not possible to have a small txn be sufficiently cheap and a large txn be sufficiently expensive. I have an issue/PR against Substrate which would make this possible:

paritytech/substrate#10784
paritytech/substrate#10785

@notlesh notlesh added A3-inprogress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jan 24, 2022
@notlesh notlesh mentioned this pull request Jan 24, 2022
4 tasks
@girazoki
Copy link
Collaborator

Dont forget to modify

Trader = (
   	UsingComponents<
   		IdentityFee<Balance>,
   		SelfReserve,
   		AccountId,
   		Balances,
   		DealWithFees<Runtime>,
   	>,
   	FirstAssetTrader<AssetType, AssetManager, XcmFeesToAccount>,
   );

for

Trader = (
   	UsingComponents<
   		WeightToFee,
   		SelfReserve,
   		AccountId,
   		Balances,
   		DealWithFees<Runtime>,
   	>,
   	FirstAssetTrader<AssetType, AssetManager, XcmFeesToAccount>,
   );

@notlesh
Copy link
Contributor Author

notlesh commented Jan 26, 2022

Alan and I discussed rolling these changes out to Alphanet for testing. This would mean only touching the moonbase runtime and leaving the others as they are, then later following up with a new PR to roll the changes out to others.

@girazoki
Copy link
Collaborator

Makes sense to me. Have we gotten any pre-liminary numbers on how the numbers look like now?

@notlesh
Copy link
Contributor Author

notlesh commented Jan 26, 2022

Makes sense to me. Have we gotten any pre-liminary numbers on how the numbers look like now?

Yes, a couple I tested were closer to a 10% difference with their Ethereum-txn counterparts. I'll try to post a list of comparisons as I do more testing today.

@librelois librelois mentioned this pull request Jan 26, 2022
32 tasks
@notlesh
Copy link
Contributor Author

notlesh commented Jan 27, 2022

Copying my notes from yesterday here:

simple balance transfer:
- before:   1.15 milli
- after:    22.6205 micro
- ethereum: 0.000021 (@ 1 GWEI)

staking delegatorBondMore:
- before:   1.3 milli
- after:    63.6010 micro
- ethereum: 0.000062 (@ 1 GWEI, potential refund)

empty system remark:
- before: 950.0001 micro
- after: 13.4787 micro
- ethereum: N/A
- KSM: 39.3329 micro

system setCode (4.6 MB):
- before: ~ 46.64 UNIT
- after: 71.2969 milli (INSECURE -- potential for spam)
- ethereum: N/A
- KSM: 1.6484 KSM

My conclusion at this point is that pallet_transaction_payment isn't going to give us the flexibility we want WRT per-byte-fee that we need to match Ethereum (specifically, it neither multiplies this fee by the WeightToFee multiplier nor the FeeMultiplierUpdate, essentially forcing you to set a constant fee at compile-time).

Some minimal adjustments to this pallet could be made to support our use-case...

@notlesh
Copy link
Contributor Author

notlesh commented Jan 31, 2022

I played with changing pallet_transaction_payment so that it applied a separate WeightToFeePolinomial to the length fee and was able to create a runtime that charged a low fee for a small txn:

  • 12.52 micro UNIT for empty system remark
  • 99.2 UNIT for a 4.6MB runtime upgrade

While this doesn't follow Ethereum txn fee logic, it is a way to reconcile the main problem with this PR. It would require upstream changes or a custom pallet...

@notlesh
Copy link
Contributor Author

notlesh commented Feb 9, 2022

I have opened an issue and PR against Substrate that may resolve the problems mentioned above:

paritytech/substrate#10784
paritytech/substrate#10785

@notlesh
Copy link
Contributor Author

notlesh commented Jun 7, 2022

This PR has been fully superseded by others, including #1576

@notlesh notlesh closed this Jun 7, 2022
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A3-inprogress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants