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

NeoToken: accept candidate registration via onNEP17Payment #3597

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

roman-khimov
Copy link
Contributor

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.

@lock9
Copy link
Contributor

lock9 commented Nov 25, 2024

  • 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
Copy link
Contributor

Hecate2 commented Nov 25, 2024

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
Copy link
Contributor Author

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.

[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));
Copy link
Member

@shargon shargon Nov 25, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


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

RegisterInternal(engine, pubkey);
Copy link
Contributor

Choose a reason for hiding this comment

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

directly call registerinternal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this a direct call?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@Jim8y Jim8y mentioned this pull request Nov 25, 2024
11 tasks
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

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

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

src/Neo/SmartContract/Native/NeoToken.cs Show resolved Hide resolved
var pubkey = ECPoint.DecodePoint(data.GetSpan(), ECCurve.Secp256r1);

if (!RegisterInternal(engine, pubkey))
throw new InvalidOperationException("failed to register candidate");
Copy link
Member

Choose a reason for hiding this comment

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

We don't burn it before throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

// 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) &&
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Member

shargon commented Dec 12, 2024

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 <[email protected]>
@NGDAdmin NGDAdmin merged commit a784c41 into neo-project:HF_Echidna Dec 20, 2024
2 of 6 checks passed
AnnaShaleva added a commit that referenced this pull request Dec 23, 2024
AnnaShaleva added a commit that referenced this pull request Dec 23, 2024
AnnaShaleva added a commit that referenced this pull request Dec 23, 2024
AnnaShaleva added a commit that referenced this pull request Dec 23, 2024
#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 <[email protected]>
AnnaShaleva added a commit that referenced this pull request Dec 23, 2024
#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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants