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

fix(scan): Improve gRPC method errors and add timeout to scan service to avoid hanging #8318

Merged
merged 14 commits into from
Mar 14, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Feb 22, 2024

Motivation

We want to return an error from zebra-grpc methods when a service call takes too long, and we want to return a more informative error when a SubscribeResults responder is dropped.

Closes #8316.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

  • Adds a timeout to the scan service before passing it to zebra_grpc::init()
  • Returns error message from service calls in gRPC method errors
  • Adds a useful error message when subscribe responders are dropped

Related Cleanups:

  • Returns oneshot receiver from ScanTask.subscribe() to avoid holding a mutable ref to self across awaits

Testing

  • Tests that the scan service returns an error for RegisterKeys requests where no new keys are added to the scan task
  • Tests that timeout layer returns a timeout error when there's no response before SCAN_SERVICE_TIMEOUT

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@arya2 arya2 added C-bug Category: This is a bug I-usability Zebra is hard to understand or use A-rpc Area: Remote Procedure Call interfaces A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 22, 2024
@arya2 arya2 self-assigned this Feb 22, 2024
@arya2 arya2 requested a review from a team as a code owner February 22, 2024 19:27
@arya2 arya2 requested review from upbqdn and removed request for a team February 22, 2024 19:27
@arya2 arya2 marked this pull request as draft February 22, 2024 19:54
@arya2 arya2 marked this pull request as ready for review February 22, 2024 20:31
@upbqdn upbqdn added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Feb 23, 2024
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Feb 23, 2024
@oxarbitrage
Copy link
Contributor

I think we need to put a better error for when the scanner had not reached the tip to close #8316

Right now (using this PR), the error does not describe that so users will now know whats going on:

$ ./grpcurl -plaintext -d '{ "keys": { "key": ["sapling_extended_full_viewing_key"] } }' '127.0.0.1:6666' scanner.Scanner/Scan
ERROR:
  Code: Unknown
  Message: scan service returned error: request timed out
$ 

@oxarbitrage
Copy link
Contributor

I am not sure if this is working. I am now trying with a state close to the tip to avoid the previous problem and an empty scanner state folder, no keys in the config. I am getting this error when i try to scan:

$ ./grpcurl -plaintext -d '{ "keys": { "key": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] } }' '127.0.0.1:6666' scanner.Scanner/Scan
ERROR:
  Code: Unknown
  Message: scan service returned error: scan task dropped responder, check that keys are already registered
$ 

I tried again and i get a different error:

$ ./grpcurl -plaintext -d '{ "keys": { "key": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] } }' '127.0.0.1:6666' scanner.Scanner/Scan
ERROR:
  Code: Unknown
  Message: scan service returned error: no keys were registered, check that keys are not already registered and are valid Sapling extended full viewing keys
$ 

@arya2 arya2 marked this pull request as draft February 28, 2024 20:50
@arya2 arya2 marked this pull request as ready for review March 14, 2024 01:30
@arya2 arya2 requested a review from a team as a code owner March 14, 2024 01:30
@oxarbitrage
Copy link
Contributor

Thanks for the new commits, ill take a new look at this soon.

@oxarbitrage
Copy link
Contributor

Nice! Here are some manual tests i did to check this out. I think there are a few minor issues that could be fixed by just adding some TODOs in the code before merging in.

$ grpcurl -plaintext -d '{ "keys": { "key": ["sapling_extended_full_viewing_key"] } }' '127.0.0.1:8231' scanner.Scanner/Scan
ERROR:
  Code: InvalidArgument
  Message: no keys were registered, check that keys are not already registered and are valid Sapling extended full viewing keys
$

Nice!

$ grpcurl -plaintext -d '{ "keys": { "key": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] } }' '127.0.0.1:8231' scanner.Scanner/Scan
{
  "results": {
    "zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz": {
      "byHeight": {
        "780532": {
          "transactions": [
            {
              "hash": "1c519cb72bc20d024f9c0954178403d16fac7ce8db594ac60db76213e6c8826b"
            }
          ]
        },
        
...
}
{
  "results": {
    "zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz": {
      "byHeight": {
        "1063107": {
          "transactions": [
            {
              "hash": "7c1b89c2006a72dcc2880892bd1d43f2438b4d5df4bc465886ec1855ccfc2f98"
            }
          ]
        }
      }
    }
  }
}
{

Maybe an error here instead of null response? We can do this later if we want to.

$ grpcurl -plaintext -d '{ "keys": ["whatever"] }' '127.0.0.1:8231' scanner.Scanner/GetResults
{
  "results": {
    "whatever": null
  }
}
$ 

Good:

$ grpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/GetResults
{
  "results": {
    "zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz": {
      "byHeight": {
        "780532": {
          "transactions": [
            {
              "hash": "1c519cb72bc20d024f9c0954178403d16fac7ce8db594ac60db76213e6c8826b"
            }
          ]
        },
        "780616": {
          "transactions": [
            {
              "hash": "9ef232c16b791e5c13c067115776dcfa1544b1830401af1c282886ca6395679e"
            }
          ]
        },
        "780773": {
          "transactions": [
            {
              "hash": "b93baab34ecc5c6e21ba4b16070581d5c72b1b38d7e121be51664f3cf0425713"
            }
          ]
        },
...

Nice:

$ grpcurl -plaintext -d '{ "keys": { "key": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] } }' '127.0.0.1:8231' scanner.Scanner/RegisterKeys
ERROR:
  Code: Unknown
  Message: no keys were registered, check that keys are not already registered and are valid Sapling extended full viewing keys
$ 

The first call should work but maybe we should error in the following ones as there is no such a key to delete results from:

$ grpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/ClearResults
{}
$ grpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/ClearResults
{}
$ grpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/ClearResults
{}
$ 

The sample applies to DeleteKeys, i think it can be important to distinguish successful requests somehow:

$ rpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/DeleteKeys
{}
$ grpcurl -plaintext -d '{ "keys": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] }' '127.0.0.1:8231' scanner.Scanner/DeleteKeys
$ grpcurl -plaintext -d '{ "keys": ["whatever"] }' '127.0.0.1:8231' scanner.Scanner/DeleteKeys
{}
$ 

This is good:

$ grpcurl -plaintext -d '{ "keys": { "key": ["zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"] } }' '127.0.0.1:8231' scanner.Scanner/RegisterKeys
{
  "keys": [
    "zxviews1q0duytgcqqqqpqre26wkl45gvwwwd706xw608hucmvfalr759ejwf7qshjf5r9aa7323zulvz6plhttp5mltqcgs9t039cx2d09mgq05ts63n8u35hyv6h9nc9ctqqtue2u7cer2mqegunuulq2luhq3ywjcz35yyljewa4mgkgjzyfwh6fr6jd0dzd44ghk0nxdv2hnv4j5nxfwv24rwdmgllhe0p8568sgqt9ckt02v2kxf5ahtql6s0ltjpkckw8gtymxtxuu9gcr0swvz"
  ]
}
$grpcurl -plaintext -d '{ "keys": { "key": ["whatever"] } }' '127.0.0.1:8231' scanner.Scanner/RegisterKeys
ERROR:
  Code: Unknown
  Message: no keys were registered, check that keys are not already registered and are valid Sapling extended full viewing keys
$ 

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

mergify bot added a commit that referenced this pull request Mar 14, 2024
@mergify mergify bot merged commit 5e86ebb into main Mar 14, 2024
135 checks passed
@mergify mergify bot deleted the improve-grpc-errors branch March 14, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug I-usability Zebra is hard to understand or use P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error messages from grpc methods.
3 participants