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

Add /consensus_params RPC endpoint #972

Merged
merged 6 commits into from
Sep 16, 2021
Merged

Conversation

thanethomson
Copy link
Contributor

Partial fulfillment of #832, and helps towards informalsystems/hermes#1336

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Copy link
Contributor Author

@andynog a quick way of testing this would be to run the tendermint-rpc binary from this branch against a validator:

# Get the latest consensus parameters
cargo run --all-features --bin tendermint-rpc -- -u https://validator-address.com latest-consensus-params

# Get the consensus parameters at height 1234
cargo run --all-features --bin tendermint-rpc -- -u https://validator-address.com consensus-params 1234

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2021

Codecov Report

Merging #972 (1d5628b) into master (80559bd) will increase coverage by 0.0%.
The diff coverage is 79.3%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #972   +/-   ##
======================================
  Coverage    72.1%   72.2%           
======================================
  Files         203     204    +1     
  Lines       16577   16591   +14     
======================================
+ Hits        11961   11985   +24     
+ Misses       4616    4606   -10     
Impacted Files Coverage Δ
rpc/src/client.rs 16.9% <ø> (ø)
rpc/src/client/bin/main.rs 0.6% <0.0%> (-0.1%) ⬇️
rpc/src/method.rs 29.5% <50.0%> (+<0.1%) ⬆️
rpc/src/endpoint/consensus_params.rs 100.0% <100.0%> (ø)
rpc/tests/kvstore_fixtures.rs 95.4% <100.0%> (+<0.1%) ⬆️
rpc/src/paging.rs 0.0% <0.0%> (ø)
light-client/src/types.rs 29.5% <0.0%> (ø)
rpc/src/endpoint/unsubscribe.rs 0.0% <0.0%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80559bd...1d5628b. Read the comment docs.

@adizere
Copy link
Member

adizere commented Sep 14, 2021

Also for testing, to complement Thane's instructions, we can patch in ibc-rs the main Cargo.toml to use this branch, and call into the latest_consensus_params from Andy's branch.

tendermint = { git = "https://github.com/informalsystems/tendermint-rs", branch = "thane/832-consensus-params" }
tendermint-rpc = { git = "https://github.com/informalsystems/tendermint-rs", branch = "thane/832-consensus-params" }
tendermint-proto = { git = "https://github.com/informalsystems/tendermint-rs", branch = "thane/832-consensus-params" }
tendermint-light-client = { git = "https://github.com/informalsystems/tendermint-rs", branch = "thane/832-consensus-params" }
tendermint-testgen = { git = "https://github.com/informalsystems/tendermint-rs", branch = "thane/832-consensus-params" }

@andynog is there a branch WIP for informalsystems/hermes#1336 already?

Signed-off-by: Thane Thomson <connect@thanethomson.com>
@andynog
Copy link

andynog commented Sep 15, 2021

@andynog a quick way of testing this would be to run the tendermint-rpc binary from this branch against a validator:

# Get the latest consensus parameters
cargo run --all-features --bin tendermint-rpc -- -u https://validator-address.com latest-consensus-params

Tested this line:

{
  "block_height": "7673671",
  "consensus_params": {
    "block": {
      "max_bytes": "200000",
      "max_gas": "40000000",
      "time_iota_ms": "1000"
    },
    "evidence": {
      "max_age_num_blocks": "1000000",
      "max_age_duration": "172800000000000",
      "max_bytes": "50000"
    },
    "validator": {
      "pub_key_types": [
        "ed25519"
      ]
    }
  }
}

And this is the response from the server using the RPC endpoint

{
  "jsonrpc": "2.0",
  "id": -1,
  "result": {
    "block_height": "7673671",
    "consensus_params": {
      "block": {
        "max_bytes": "200000",
        "max_gas": "40000000",
        "time_iota_ms": "1000"
      },
      "evidence": {
        "max_age_num_blocks": "1000000",
        "max_age_duration": "172800000000000",
        "max_bytes": "50000"
      },
      "validator": {
        "pub_key_types": [
          "ed25519"
        ]
      },
      "version": {}
    }
  }
}

Most seems OK, the only difference I see is that the RPC outputs a version field. I don't need to use it as far as I know for now but just wanted to point that out.

Get the consensus parameters at height 1234

cargo run --all-features --bin tendermint-rpc -- -u https://validator-address.com consensus-params 1234

This line also seems to work fine:

{
  "block_height": "7673671",
  "consensus_params": {
    "block": {
      "max_bytes": "200000",
      "max_gas": "40000000",
      "time_iota_ms": "1000"
    },
    "evidence": {
      "max_age_num_blocks": "1000000",
      "max_age_duration": "172800000000000",
      "max_bytes": "50000"
    },
    "validator": {
      "pub_key_types": [
        "ed25519"
      ]
    }
  }
}

Copy link

@andynog andynog left a comment

Choose a reason for hiding this comment

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

I think overall looks good to me. Had a minor comment about a version field

rpc/src/endpoint/consensus_params.rs Show resolved Hide resolved
@romac
Copy link
Member

romac commented Sep 16, 2021

Sorry, about be7c665 and its revert, I failed to notice the latest_consensus_params endpoint.

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

All good from my side, we're now using the new endpoint succesfully in informalsystems/hermes#1361 :)

@thanethomson
Copy link
Contributor Author

Awesome. Yeah if we eventually need the version field we can add it in a follow-up PR.

@thanethomson thanethomson merged commit 9fafa34 into master Sep 16, 2021
@thanethomson thanethomson deleted the thane/832-consensus-params branch September 16, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants