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

Use error code -1 for incorrect RPC parameters #6054

Closed
teor2345 opened this issue Jan 31, 2023 · 2 comments · Fixed by #6066
Closed

Use error code -1 for incorrect RPC parameters #6054

teor2345 opened this issue Jan 31, 2023 · 2 comments · Fixed by #6066
Assignees
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jan 31, 2023

Motivation

node-stratum-pool expects a -1 error code when the RPC parameters are incorrect, but jsonrpc_core provides code -32602 by default:
https://github.com/s-nomp/node-stratum-pool/blob/d86ae73f8ff968d9355bb61aac05e0ebef36ccb5/lib/pool.js#L459-L461

equihash-solomining logs:

[PoolWorker][Thread 0][2023/01/31 16:23:19] Could not detect block submission RPC method, [{"result":{"isvalid":true,"address":"tmRGc4CD1UyUdbSJmTUzcB6oDqk4qUaHnnh"},"id":1675146199404},{"result":1,"id":1675146199399},{"result":{"build":"v1.0.0-rc.3+60.g3ab518e.modified","subversion":"/Zebra:1.0.0-rc.3/"},"id":1675146199403},{"result":{"networksolps":4,"networkhashps":4,"chain":"test","testnet":true},"id":1675146199402},{"error":{"code":-32602,"message":"`params` should have at least 1 argument(s)"},"id":1675146199405}]

Complex Code or Requirements

I think we can fix this by changing the error code in an IoHandler, after doing the RPC call:
https://docs.rs/jsonrpc-core/18.0.0/jsonrpc_core/struct.IoHandler.html#method.handle_call

This is very similar to our existing TracingMiddleware, we just need to change the error code in the "incorrect parameters" case:

if let Some(Output::Failure(Failure {
error:
Error {
code: ErrorCode::MethodNotFound,
..
},
..
})) = output
{
tracing::warn!("Received unrecognized RPC request: {call_description}");
}

Testing

We can manually test this with equihash-solomining or s-nomp.

We might be able to do a snapshot test for submitblock with no arguments, but we'd have to run the whole RPC server to do it. (Calling the RPC function directly doesn't work, because Rust requires valid arguments to compile.)

Related Work

This blocks #5934 with both equihash-solomining and s-nomp.

The only other error codes I could find are in the payment code, which Zebra doesn't support because it doesn't have a wallet:
https://github.com/s-nomp/s-nomp/blob/8567a4c649ab6f5ea37ac3c3901f2d9c2597199a/libs/paymentProcessor.js#L750

@teor2345 teor2345 added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Jan 31, 2023
@teor2345 teor2345 self-assigned this Jan 31, 2023
@mpguerra mpguerra added this to Zebra Jan 31, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Jan 31, 2023
@teor2345
Copy link
Contributor Author

@mpguerra I'd like to fix this in this sprint, because it's blocking my progress on testing mining.

@mpguerra
Copy link
Contributor

mpguerra commented Feb 2, 2023

@teor2345 can you please add an estimate for this issue?

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants