NeoToken: accept candidate registration via onNEP17Payment #3597

roman-khimov · 2024-11-24T16:01:54Z

Solves two problems:

inability to estimate GAS needed for registerCandidate in a regular way because of its very high fee (more than what normal RPC servers allow)
inability to have MaxBlockSystemFee lower than the registration price which is very high on its own (more than practically possible to execute)

lock9 · 2024-11-25T01:50:06Z

inability to estimate GAS needed for registerCandidate in a regular way because of its very high fee (more than what normal RPC servers allow)

Just a side note: this is also an issue when deploying a smart contract.

Hecate2 · 2024-11-25T03:05:01Z

Could it be better with only await GAS.Burn(engine, Hash, GetRegisterPrice(engine.SnapshotCache)); in RegisterCandidate? We can also Burn instead of AddFee in contract deployment.

roman-khimov · 2024-11-25T06:51:19Z

Just a side note: this is also an issue when deploying a smart contract.

Depends on the server, I think 20 GAS is enough usually.

Could it be better with only await GAS.Burn(engine, Hash, GetRegisterPrice(engine.SnapshotCache)); in RegisterCandidate?

NEO (contract) receives some GAS in this scheme, if it's not burnt NEO will have some GAS on its account which is somewhat weird and was never intended to happened.

shargon · 2024-11-25T09:31:54Z

