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

penumbra.util.tendermint_proxy.v1.TendermintProxyService.GetBlocksByHeight seems broken #4543

Closed
turbocrime opened this issue Jun 4, 2024 · 7 comments
Labels
needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@turbocrime
Copy link
Contributor

Describe the bug

GetBlocksByHeight fails to respond for any attempted height

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://grpcui.testnet.penumbra.zone/
  2. Use Service name penumbra.util.tendermint_proxy.v1.TendermintProxyService
  3. Use Method name GetBlockByHeight
  4. Input a valid height
  5. Invoke

Expected behavior

Block data is returned

Screenshots

Screenshot_2024-06-04_at_10 27 26
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra Jun 4, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Jun 4, 2024
@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

I'm trying to write a test script for it locally but having some trouble.

I can call it with grpcurl at least, but the golang protobuf library isn't playing nicely with the way cometbft encoded timestamps:

grpcurl -d '{ "height": 326710 }' grpc.testnet.penumbra.zone:443 penumbra.util.tendermint_proxy.v1.TendermintProxyService.GetBlockByHeight
Failed to format response message 1: ns out of range [0, 1000000000)

and I can't find a command line flag to tell grpcurl to print the raw response

I then tried writing a test script in Rust, and was able to call against localhost, but not the testnet endpoint:

let mut tm_client = TendermintProxyServiceClient::connect("http://localhost:8080").await?;
let r = tonic::Request::new(GetBlockByHeightRequest { height: 326_710 });
let res = tm_client.get_block_by_height(r).await?.into_inner();

^^ that works

let mut tm_client = TendermintProxyServiceClient::connect("https://grpc.testnet.penumbra.zone:443").await?;
let r = tonic::Request::new(GetBlockByHeightRequest { height: 326_710 });
let res = tm_client.get_block_by_height(r).await?.into_inner();

^^ that doesn't:

Error: Status { code: Internal, message: "protocol error: received message with invalid compression flag: 52 (valid flags are 0 and 1) while receiving response with status: 404 Not Found", metadata: MetadataMap { headers: {"content-type": "text/plain; charset=utf-8", "x-content-type-options": "nosniff", "content-length": "19", "date": "Tue, 04 Jun 2024 17:54:11 GMT"} }, source: None }

I have no idea why it would get a 404 on grpc.testnet.penumbra.zone but not localhost, especially considering that I was able to grpcurl the endpoint.

@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

The underlying issue with using grpcui to call is the same golang parsing issue. If you use the Chrome network inspector and check the response you'll find:

  "responses": [
    {
      "message": "ns out of range [0, 1000000000)",
      "isError": true
    }
  ],

@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

Figured it out, that was Tonic feature flags.

I'm able to call the API from rust code now and get a response so I don't think the API is actually broken however there is an issue in that historical data is unavailable post-upgrade. I know @conorsch has been looking at things in that domain, maybe there's a fix?

@turbocrime
Copy link
Contributor Author

Screenshot_2024-06-04_at_11 44 15

nanos from the endpoint appear to be negative, which is against spec

https://github.com/protocolbuffers/protobuf/blob/bf7ac9f2f11e15c1a8f4b41428a75ad1c039a8fe/src/google/protobuf/timestamp.proto#L133-L144

message Timestamp {
  // Represents seconds of UTC time since Unix epoch
  // 1970-01-01T00:00:00Z. Must be from 0001-01-01T00:00:00Z to
  // 9999-12-31T23:59:59Z inclusive.
  int64 seconds = 1;


  // Non-negative fractions of a second at nanosecond resolution. Negative
  // second values with fractions must still have non-negative nanos values
  // that count forward in time. Must be from 0 to 999,999,999
  // inclusive.
  int32 nanos = 2;
}

@turbocrime
Copy link
Contributor Author

notably, timestamp with negative nanos are unable to re-transmit to the client through our web communication channel

Screenshot_2024-06-04_at_11 21 26

@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

Using an int32 to hold a value that's supposed to be positive is chaotic

@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

Closing as this isn't actually "broken" (well besides the nanos thing) and we need to build our own RPC endpoint

@zbuc zbuc closed this as completed Jun 4, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-refinement unclear, incomplete, or stub issue that needs work
Projects
Archived in project
Development

No branches or pull requests

2 participants