auto-claim gas if it's needed #2008

shargon · 2020-10-15T14:45:41Z

shargon · 2020-10-15T14:59:54Z

@neo-project/ngd-shanghai Could you help me testing it?

Qiao-Jin · 2020-10-16T07:41:31Z

How about here:

Line 309 in 028f4b5

    
           balances_gas = accounts.Select(p => (Account: p, Value: NativeContract.GAS.BalanceOf(snapshot, p))).Where(p => p.Value.Sign > 0).ToList();

Besides, how about an overide func GAS.BalanceOf which takes NEO.UnclaimedGas into account?

shargon · 2020-10-16T08:22:18Z

Besides, how about an overide func GAS.BalanceOf which takes NEO.UnclaimedGas into account?

It's a good idea :), we can change it later if this work.

erikzhang · 2020-10-16T09:49:28Z

If the gas is not enough, users can buy some on the exchange.

shargon · 2020-10-29T11:29:31Z

If the gas is not enough, users can buy some on the exchange.

Yes, but moving the logic from balanceChanging to OnPersist we can avoid the first use problem without buy Gas, only NEO.

roman-khimov · 2020-10-30T14:04:00Z

src/neo/Ledger/TransactionVerificationContext.cs

-            return balance >= fee;
+            if (balance >= fee) return true;
+
+            BigInteger unclaimed = NativeContract.NEO.UnclaimedGas(snapshot, tx.Sender, snapshot.Height);


This is in the hot path for transaction verification and after #1848 this call can be quite expensive (with a number of DB accesses).

But I think this is necessary anyway @roman-khimov , because we must know if someone has enough funds for transfer, during Verification... it really costs to calculate, but we don't need precise value. Maybe, if we have some caching on unclaimedGas values, we could call some method EnoughUnclaimedGas(VALUE), so that if caching already has enough, we don't recalculate. Truth is, we just need to know if Unclaimed is bigger than VALUE, not its precise value.

If it has funds, there will be no problem, otherwise, we need to check it.

src/neo/SmartContract/Native/Tokens/NeoToken.cs

vncoelho · 2020-10-30T14:28:20Z

src/neo/SmartContract/Native/Tokens/GasToken.cs