src/Neo/SmartContract/Native/NeoToken.cs

        [ContractMethod(RequiredCallFlags = CallFlags.States)]
        private bool RegisterCandidate(ApplicationEngine engine, ECPoint pubkey)
        {
-            if (!engine.CheckWitnessInternal(Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash()))
-                return false;
            // In the unit of datoshi, 1 datoshi = 1e-8 GAS
            engine.AddFee(GetRegisterPrice(engine.SnapshotCache));


why not return false if AddFee is not possible? Something like TryAddFee?

This won't help. The appropriate amount of GAS is already in the system fee (which is burnt forever before transaction is executed). If not, it'll fail and still burn whatever GAS is in the system fee.

This won't help. The appropriate amount of GAS is already in the system fee (which is burnt forever before transaction is executed). If not, it'll fail and still burn whatever GAS is in the system fee.

And create a new syscall, TryRegisterCandidate?

How can it help? We need GAS. It either comes from the system fee or via a transfer. The first one burns the fee irrespective of the outcome. The second is what we have here.

The only other option is GAS.Transfer(pub_key_owner, ...) or even direct GAS.Burn(pub_key_owner, ...) call from NEO contract itself, but that's not the NEP-27 way. Transfers should be transfers and NEO deducting something from our GAS balance doesn't look nice.

Por testing you can try and for execute you can not try 😆

I mean if the problem is the gas burn, we can find a global solution for it, maybe we have this problem in a different contract

We can add extra gas for invokes that can only be burned, not consumed by opcodes. If the execution is ApplicationTest or similar allow this behavior, and if this value is negative, return an error by rpc

we can find a global solution for it, maybe we have this problem in a different contract

And we have one. It's NEP-27. Allows to send any amounts of any token to some contract safely.

We can add extra gas for invokes that can only be burned, not consumed by opcodes

Once upon a time there was #2444. And then there was #2560. It can't work, system fee must be burned into ashes prior to execution.

Once upon a time there was #2444. And then there was #2560. It can't work, system fee must be burned into ashes prior to execution.

But this problem was different, this problem is testing the transaction with 1K GAS, but with a limited ExecutionEngine (1GAS for example)

This problem is #3552 (MaxBlockSystemFee) first of all. Solving RPC estimations is just a (very) nice side-effect.

Jim8y · 2024-11-25T11:49:39Z

src/Neo/SmartContract/Native/NeoToken.cs

+
+            await GAS.Burn(engine, Hash, amount);
+
+            RegisterInternal(engine, pubkey);


directly call registerinternal here?

Isn't this a direct call?

I mean, can this be mistakenly triggered? cause there is only a GAS token and amount check.

We need to share some code between the old registerCandidate and new onNEP17Payment (two externally-available contract methods), that's RegisterInternal (which is private).

i understand how you want it to work, but my concern is can we have some way to explicitly telling the system that the transaction is for registration? for instance a string "registerCandidate" in data.

Can be done, data can be anything, for example an array of ["registerCandidate", pubkey]. Here we have the minimal data possible (key only). It's hard to expect any extensions of this callback, but even if needed it still can be done in a compatible way (accept an array or anything else instead of key).

AnnaShaleva

Otherwise LGTM.

src/Neo/SmartContract/Native/NeoToken.cs

Solves two problems: * inability to estimate GAS needed for registerCandidate in a regular way because of its very high fee (more than what normal RPC servers allow) * inability to have MaxBlockSystemFee lower than the registration price which is very high on its own (more than practically possible to execute) Fixes neo-project#3552. Signed-off-by: Roman Khimov <[email protected]>

AnnaShaleva

Otherwise LGTM.

AnnaShaleva · 2024-12-04T19:46:32Z

src/Neo/SmartContract/Native/NeoToken.cs

        [ContractMethod(RequiredCallFlags = CallFlags.States)]
        private bool RegisterCandidate(ApplicationEngine engine, ECPoint pubkey)
        {
-            if (!engine.CheckWitnessInternal(Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash()))
-                return false;
            // In the unit of datoshi, 1 datoshi = 1e-8 GAS
            engine.AddFee(GetRegisterPrice(engine.SnapshotCache));


Sorry for nitpicking, but I've just noticed this issue: from the code point there's almost no difference, you've just moved the common code to a separate method. But execution result technically may be different (comparing the old and the new behaviour of registerCandidate): consider transaction that calls registerCandidate and has insufficient system fee and is not witnessed by the candidate:

Old implementation will return false after witness check, and execution state will be HALT.

New implementation will panic on engine.AddFee, and execution state will be FAULT.

So technically, it's a state diff. Practically, there likely no such transaction in the public networks (but we may always submit one!). Anyway, I'd suggest to get the old code back, to avoid possible state diff.

Really nice catch. FAULT can be leveraged on its own, but with 1000 GAS paid you can even have different HALTs (the result of System.Runtime.GasLeft can tell the difference for the rest of the malicious script). This can be fixed by duplicating witness check in the RegisterCandidate (in a hardfork-dependent way), but I won't update this PR now since there are other ideas on how to solve this (which were not even discussed anywhere). So if that's not the "right" approach, I'll close the PR anyway.

shargon · 2024-12-05T08:33:58Z

src/Neo/SmartContract/Native/NeoToken.cs

+            var pubkey = ECPoint.DecodePoint(data.GetSpan(), ECCurve.Secp256r1);
+
+            if (!RegisterInternal(engine, pubkey))
+                throw new InvalidOperationException("failed to register candidate");


We don't burn it before throw?

throw is like ABORT here, transaction will end up in FAULT state, nothing happened.

a reason why a want to update these exception name to uncatchableexception~~~

Uncatchable can also be misunderstood, I can catch it in various places, it's just that VM/contracts are not even aware of this happening.

uncatachable should be named as normal. Name should be VMException with inner exception of normal name. Thats kind of how it done currently in the vm.

roman-khimov mentioned this pull request Nov 24, 2024

NEO candidate registration via NEP-27 callback nspcc-dev/neo-go#3700

Open

cschuchardt88 added the Waiting for Review label Nov 24, 2024

shargon reviewed Nov 25, 2024

View reviewed changes

Jim8y reviewed Nov 25, 2024

View reviewed changes

Jim8y mentioned this pull request Nov 25, 2024

Add hardfork HF_Echidna #3454

Draft

9 tasks

AnnaShaleva reviewed Nov 26, 2024

View reviewed changes

src/Neo/SmartContract/Native/NeoToken.cs Outdated Show resolved Hide resolved

src/Neo/SmartContract/Native/NeoToken.cs Outdated Show resolved Hide resolved

roman-khimov force-pushed the nep27-registration branch from 1f44969 to 8f358d6 Compare November 29, 2024 19:46

roman-khimov requested review from AnnaShaleva, shargon and Jim8y November 29, 2024 19:47

Jim8y added the Hardfork label Nov 30, 2024

AnnaShaleva reviewed Dec 4, 2024

View reviewed changes

shargon reviewed Dec 5, 2024

View reviewed changes

Jim8y mentioned this pull request Dec 5, 2024

[Dec.5] Core Dev Regular Meeting Proposal Discussion #3608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NeoToken: accept candidate registration via onNEP17Payment #3597

NeoToken: accept candidate registration via onNEP17Payment #3597

roman-khimov commented Nov 24, 2024

lock9 commented Nov 25, 2024

Hecate2 commented Nov 25, 2024 •

edited

Loading

roman-khimov commented Nov 25, 2024

shargon Nov 25, 2024 •

edited

Loading

roman-khimov Nov 25, 2024

shargon Nov 25, 2024

roman-khimov Nov 25, 2024

shargon Nov 25, 2024

shargon Nov 25, 2024

shargon Nov 25, 2024

roman-khimov Nov 26, 2024

shargon Nov 27, 2024

roman-khimov Nov 27, 2024

Jim8y Nov 25, 2024

roman-khimov Nov 25, 2024

Jim8y Nov 25, 2024

roman-khimov Nov 25, 2024

Jim8y Nov 27, 2024

roman-khimov Nov 27, 2024

AnnaShaleva left a comment

AnnaShaleva left a comment

AnnaShaleva Dec 4, 2024 •

edited

Loading

roman-khimov Dec 4, 2024

shargon Dec 5, 2024

roman-khimov Dec 5, 2024

Jim8y Dec 5, 2024

roman-khimov Dec 5, 2024

cschuchardt88 Dec 5, 2024


		await GAS.Burn(engine, Hash, amount);

		RegisterInternal(engine, pubkey);

NeoToken: accept candidate registration via onNEP17Payment #3597

Are you sure you want to change the base?

NeoToken: accept candidate registration via onNEP17Payment #3597

Conversation

roman-khimov commented Nov 24, 2024

lock9 commented Nov 25, 2024

Hecate2 commented Nov 25, 2024 • edited Loading

roman-khimov commented Nov 25, 2024

shargon Nov 25, 2024 • 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

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

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

Choose a reason for hiding this comment

AnnaShaleva left a comment

Choose a reason for hiding this comment

AnnaShaleva left a comment

Choose a reason for hiding this comment

AnnaShaleva Dec 4, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hecate2 commented Nov 25, 2024 •

edited

Loading

shargon Nov 25, 2024 •

edited

Loading

AnnaShaleva Dec 4, 2024 •

edited

Loading