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.

Sign up for GitHub

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

Jump to bottom

go/consensus/api: Change Block's Hash field type to common Hash type #4281

Merged
merged 4 commits into from
Sep 27, 2021
Merged

go/consensus/api: Change Block's Hash field type to common Hash type #4281

merged 4 commits into from
Sep 27, 2021

Conversation

tjanez
Copy link
Member

@tjanez tjanez commented Sep 24, 2021 •
edited
Loading

Also change the type of Status' hash-related field (LatestHash, GenesisHash, LastRetainedHash) to the common Hash type.

This will make all hash-related fields in the "consensus" part of oasis-node control status's output use hex-encoding rather than Base64-encoding.

It follows a similar change for textual representation of runtime IDs and runtime state roots which was done in #4279.

The motivation for this change is to unify:

Example of previous "consensus" part of oasis-node control status:

"consensus": {
    "version": {
      "major": 4
    },
    "backend": "tendermint",
    "features": 3,
    "node_peers": [
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:36656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656"
    ],
    "latest_height": 5185037,
    "latest_hash": "Id6Z/Nogj4vKCBTiRwWGDM1dqyuIZ7ZsqA77o1U3Yrc=",
    "latest_time": "2021-09-24T21:48:23+02:00",
    "latest_epoch": 8636,
    "latest_state_root": {
      "ns": "0000000000000000000000000000000000000000000000000000000000000000",
      "version": 5185036,
      "root_type": 1,
      "hash": "69356fab71735bc0976a7af0aa85efdcc14bc4befc815e53ac8eb40a58d733d3"
    },
    "genesis_height": 3027601,
    "genesis_hash": "ZwRTUSdNw9W+NwPP1OxuG1hEHRndlvXfDOpQLE9tlfQ=",
    "last_retained_height": 3027601,
    "last_retained_hash": "ZwRTUSdNw9W+NwPP1OxuG1hEHRndlvXfDOpQLE9tlfQ=",
    "chain_context": "53852332637bacb61b91b6411ab4095168ba02a50be4c3f82448438826f23898",
    "is_validator": false
  },

Example of new "consensus" part of oasis-node control status:

  "consensus": {
    "version": {
      "major": 4
    },
    "backend": "tendermint",
    "features": 3,
    "node_peers": [
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:36656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656",
      "[email protected]:26656"
    ],
    "latest_height": 5185037,
    "latest_hash": "21de99fcda208f8bca0814e24705860ccd5dab2b8867b66ca80efba3553762b7",
    "latest_time": "2021-09-24T21:48:23+02:00",
    "latest_epoch": 8636,
    "latest_state_root": {
      "ns": "0000000000000000000000000000000000000000000000000000000000000000",
      "version": 5185036,
      "root_type": 1,
      "hash": "69356fab71735bc0976a7af0aa85efdcc14bc4befc815e53ac8eb40a58d733d3"
    },
    "genesis_height": 3027601,
    "genesis_hash": "67045351274dc3d5be3703cfd4ec6e1b58441d19dd96f5df0cea502c4f6d95f4",
    "last_retained_height": 3027601,
    "last_retained_hash": "67045351274dc3d5be3703cfd4ec6e1b58441d19dd96f5df0cea502c4f6d95f4",
    "chain_context": "53852332637bacb61b91b6411ab4095168ba02a50be4c3f82448438826f23898",
    "is_validator": false
  },

@tjanez tjanez added the c:consensus/cometbft Category: CometBFT label Sep 24, 2021
@tjanez tjanez force-pushed the tjanez/consensus-hashes-hex-encoded branch from 481aca6 to 7a56ead Compare September 26, 2021 21:11
@tjanez tjanez force-pushed the tjanez/consensus-hashes-hex-encoded branch from 7a56ead to 26cfca4 Compare September 27, 2021 10:15
@tjanez tjanez marked this pull request as ready for review September 27, 2021 10:17
@tjanez tjanez changed the title go/consensus/api: Change hash-related fields in Status type to Hash go/consensus/api: Change Block's Hash field type to common Hash type Sep 27, 2021
@codecov
Copy link

codecov bot commented Sep 27, 2021 •
edited
Loading

Codecov Report

Merging #4281 (26cfca4) into master (3885f1e) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head 26cfca4 differs from pull request most recent head 613e1c8. Consider uploading reports for the commit 613e1c8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4281      +/-   ##
==========================================
- Coverage   68.92%   68.75%   -0.17%     
==========================================
  Files         411      411              
  Lines       47208    47211       +3     
==========================================
- Hits        32536    32460      -76     
- Misses      10632    10733     +101     
+ Partials     4040     4018      -22
Impacted Files Coverage Δ
go/consensus/api/api.go 22.22% <ø> (ø)
go/common/crypto/hash/hash.go 87.50% <100.00%> (+0.61%) ⬆️
go/consensus/tendermint/api/api.go 84.15% <100.00%> (ø)
go/ias/http/http.go 20.33% <0.00%> (-44.07%) ⬇️
go/keymanager/api/policy_sgx.go 33.33% <0.00%> (-13.34%) ⬇️
go/storage/api/context.go 87.87% <0.00%> (-12.13%) ⬇️
go/consensus/tendermint/full/services.go 77.69% <0.00%> (-7.70%) ⬇️
go/oasis-node/cmd/ias/auth.go 68.96% <0.00%> (-6.90%) ⬇️
go/common/cbor/codec.go 78.37% <0.00%> (-5.41%) ⬇️
...consensus/tendermint/apps/keymanager/keymanager.go 62.63% <0.00%> (-3.30%) ⬇️
... and 37 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 3885f1e...613e1c8. Read the comment docs.

@tjanez
go/common/crypto/hash: Add Hex() method to Hash type
66d7e0c 
It can use used a stable method for obtaining the hex-encoded
representation of a hash in case String() method is changed to something
else in the future.
@tjanez
go/consensus/api: Change Block's Hash field type to common Hash type
9a4fe8a 
Also change the type of Status' hash-related field (LatestHash,
GenesisHash, LastRetainedHash) to the common Hash type
(go/common/crypto/hash.Hash) to allow nicer text/JSON serialization.
@tjanez
docs: Update example output of oasis-node control status CLI command
613e1c8
@tjanez tjanez force-pushed the tjanez/consensus-hashes-hex-encoded branch from 26cfca4 to 613e1c8 Compare September 27, 2021 12:20
@tjanez tjanez enabled auto-merge September 27, 2021 12:43
@tjanez tjanez merged commit c352783 into master Sep 27, 2021
@tjanez tjanez deleted the tjanez/consensus-hashes-hex-encoded branch September 27, 2021 12:56
@tjanez tjanez mentioned this pull request Sep 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kostko kostko kostko approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

@ptrus ptrus Awaiting requested review from ptrus ptrus is a code owner

@Yawning Yawning Awaiting requested review from Yawning

Assignees
No one assigned
Labels
c:consensus/cometbft Category: CometBFT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tjanez @kostko