Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Non-specific error for JSON-RPC specification #297

Closed
im-kulikov opened this issue Feb 11, 2019 · 9 comments
Closed

Non-specific error for JSON-RPC specification #297

im-kulikov opened this issue Feb 11, 2019 · 9 comments
Labels
discussion Issue state: Proposed, to be discussed by the community.

Comments

@im-kulikov
Copy link

This error is reproduced on versions 2.8.0-2.9.1

How to reproduce:

  • just use image: cityofzion/neo-privatenet:2.9.1
  • make request (for example):
## Submit block
curl -X "POST" "http://127.0.0.1:30333/" \
     -H 'Content-Type: application/json; charset=utf-8' \
     -H 'Accept-Encoding: gzip, zlib, deflate, zstd, br' \
     -d $'{
  "jsonrpc": "2.0",
  "method": "sendtoaddress",
  "id": 1,
  "params": []
}'
  • response:
{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": -2146233086,
    "message": "Index was out of range. Must be non-negative and less than the size of the collection.\nParameter name: index"
  }
}

"Index was out of range. Must be non-negative and less than the size of the collection.\nParameter name: index"

this is a specific for C# exception

The JSON-RPC (https://www.jsonrpc.org/specification#error_object) says that "error codes from and including -32768 to -32000 are reserved for pre-defined errors". In this case, the application itself is providing an specific error. I think that if no param is given, error -32602 should be raised, then.

Message must be something like "Method require params"

@im-kulikov
Copy link
Author

UPD: The problem is reproduced with any method that requires input parameters

@im-kulikov
Copy link
Author

I find almost same exeption...

## Submit block
curl -X "POST" "http://127.0.0.1:30333/" \
     -H 'Content-Type: application/json; charset=utf-8' \
     -H 'Accept-Encoding: gzip, zlib, deflate, zstd, br' \
     -d $'{
  "jsonrpc": "2.0",
  "method": "getaccountstate",
  "id": 1,
  "params": [
    123
  ]
}'

Response:

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": -2146233033,
    "message": "One of the identified items was in an invalid format."
  }
}

@shargon
Copy link
Member

shargon commented Feb 11, 2019

The error should be more descriptive

@im-kulikov
Copy link
Author

@shargon yep.. but not C# exception..

@im-kulikov
Copy link
Author

"Index was out of range. Must be non-negative and less than the size of the collection.\nParameter name: index"

and

"One of the identified items was in an invalid format."

is C# System Exceptions..

I just propose to catch them and give the correct error message and code, according to the specification JSON-RPC

@im-kulikov
Copy link
Author

@shargon or you are talking about something else?

@shargon
Copy link
Member

shargon commented Feb 12, 2019

I agree with you

@im-kulikov
Copy link
Author

The first problem is that the presence of an index in the array is not checked. This generates an exception that no one else handles.

The second problem is caused by the fact that there is no conversion error handling.

im-kulikov added a commit to im-kulikov/neo that referenced this issue Feb 17, 2019
@im-kulikov
Copy link
Author

I make PR to solve that issue neo-project/neo#587

@erikzhang erikzhang added discussion Issue state: Proposed, to be discussed by the community. and removed help wanted labels Feb 18, 2019
erikzhang pushed a commit to neo-project/neo that referenced this issue Feb 18, 2019
ghost pushed a commit to ZoroChain/Zoro that referenced this issue Feb 22, 2019
ghost pushed a commit to ZoroChain/Zoro-RpcHost that referenced this issue Feb 22, 2019
txhsl added a commit to txhsl/neo that referenced this issue Feb 25, 2019
* Handles escape characters in JSON

