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 😆

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.

The behaviour is different, before if was wrong signed, the fee was not burned

The system fee is already gone by the time this executes (whether it's sufficient or not). The only thing left here is #3597 (comment), I can fix it, OK.

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

AnnaShaleva

Otherwise LGTM.

src/Neo/SmartContract/Native/NeoToken.cs

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.

shargon · 2024-12-12T14:14:14Z

src/Neo/SmartContract/Native/NeoToken.cs

+            // This check can be removed post-Echidna if compatible,
+            // RegisterInternal does this anyway.
+            var index = engine.PersistingBlock?.Index ?? Ledger.CurrentIndex(engine.SnapshotCache);
+            if (!engine.ProtocolSettings.IsHardforkEnabled(Hardfork.HF_Echidna, index) &&


Maybe we should allow it also with Echidna, it doesn't hurt

This will require us keeping it forever then (in which case it's easier to duplicate the check). But my intention was to drop this check here eventually (keeping it in RegisterInternal only). But I'm OK with any option.

to drop this check here eventually

Vote up for this, we may remove this code after transition to Echidna, and the code will be more pretty. But we need an issue for that in order not to forget.

roman-khimov · 2024-12-12T14:24:32Z

BTW, @shargon, did you do 6d2d121 manually? Because it was formatted a bit differently originally and then I just did dotnet-format (since I have no idea how to format C# anyway and Emacs is usually doing it wrong). The result was a bit weird to me (and it's in cedae6a), but I can't argue with dotnet-format. Now I'm not sure if it's configured correctly or is this the expected behavior (it tries to format some code differently in the same file, btw, but I've not committed that to avoid distraction).

shargon · 2024-12-12T14:26:22Z

BTW, @shargon, did you do 6d2d121 manually? Because it was formatted a bit differently originally and then I just did dotnet-format (since I have no idea how to format C# anyway and Emacs is usually doing it wrong). The result was a bit weird to me (and it's in cedae6a), but I can't argue with dotnet-format. Now I'm not sure if it's configured correctly or is this the expected behavior (it tries to format some code differently in the same file, btw, but I've not committed that to avoid distraction).

Yes it was manually... maybe github use tabs...

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 <roman@nspcc.ru>

Follow #3597. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

#3597 introduces `onNEP17Payment` handler to native NeoToke contract starting from Echidna hardfork. We need to update the list of supported standards respectively. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

#3643) #3597 introduces `onNEP17Payment` handler to native NeoToke contract starting from Echidna hardfork. We need to update the list of supported standards respectively. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * add hardofork HF_Echidna * Add entries to `Designation` event (#3397) * Add entries to Designation event * Change to HF_Echidna * Add UT * Add count * [Neo Core StdLib] Add Base64url (#3453) * add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead. * format * Fixed typo * Added back #3397 * Fixed tests * fixed global.json * Update src/Neo/Neo.csproj * Update src/Neo/Neo.csproj * [`Fix`]: integer overflow in `JumpTable.SubStr ` (#3496) * fix: int overflow in SubStr * fix: int overflow in SubStr * format * Versioning change * Clean * Rename * Show change * Space * remove duplicated lines in gitignroe --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> * Fix NEO callstates (#3599) * Allow callstates to use HF * Rename to method * Other rename * Change the way * Reduce changes * Reduce changes * Adapt name always * Avoid string when only is lower the first char * UT * Test all * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Reuse Load from stream * Unify * Fix default logic * Change ContractMethod to allowMultiple * Use LowerInvariant * Move CheckingHardfork * Remove optional arg * Fix build * Avoid file not found error --------- Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * fix tests error (#3636) * fux build error * Update src/Neo/SmartContract/ApplicationEngine.cs --------- Co-authored-by: Shargon <shargon@gmail.com> * NeoToken: accept candidate registration via onNEP17Payment (#3597) 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 #3552. Signed-off-by: Roman Khimov <roman@nspcc.ru> * specify the argument exception information. * Fix Ut (#3635) * NeoToken: add NEP-27 to supported standards list starting from Echidna (#3643) #3597 introduces `onNEP17Payment` handler to native NeoToke contract starting from Echidna hardfork. We need to update the list of supported standards respectively. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * ut: fix HF_Echidna unit tests (#3646) * Fix UT * Update src/Neo/ProtocolSettings.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: nan01ab <yjcc201374@outlook.com> * Update src/Neo/ProtocolSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * [Core Add] Add support to Ed25519 (#3507) * fix unnecessary change * Clean using --------- Co-authored-by: Fernando Diaz Toledano <shargon@gmail.com> * Fix `HF_Echidna` comments (#3679) * Fix obsolete * Fix https://github.com/neo-project/neo/pull/3454/files#r1912152270 * Fix comment * Update RoleManagement.cs * Unset HF_Echidna * Revert getTransaction * Revert verifyWithECDsa * format --------- Signed-off-by: Roman Khimov <roman@nspcc.ru> Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: nan01ab <yjcc201374@outlook.com> Co-authored-by: Roman Khimov <roman@nspcc.ru> Co-authored-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>

roman-khimov mentioned this pull request Nov 24, 2024

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

Merged

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

Merged

11 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

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

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

roman-khimov force-pushed the nep27-registration branch from 8f358d6 to cedae6a Compare December 12, 2024 09:21

roman-khimov requested review from AnnaShaleva and shargon December 12, 2024 09:21

shargon reviewed Dec 12, 2024

View reviewed changes

AnnaShaleva approved these changes Dec 18, 2024

View reviewed changes

AnnaShaleva mentioned this pull request Dec 18, 2024

[Dec.19] Core Dev Regular Meeting Proposal Discussion #3632

Closed

Jim8y approved these changes Dec 19, 2024

View reviewed changes

Jim8y requested review from nan01ab, shargon and cschuchardt88 December 19, 2024 07:33

roman-khimov force-pushed the nep27-registration branch from 6d2d121 to a1ff32c Compare December 19, 2024 09:02

shargon approved these changes Dec 19, 2024

View reviewed changes

nan01ab approved these changes Dec 20, 2024

View reviewed changes

NGDAdmin merged commit a784c41 into neo-project:HF_Echidna Dec 20, 2024
2 of 6 checks passed

roman-khimov mentioned this pull request Dec 20, 2024

Remove this deprecated code in 3.9.0 #3260

Open

AnnaShaleva mentioned this pull request Dec 23, 2024

Consider removing compatibility code from Neo's registerCandidate nspcc-dev/neo-go#3774

Open

AnnaShaleva added a commit that referenced this pull request Dec 23, 2024

Neo: add NEP-27 to the list of supported standards starting from Echidna

4a393cb

Follow #3597. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit that referenced this pull request Dec 23, 2024

NeoToken: add NEP-27 to supported standards list starting from Echidna

991ba34

Follow #3597. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva added a commit that referenced this pull request Dec 23, 2024

NeoToken: add NEP-27 to supported standards list starting from Echidna

f3ec7a8

Follow #3597. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva mentioned this pull request Dec 23, 2024

NeoToken: add NEP-27 to supported standards list starting from Echidna #3643

Merged

15 tasks

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

roman-khimov Nov 26, 2024

shargon Nov 27, 2024

roman-khimov Nov 27, 2024

shargon Dec 12, 2024

roman-khimov Dec 12, 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

shargon Dec 5, 2024

roman-khimov Dec 5, 2024

Jim8y Dec 5, 2024

roman-khimov Dec 5, 2024

cschuchardt88 Dec 5, 2024

shargon Dec 12, 2024

roman-khimov Dec 12, 2024

AnnaShaleva Dec 18, 2024

roman-khimov commented Dec 12, 2024

shargon commented Dec 12, 2024


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

		RegisterInternal(engine, pubkey);

NeoToken: accept candidate registration via onNEP17Payment #3597

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

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

roman-khimov commented Dec 12, 2024

shargon commented Dec 12, 2024

Hecate2 commented Nov 25, 2024 •

edited

Loading

shargon Nov 25, 2024 •

edited

Loading