neo-project / neo-modules

MIT License
60 stars 100 forks source link

RpcServer: return `RpcError.InsufficientNetworkFee` error where appropriate #889

Open AnnaShaleva opened 6 months ago

AnnaShaleva commented 6 months ago

Describe the bug After https://github.com/neo-project/neo-modules/pull/815 merge, RpcError.InsufficientFunds (with code -511) is returned on the incoming transaction verification when:

  1. Sender's balance is not enough to pay for all his transactions in the pool
  2. Sender's balance is not enough to pay for the particular incoming transaction's fees

Whereas RPC errors proposal postulates that -511 Insufficient funds error should be returned only in case 1, i.e. when:

| -511 || Insufficient funds || Sender doesn't have enough GAS to pay for all currently pooled transactions.

And all other cases of insufficient sender's balance should be covered by generic -500 error:

| -500 || Unclassified verification error|| Anything that can't be expressed by other codes.

To Reproduce Steps to reproduce the behavior:

  1. Check the transaction verification code from core: https://github.com/neo-project/neo/blob/b05501af882a0d1f2a1a7841c6ddc4d0504e5fc1/src/Neo/Network/P2P/Payloads/Transaction.cs#L368-L395
  2. Check the error conversion code on the RPC server side: https://github.com/neo-project/neo-modules/blob/253a77e2b88f139bebb57779e4f3ca8837f92f68/src/RpcServer/RpcServer.Node.cs#L97-L100

Expected behavior

  1. We need to modify TransactionVerificationContext.CheckTransaction so that it returns some new special error, this error should be detached from VerifyResult.InsufficientFunds and ported to other components (ref. https://github.com/neo-project/neo/blob/b05501af882a0d1f2a1a7841c6ddc4d0504e5fc1/src/Neo/Network/P2P/Payloads/Transaction.cs#L368.). This special error should be converted to RpcError with -511 code on the RPC server side whereas the rest of VerifyResult.InsufficientFunds cases should be converted to RpcError with -500 code.
  2. We may need to adjust error text of RpcError.ExpiredTransaction to make it more precise to reflect the proposal intention. Currently it's: https://github.com/neo-project/neo-modules/blob/253a77e2b88f139bebb57779e4f3ca8837f92f68/src/RpcServer/RpcError.cs#L63

Platform:

Additional context The same problem was recently fixed in NeoGo, see https://github.com/nspcc-dev/neo-go/pull/3360. This issue is not critical, but eventually it should be fixed.

roman-khimov commented 6 months ago

That's not the way it was intended to be, sorry. See https://github.com/nspcc-dev/neo-go/pull/3361. The problem with C# code/modules is that it never returns -504.

I'll improve https://github.com/neo-project/proposals/pull/156 wording about -511 though.

AnnaShaleva commented 6 months ago

Good, then no changes from neo-modules or core side are required, thus I'm closing this issue as not planned.

AnnaShaleva commented 6 months ago

The problem with C# code/modules is that it never returns -504.

Although we may use this issue to enable -504 in neo-modules (and core).