@@ -27,6 +27,7 @@ protected override void OnPersist(ApplicationEngine engine)
            long totalNetworkFee = 0;
            foreach (Transaction tx in engine.Snapshot.PersistingBlock.Transactions)
            {
+                NEO.DistributeGas(engine, tx.Sender);


@shargon, maybe it should only distributeGAS for the tx.Sender that needs it.
In this sense, perhaps we could have a flag before return balance + unclaimed >= fee;

Please, take a look again

…nclaimed-gas

shargon · 2020-11-14T12:35:21Z

@superboyiii could you test it please?

vncoelho · 2021-01-06T16:02:06Z

If the gas is not enough, users can buy some on the exchange.

It is a barrier that we can avoid, @erikzhang, at least for a basic transaction, if the user holds NEO for some couple of time.

…nclaimed-gas

shargon · 2021-01-07T08:53:58Z

@erikzhang As @vncoelho said, it will help with adoption, impatient users always can buy GAS.

superboyiii · 2021-01-30T14:25:25Z

Done! it should work with neo-node master

I'm testing it on latest master. Not sure how to test it. I saw you changed CheckTransaction() to add unclaimed gas into calculation. But cli is not using this, I mean I supposed to use send command and it will calculate netfee within the claimed + unclaimed, if not enough claimed, then claim the unclaimed to be fee.

shargon · 2021-01-31T09:30:57Z

@superboyiii please check it again

superboyiii · 2021-01-31T12:28:35Z

neo/src/neo/Wallets/Wallet.cs

Line 323 in faa698e

    
           balances_gas = accounts.Select(p => (Account: p, Value: NativeContract.GAS.BalanceOf(snapshot, p))).Where(p => p.Value.Sign > 0).ToList();

@shargon We're using this MakeTransaction(TransferOutput[] outputs, UInt160 from = null, Signer[] cosigners = null) for send command. Need using unclaimedGas as balance as well.
https://github.com/neo-project/neo-node/blob/a7f06cab3f5cb3099ba7e75ba3d1e18d6eda337d/neo-cli/CLI/MainService.Wallet.cs#L475

superboyiii · 2021-02-02T10:32:27Z

I could make Tx successfully but seems it has been rejected by mempool verification.

You forget here:

superboyiii · 2021-02-02T11:45:07Z

I think for neo-core it's OK now, but we need modify more on neo-node and neo-modules.
For example: https://github.com/neo-project/neo-node/blob/f0fc56690388167fa472b843a659d61fd135238e/neo-cli/CLI/MainService.cs#L480
Vote still will be failed.
I will not approve this beacuse we'll release Preview5 tommorrow. It could be merged in next version.

erikzhang · 2021-02-02T11:56:43Z

I'm not sure if we really need it. Originally, if there was no GAS in my account, I know the transaction would fail. Now even if I transfer all the GAS away, it may still be claimed automatically.

superboyiii · 2021-02-02T12:04:48Z

I'm not sure if we really need it. Originally, if there was no GAS in my account, I know the transaction would fail. Now even if I transfer all the GAS away, it may still be claimed automatically.

unclaimed gas + claimed gas = your gas. I think it makes sense. No need to ask user know how much they need to claim because the claim mechanism is never important. If we can claim for user, we just do it. And CLI should show unclaimed gas + claimed gas as balance and use it as balance.

shargon · 2021-02-02T12:09:24Z

Now even if I transfer all the GAS away, it may still be claimed automatically.

But it is a benefit more than a problem. Otherwise, in many cases this gas was like burned.

superboyiii · 2021-02-02T12:20:16Z

It benefits a lot in initial NEO transaction. Exchange don't allow a little gas withdraw, that means you need buy a certain amount of GAS just for one transaction. It's not good for NEO flowability and it makes exchange difficult for initial NEO3 technicial integration.

roman-khimov · 2021-02-02T13:06:51Z

On a real network we'll get 100 tx in a block and all of them will come from different addresses. So we'll distribute GAS for all of them and invoke onNEP17Payment for all of them (which can fail BTW). Then in the next block we'll get another 100 tx and something like half of them will come from the same addresses as in previous block, so we'll distribute GAS... Practically it means that for some active addresses GAS will be distributed with every block. My concern is that it can add some substantial overhead and a lot of not really useful (but still happening) invocations and state changes.

And accepting a transaction which can't really pay according to the state we have at the current height is still a little scary to me, even though it doesn't seem to create any issues if GAS is to be distributed before writing the commission off the balance.

src/neo/Ledger/TransactionVerificationContext.cs

vncoelho · 2021-02-02T13:58:24Z

src/neo/SmartContract/Native/GasToken.cs

            foreach (Transaction tx in engine.PersistingBlock.Transactions)
            {
+                if (distributed.Add(tx.Sender)) NEO.DistributeGas(engine, tx.Sender);


This is just to ensure that it is distributed only once per Persist, right?

Isn't there any need to avoid the

neo/src/neo/SmartContract/Native/NeoToken.cs

Line 45 in 02fdcc6

protected override void OnBalanceChanging(ApplicationEngine engine, UInt160 account, NeoAccountState state, BigInteger amount)

?

If the transfer also changes NEO's balance?

We're running this line in OnPersist context. It invokes GAS.Mint. It calls PostTransfer. Which calls onNEP17Payment. That fails. What are we gonna do then? Fail persisting a block? Not an option. Ignore this problem and fail GAS distribution? Fine, except some storage changes are already in-flight in this context and, most importantly, tx.Sender may just not have enough GAS for the Burn() one line below. So, what are the options we have?

@roman-khimov it's the same problem here

neo/src/neo/SmartContract/Native/NeoToken.cs

Line 161 in bfbb78b

GAS.Mint(engine, account, gasPerBlock * CommitteeRewardRatio / 100, false);

I think that we should not rise onNEP17Payment during OnPersist

It's not. This invocation specifically has false for callOnPayment. And we only have simple standard accounts in the committee (their keys are known).

I'll put "false" here too

I don't think it's legal to do this! Unlike committee members transaction sender can be anything, including deployed contract. And deployed contract must be invoked on this mint.

Yes, I we merge it, the generated gas will come to not call onNEP17Payment, before this PR it does....

superboyiii · 2021-02-25T07:15:35Z

@erikzhang @shargon @roman-khimov @vncoelho Let's move on? If we'd like to do this, we shall merge it ASAP and modify neo-node and neo-modules before RC1.

superboyiii · 2021-03-02T05:56:18Z

@erikzhang Need your review.

erikzhang · 2021-03-02T06:09:25Z

We're running this line in OnPersist context. It invokes GAS.Mint. It calls PostTransfer. Which calls onNEP17Payment. That fails. What are we gonna do then? Fail persisting a block? Not an option. Ignore this problem and fail GAS distribution? Fine, except some storage changes are already in-flight in this context and, most importantly, tx.Sender may just not have enough GAS for the Burn() one line below. So, what are the options we have?

I agree with @roman-khimov. And I think we shouldn't merge this PR.

shargon added 2 commits October 15, 2020 16:40

Claim GAS before spend

2cee028

Optimize

14ee224

shargon requested a review from erikzhang October 15, 2020 14:45

shargon added 2 commits October 15, 2020 16:54

Rename

2a29c0d

Allow to create tx

a9e95ed

shargon added 3 commits October 29, 2020 12:21

Merge branch 'master' into allow-to-use-unclaimed-gas

31ab7bd

Reduce changes

8fb47bd

Auto DistributeGas

3fedd30

shargon marked this pull request as ready for review October 29, 2020 11:28

Clean code

6d8f941

shargon requested review from igormcoelho and Tommo-L October 29, 2020 11:35

Merge branch 'master' into allow-to-use-unclaimed-gas

133eae9

roman-khimov reviewed Oct 30, 2020

View reviewed changes

vncoelho reviewed Oct 30, 2020

View reviewed changes

shargon added 3 commits November 1, 2020 11:10

Some fixes

a89c9a3

dotnet format

3512d93

Merge remote-tracking branch 'neo-project/master' into allow-to-use-u…

5484f82

…nclaimed-gas

vncoelho closed this Jan 6, 2021

vncoelho reopened this Jan 6, 2021

Merge remote-tracking branch 'neo-project/master' into allow-to-use-u…

8a0825f

…nclaimed-gas

shargon mentioned this pull request Jan 15, 2021

node: Can't send all gas to an address in neo-cli neo-project/neo-node#717

Closed

Add unclaimed gas

faa698e

Tommo-L previously approved these changes Jan 31, 2021

View reviewed changes

Other fix

652e2c8

shargon dismissed Tommo-L’s stale review via 652e2c8 February 1, 2021 08:19

shargon added 2 commits February 2, 2021 11:37

Fix

973ab49

Merge branch 'master' into allow-to-use-unclaimed-gas

bf68a21

Merge branch 'master' into allow-to-use-unclaimed-gas

bfbb78b

vncoelho reviewed Feb 2, 2021

View reviewed changes

src/neo/Ledger/TransactionVerificationContext.cs Show resolved Hide resolved

vncoelho reviewed Feb 2, 2021

View reviewed changes

Merge branch 'master' into allow-to-use-unclaimed-gas

8234327

roman-khimov mentioned this pull request Feb 25, 2021

3.0 GAS distribution and rounding effects #2364

Closed

Fixes

3701664

shargon closed this Mar 2, 2021

shargon deleted the allow-to-use-unclaimed-gas branch March 2, 2021 07:35

shargon mentioned this pull request Apr 22, 2021

Add flash loan support to the GAS native contract #2441

Closed

roman-khimov mentioned this pull request Aug 17, 2021

Practical transaction sponsoring #2577

Open

roman-khimov mentioned this pull request Nov 14, 2023

auto-claim gas on tx submission #1078

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto-claim gas if it's needed #2008

auto-claim gas if it's needed #2008

shargon commented Oct 15, 2020 •

edited

Loading

shargon commented Oct 15, 2020

Qiao-Jin commented Oct 16, 2020

shargon commented Oct 16, 2020 •

edited

Loading

erikzhang commented Oct 16, 2020

shargon commented Oct 29, 2020 •

edited

Loading

roman-khimov Oct 30, 2020

igormcoelho Oct 30, 2020 •

edited

Loading

shargon Nov 14, 2020

vncoelho Oct 30, 2020

shargon Nov 1, 2020

shargon commented Nov 14, 2020

vncoelho commented Jan 6, 2021 •

edited

Loading

shargon commented Jan 7, 2021

superboyiii commented Jan 30, 2021 •

edited

Loading

shargon commented Jan 31, 2021

superboyiii commented Jan 31, 2021 •

edited

Loading

superboyiii commented Feb 2, 2021

superboyiii commented Feb 2, 2021

erikzhang commented Feb 2, 2021

superboyiii commented Feb 2, 2021

shargon commented Feb 2, 2021

superboyiii commented Feb 2, 2021 •

edited

Loading

roman-khimov commented Feb 2, 2021 •

edited

Loading

vncoelho Feb 2, 2021

vncoelho Feb 2, 2021

roman-khimov Feb 25, 2021

shargon Feb 26, 2021 •

edited

Loading

roman-khimov Feb 26, 2021

shargon Feb 26, 2021

roman-khimov Feb 26, 2021

shargon Feb 26, 2021

superboyiii commented Feb 25, 2021 •

edited

Loading

superboyiii commented Mar 2, 2021

erikzhang commented Mar 2, 2021

auto-claim gas if it's needed #2008

auto-claim gas if it's needed #2008

Conversation

shargon commented Oct 15, 2020 • edited Loading

shargon commented Oct 15, 2020

Qiao-Jin commented Oct 16, 2020

shargon commented Oct 16, 2020 • edited Loading

erikzhang commented Oct 16, 2020

shargon commented Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

igormcoelho Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Nov 14, 2020

vncoelho commented Jan 6, 2021 • edited Loading

shargon commented Jan 7, 2021

superboyiii commented Jan 30, 2021 • edited Loading

shargon commented Jan 31, 2021

superboyiii commented Jan 31, 2021 • edited Loading

superboyiii commented Feb 2, 2021

superboyiii commented Feb 2, 2021

erikzhang commented Feb 2, 2021

superboyiii commented Feb 2, 2021

shargon commented Feb 2, 2021

superboyiii commented Feb 2, 2021 • edited Loading

roman-khimov commented Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superboyiii commented Feb 25, 2021 • edited Loading

superboyiii commented Mar 2, 2021

erikzhang commented Mar 2, 2021

shargon commented Oct 15, 2020 •

edited

Loading

shargon commented Oct 16, 2020 •

edited

Loading

shargon commented Oct 29, 2020 •

edited

Loading

igormcoelho Oct 30, 2020 •

edited

Loading

vncoelho commented Jan 6, 2021 •

edited

Loading

superboyiii commented Jan 30, 2021 •

edited

Loading

superboyiii commented Jan 31, 2021 •

edited

Loading

superboyiii commented Feb 2, 2021 •

edited

Loading

roman-khimov commented Feb 2, 2021 •

edited

Loading

shargon Feb 26, 2021 •

edited

Loading

superboyiii commented Feb 25, 2021 •

edited

Loading