*  Pass ApplicationExecution to IPersistencePlugin (neo-project#531)

* Update dependencies: (neo-project#532)

- Akka 1.3.11
- Microsoft.AspNetCore.ResponseCompression 2.2.0
- Microsoft.AspNetCore.Server.Kestrel 2.2.0
- Microsoft.AspNetCore.Server.Kestrel.Https 2.2.0
- Microsoft.AspNetCore.WebSockets 2.2.0
- Microsoft.EntityFrameworkCore.Sqlite 2.2.0
- Microsoft.Extensions.Configuration.Json 2.2.0

* change version to v2.9.4

* Updating Unknown to Policy Fail (neo-project#533)

* Fix a dead lock in `WalletIndexer`

* Downgrade Sqlite to 2.1.4 (neo-project#535)

* RPC call gettransactionheight (neo-project#541)

* getrawtransactionheight

Nowadays two calls are need to get a transaction height, `getrawtransaction` with `verbose` and then use the `blockhash`.
Other option is to use `confirmations`, but it can be misleading.

* Minnor fix

* Shargon's tip

* modified

* Allow to use the wallet inside a RPC plugin (neo-project#536)

* Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500)

Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500)

* Keep both verified and unverified (previously verified) transactions in sorted trees so ejecting transactions above the pool size is a low latency operation.
* Re-verify unverified transactions when Blockchain actor is idle.
* Don't re-verify transactions needlessly when not at the tip of the chain.
* Support passing a flag to `getrawmempool` to retrieve both verified and unverified TX hashes.
* Support MaxTransactionsPerBlock and MaxFreeTransactionsPerBlock from Policy plugins.
* Rebroadcast re-verified transactions if it has been a while since last broadcast (high priority transactions are rebroadcast more frequently than low priority transactions.

* Policy filter GetRelayResult message (neo-project#543)

* Policy filter GetRelayResult message

* adding fixed numbering for return codes

* Removed enum fixed values

* Add some initial MemoryPool unit tests. Fix bug when Persisting the GenesisBlock (neo-project#549)

* More MemoryPool Unit Tests. Improve Re-broadcast back-off to an increasing linear formula. (neo-project#554)

* Ensuring Object Reference check of SortedSets for speed-up (neo-project#557)

* Minor comments update on Mempool class (neo-project#556)

* Update MemoryPool Unit Test to add random fees to Mock Transactions (neo-project#558)

* Add Unit Test for MemoryPool sort order. Fixed sort order to return descending. (neo-project#559)

* Add unit test to verify memory pool sort order and reverification order. Fixed sort order bug.

* VerifyCanTransactionFitInPool works as intended. Also inadvertantly verified GetLowestFeeTransaction() works.

* Benchmark structure for UInt classes (neo-project#553)

* basic benchmark structure for UInt classes

* commented code2 from lights for now

* updated tests. all seem correct now

* Switch to using a benchmark method taking a method delegate to benchmark.

* Make pass.

* 1 million iterations.

* Switch to ulong for the 4th option, and it is still the same speed.

* fix test data for 50% equal data

* make test pass

* neo.UnitTests/UT_UIntBenchmarks.cs

* neo.UnitTests/UT_UIntBenchmarks.cs

* Base 20 - UInt160 tests

* neo.UnitTests/UT_UIntBenchmarks.cs

* inlined 160

* complete tests with UInt256 and UInt160

* neo.UnitTests/UT_UIntBenchmarks.cs

* Lights division calculation

* Treat lower hashes as higher priority. Fix MemoryPool UT for Hash order. (neo-project#563)

* Treat lower hashes as higher priority. 
* Fix MemoryPool UT for Hash order.
* Renaming Trasanction in PoolItem for clarity.

* Make PoolItem independent and add PoolItem tests (neo-project#562)

* make poolitem independent

* Merging

* Multiply by -1

* Fix other

* Fix Tx

* Removing -1 extra multiplication

* Fix

* make PoolItem internal and added test class

* Update PoolItem.cs

* added comments for PoolItem variables

* getting time from TimeProvider to allow testing

* basic test

* reset time provider

* Add Hash comparison

* Adding time provider again and equals

* Fix arithmetic

* Comment on PoolItem

* Update PoolItem.cs

* protecting tests against TimeProvider changes on fails

* reusing setup part

* fixed serialization properties

* Improve generation of creating mock DateTime values. Implement hash comparison tests.

* Adjust comment.

* Treat Claim transactions as the highest low priority transactions. (neo-project#565)

* Allow persistence plugins to commit as a final step. (neo-project#568)

* Allow persistence plugins to commit as a final step.

* Plugins commit before core commits, once all plugins have handled initial work OnPersist.

* Allow PersistencePlugin to determine whether to crash if commit fails.

* Add ShouldThrowExceptionFromCommit method to IPersistencePlugin.

* Throw all commit exceptions that should be thrown in an AggregateException.

* Add a Plugin type for observing transactions added or removed from the MemoryPool. (neo-project#580)

* Correctly handle conversions between JSON objects (neo-project#586)

* Fix neo-project/neo-node#297 (neo-project#587)

* Replace new JArray with .ToArray (AccountState) (neo-project#581)

* Ensure `LocalNode` to be stoped before shutting down the `NeoSystem`
txhsl added a commit to txhsl/neo that referenced this issue Feb 25, 2019
* Handles escape characters in JSON

*  Pass ApplicationExecution to IPersistencePlugin (neo-project#531)

* Update dependencies: (neo-project#532)

- Akka 1.3.11
- Microsoft.AspNetCore.ResponseCompression 2.2.0
- Microsoft.AspNetCore.Server.Kestrel 2.2.0
- Microsoft.AspNetCore.Server.Kestrel.Https 2.2.0
- Microsoft.AspNetCore.WebSockets 2.2.0
- Microsoft.EntityFrameworkCore.Sqlite 2.2.0
- Microsoft.Extensions.Configuration.Json 2.2.0

* change version to v2.9.4

* Updating Unknown to Policy Fail (neo-project#533)

* Fix a dead lock in `WalletIndexer`

* Downgrade Sqlite to 2.1.4 (neo-project#535)

* RPC call gettransactionheight (neo-project#541)

* getrawtransactionheight

Nowadays two calls are need to get a transaction height, `getrawtransaction` with `verbose` and then use the `blockhash`.
Other option is to use `confirmations`, but it can be misleading.

* Minnor fix

* Shargon's tip

* modified

* Allow to use the wallet inside a RPC plugin (neo-project#536)

* Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500)

Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500)

* Keep both verified and unverified (previously verified) transactions in sorted trees so ejecting transactions above the pool size is a low latency operation.
* Re-verify unverified transactions when Blockchain actor is idle.
* Don't re-verify transactions needlessly when not at the tip of the chain.
* Support passing a flag to `getrawmempool` to retrieve both verified and unverified TX hashes.
* Support MaxTransactionsPerBlock and MaxFreeTransactionsPerBlock from Policy plugins.
* Rebroadcast re-verified transactions if it has been a while since last broadcast (high priority transactions are rebroadcast more frequently than low priority transactions.

* Policy filter GetRelayResult message (neo-project#543)

* Policy filter GetRelayResult message

* adding fixed numbering for return codes

* Removed enum fixed values

* Add some initial MemoryPool unit tests. Fix bug when Persisting the GenesisBlock (neo-project#549)

* More MemoryPool Unit Tests. Improve Re-broadcast back-off to an increasing linear formula. (neo-project#554)

* Ensuring Object Reference check of SortedSets for speed-up (neo-project#557)

* Minor comments update on Mempool class (neo-project#556)

* Update MemoryPool Unit Test to add random fees to Mock Transactions (neo-project#558)

* Add Unit Test for MemoryPool sort order. Fixed sort order to return descending. (neo-project#559)

* Add unit test to verify memory pool sort order and reverification order. Fixed sort order bug.

* VerifyCanTransactionFitInPool works as intended. Also inadvertantly verified GetLowestFeeTransaction() works.

* Benchmark structure for UInt classes (neo-project#553)

* basic benchmark structure for UInt classes

* commented code2 from lights for now

* updated tests. all seem correct now

* Switch to using a benchmark method taking a method delegate to benchmark.

* Make pass.

* 1 million iterations.

* Switch to ulong for the 4th option, and it is still the same speed.

* fix test data for 50% equal data

* make test pass

* neo.UnitTests/UT_UIntBenchmarks.cs

* neo.UnitTests/UT_UIntBenchmarks.cs

* Base 20 - UInt160 tests

* neo.UnitTests/UT_UIntBenchmarks.cs

* inlined 160

* complete tests with UInt256 and UInt160

* neo.UnitTests/UT_UIntBenchmarks.cs

* Lights division calculation

* Treat lower hashes as higher priority. Fix MemoryPool UT for Hash order. (neo-project#563)

* Treat lower hashes as higher priority. 
* Fix MemoryPool UT for Hash order.
* Renaming Trasanction in PoolItem for clarity.

* Make PoolItem independent and add PoolItem tests (neo-project#562)

* make poolitem independent

* Merging

* Multiply by -1

* Fix other

* Fix Tx

* Removing -1 extra multiplication

* Fix

* make PoolItem internal and added test class

* Update PoolItem.cs

* added comments for PoolItem variables

* getting time from TimeProvider to allow testing

* basic test

* reset time provider

* Add Hash comparison

* Adding time provider again and equals

* Fix arithmetic

* Comment on PoolItem

* Update PoolItem.cs

* protecting tests against TimeProvider changes on fails

* reusing setup part

* fixed serialization properties

* Improve generation of creating mock DateTime values. Implement hash comparison tests.

* Adjust comment.

* Treat Claim transactions as the highest low priority transactions. (neo-project#565)

* Allow persistence plugins to commit as a final step. (neo-project#568)

* Allow persistence plugins to commit as a final step.

* Plugins commit before core commits, once all plugins have handled initial work OnPersist.

* Allow PersistencePlugin to determine whether to crash if commit fails.

* Add ShouldThrowExceptionFromCommit method to IPersistencePlugin.

* Throw all commit exceptions that should be thrown in an AggregateException.

* Add a Plugin type for observing transactions added or removed from the MemoryPool. (neo-project#580)

* Correctly handle conversions between JSON objects (neo-project#586)

* Fix neo-project/neo-node#297 (neo-project#587)

* Replace new JArray with .ToArray (AccountState) (neo-project#581)

* Ensure `LocalNode` to be stoped before shutting down the `NeoSystem`
rodoufu pushed a commit to rodoufu/neo that referenced this issue Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Issue state: Proposed, to be discussed by the community.
Projects
None yet
Development

No branches or pull requests

3 participants