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

Add Fee Ratio #2032

Closed
wants to merge 29 commits into from
Closed

Add Fee Ratio #2032

wants to merge 29 commits into from

Conversation

Qiao-Jin
Copy link
Contributor

@Qiao-Jin Qiao-Jin commented Oct 28, 2020

Close #1875 & another implementation of dynamic "fee ratio" part in #1453.

This PR aims to implement a dynamic fee ratio which can be used to adjust real gas cost of transactions, i.e. when ratio = 2, overall transaction gas cost will be half of original value. This ratio can be adjusted by committee members if needed.

Jin Qiao and others added 2 commits October 28, 2020 11:36
@@ -459,7 +460,7 @@ private void Persist(Block block)
clonedSnapshot.Transactions.Add(tx.Hash, state);
clonedSnapshot.Transactions.Commit();

using (ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, tx, clonedSnapshot, tx.SystemFee))
using (ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, tx, clonedSnapshot, tx.SystemFee * ratio))
Copy link
Member

Choose a reason for hiding this comment

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

Very good. I like this design.
Thus, ratio 0.5 makes the network more expensive.

@@ -294,7 +294,7 @@ public virtual VerifyResult VerifyStateDependent(StoreView snapshot, Transaction
foreach (TransactionAttribute attribute in Attributes)
if (!attribute.Verify(snapshot, this))
return VerifyResult.Invalid;
long net_fee = NetworkFee - Size * NativeContract.Policy.GetFeePerByte(snapshot);
long net_fee = NetworkFee * NativeContract.Policy.GetFeeRatio(snapshot) - Size * NativeContract.Policy.GetFeePerByte(snapshot);
Copy link
Member

@vncoelho vncoelho Oct 28, 2020

Choose a reason for hiding this comment

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

It is the same, but maybe:
(NetworkFee * NativeContract.Policy.GetFeeRatio(snapshot)) - (Size * NativeContract.Policy.GetFeePerByte(snapshot)), to make it clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have fixed, please have a check

@Qiao-Jin
Copy link
Contributor Author

In the latest commit I changed the definition of FeeRatio. After changing the 'real' price of each opcode/interop will be as follows:

original value * FeeRatio / 100

Or in other word, FeeRatio means how many percent of original price the current price is.

Initial FeeRatio will be 100. FeeRatio will range from 1 to 10000.

@erikzhang
Copy link
Member

I think the easiest way is to set the price of NOP to 1, and then the initial ratio is 30.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Oct 30, 2020

I think the easiest way is to set the price of NOP to 1, and then the initial ratio is 30.

This PR provides a method to set overall fee dynamically. If we don't set such an ratio and want to change tx fee later, we would need a hard folk.

@superboyiii superboyiii mentioned this pull request Nov 2, 2020
44 tasks
@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 2, 2020

Can we merge this one?

@erikzhang
Copy link
Member

I think the easiest way is to set the price of NOP to 1, and then the initial ratio is 30.

We should implement this first.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 2, 2020

I think the easiest way is to set the price of NOP to 1, and then the initial ratio is 30.

We should implement this first.

Do we need a hard-coded ratio or a dynamic one like described in this PR?

@erikzhang
Copy link
Member

Do we need a hard-coded ratio or a dynamic one like described in this PR?

We need a dynamic one with the initial value 30.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 3, 2020

We need a dynamic one with the initial value 30.

Done

@erikzhang
Copy link
Member

This ratio should not affect the storage price. And many native contract methods are based on the storage size.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 4, 2020

This ratio should not affect the storage price. And many native contract methods are based on the storage size.

Have done

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

In which case we want to use withoutRatio for witnesses ?

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 5, 2020

In which case we want to use withoutRatio for witnesses ?

For opcodes & interops which should not be influenced by fee ratio i.e. StoragePrice

@shargon
Copy link
Member

shargon commented Nov 5, 2020

This ratio should not affect the storage price

If we want to pay 1 USD per byte (for example), we will need to affect the storage price, isn't it?

@roman-khimov
Copy link
Contributor

If we want to pay 1 USD per byte (for example), we will need to affect the storage price, isn't it?

I think any storage-related prices can be derived from FeePerByte (StoragePrice = 100 * FeePerByte, etc). And then syscalls can make any proportions using FeePerByte (for storage) and NOP fee (for execution). So they could specify an amount of work they're doing in bytes/nops (minimal units of work) and then the ApplicationEngine would translate it to GAS according to current GAS value for these basic units of work.

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 6, 2020

I think any storage-related prices can be derived from FeePerByte (StoragePrice = 100 * FeePerByte, etc)

FeePerByte is for network transmission and StoragePrice is for storage, they are for different usage & don't have direct relationship, so I don't think it's good to keep them synchronized.

@Qiao-Jin
Copy link
Contributor Author

I rewrote the fee logic so that fee ratio is applied to each opcode & interops. Scenarios for func Transaction.VerifyIndependent where input snapshot is null are also considered. Original logic of func VerifyWitnesses is restored to make corresponding logic simpler.

@@ -67,10 +69,12 @@ protected ApplicationEngine(TriggerType trigger, IVerifiable container, StoreVie
this.ScriptContainer = container;
this.Snapshot = snapshot;
this.gas_amount = gas;
this.feeRatio = snapshot is null ? 1 : NativeContract.Policy.GetFeeRatio(Snapshot);
Copy link
Member

Choose a reason for hiding this comment

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

The verifications always use the ratio 1?

Copy link
Contributor Author

@Qiao-Jin Qiao-Jin Nov 11, 2020

Choose a reason for hiding this comment

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

This serves scenarios such as Transaction.VerifyIndependent where input snapshot is null. In such cases we should use minimum fee ratio. This is because if fee ratio is really set to 1, using other values might drop transactions which are actually valid. And the "real" expense used by verifying witness will be checked afterwards in Transaction.VerifyDependent so that no invalid transaction will get on chain.

Copy link
Member

Choose a reason for hiding this comment

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

But 1 will cause attacks.

Copy link
Contributor Author

@Qiao-Jin Qiao-Jin Nov 11, 2020

Choose a reason for hiding this comment

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

But 1 will cause attacks.

In PR #2054, we will check whether a witness's VerificationScript is StandardContract, so that attack can be blocked.

@shargon
Copy link
Member

shargon commented Nov 11, 2020

I think that we should decide if we want to work on this version or #2045
Can we use #1875 for this discussion?

@Qiao-Jin
Copy link
Contributor Author

Qiao-Jin commented Nov 12, 2020

I think that we should decide if we want to work on this version or #2045

I personally think this PR works better because:

(1) Interop services are also considered. As VM expense = fee of opcodes + fee of interops, interops should also be considered naturally.

(2) In this PR hard-coded limitations are also considered such as TestModeFee, OracleRequestPrice, etc. These limitations should also be considered in case of fee ratio changing.

Can we use #1875 for this discussion?

Sure of course.

@Qiao-Jin Qiao-Jin closed this Nov 18, 2020
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.

Redefine opcode prices and make them adjustable
6 participants