Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Rename genesis block to genesis config #6816

Merged
merged 1 commit into from
Nov 9, 2019
Merged

Rename genesis block to genesis config #6816

merged 1 commit into from
Nov 9, 2019

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Nov 8, 2019 •
edited
Loading

  • Rename GenesisBlock and GenesisBlockInfo
  • Change json rpc api from getGenesisBlockhash to getGenesisHash
  • Change expected-genesis-blockhash arg to expected-genesis-hash

Part 1 of fix for #6795

@codecov
Copy link

codecov bot commented Nov 8, 2019 •
edited
Loading

Codecov Report

Merging #6816 into master will decrease coverage by 9.7%.
The diff coverage is 87.9%.

@@           Coverage Diff            @@
##           master   #6816     +/-   ##
========================================
- Coverage    79.9%   70.1%   -9.8%     
========================================
  Files         219     219             
  Lines       42145   48014   +5869     
========================================
+ Hits        33695   33697      +2     
- Misses       8450   14317   +5867

@garious
Copy link
Contributor

garious commented Nov 8, 2019

The thing that you've renamed GenesisConfigHash looks to be a blockhash. Technically, a GenesisConfig hash could exist, but I don't think that's what's being returned.

mvines
mvines reviewed Nov 8, 2019
View reviewed changes
book/src/api-reference/jsonrpc-api.md Outdated
@@ -21,7 +21,7 @@ To interact with a Solana node inside a JavaScript application, use the [solana-
* [getClusterNodes](jsonrpc-api.md#getclusternodes)
* [getEpochInfo](jsonrpc-api.md#getepochinfo)
* [getEpochSchedule](jsonrpc-api.md#getepochschedule)
* [getGenesisBlockhash](jsonrpc-api.md#getgenesisblockhash)
* [getGenesisConfigHash](jsonrpc-api.md#getgenesisconfighash)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather we keep this as getGenesisBlockhash, and strike the downstream changes due to this rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mvines if we keep this as getGenesisBlockhash are we considering the genesis config's hash as a blockhash? Or is this being interpreted as the genesis block's hash? If it's the latter, I think we should return the hash of the first block here

Copy link
Contributor

Choose a reason for hiding this comment

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

genesis config's hash is a blockhash as in bank0 that's the only recent_blockhash available to transactions to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to getGenesisHash as discussed in Discord

@jstarry
Copy link
Contributor Author

jstarry commented Nov 8, 2019

The thing that you've renamed GenesisConfigHash looks to be a blockhash. Technically, a GenesisConfig hash could exist, but I don't think that's what's being returned.

@garious it looks like a "blockhash" from a bank's perspective but it is computed as a hash of the genesis config, there are no entries in it. I don't think it fits the definition here: https://github.com/solana-labs/solana/blob/master/book/src/terminology.md#blockhash

@mvines
Copy link
Contributor

mvines commented Nov 8, 2019

The thing that you've renamed GenesisConfigHash looks to be a blockhash. Technically, a GenesisConfig hash could exist, but I don't think that's what's being returned.

@garious it looks like a "blockhash" from a bank's perspective but it is computed as a hash of the genesis config, there are no entries in it. I don't think it fits the definition here: https://github.com/solana-labs/solana/blob/master/book/src/terminology.md#blockhash

Fix the definition?

@garious
Copy link
Contributor

garious commented Nov 8, 2019

I'm lost, sorry. I don't see a problem with the blockhash definition. Remind me why RPC needs to query for the genesis hash?

@garious
Copy link
Contributor

garious commented Nov 8, 2019

Thinking more on it, calling the GenesisConfig hash a blockhash seems very wrong.

@jstarry
Copy link
Contributor Author

jstarry commented Nov 8, 2019

I'm lost, sorry. I don't see a problem with the blockhash definition. Remind me why RPC needs to query for the genesis hash?

We check it on validator boot up to make sure that a validator is starting up with the right genesis configuration

@jstarry jstarry closed this Nov 8, 2019
@jstarry
Copy link
Contributor Author

jstarry commented Nov 8, 2019

won't fix - easier to update terminology

@garious
Copy link
Contributor

garious commented Nov 8, 2019

Fixing the terminology comes with a fixed, known cost. The hardest part is coming to consensus, and that part is done. Not fixing terminology adds a cost to every engineer that encounters the inconsistency as well as every bug that arises around the confusion.

@jstarry jstarry reopened this Nov 9, 2019
@jstarry jstarry requested a review from garious November 9, 2019 01:41
mvines
mvines previously approved these changes Nov 9, 2019
View reviewed changes
@jstarry jstarry added the CI Pull Request is ready to enter CI label Nov 9, 2019
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 9, 2019
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
garious
garious previously approved these changes Nov 9, 2019
View reviewed changes
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
@mergify mergify bot dismissed stale reviews from mvines and garious November 9, 2019 04:00

Pull request has been modified.

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Nov 9, 2019
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@jstarry jstarry merged commit 9807f47 into solana-labs:master Nov 9, 2019
@jstarry jstarry deleted the genesis-config branch November 9, 2019 04:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@garious garious garious left review comments

@mvines mvines mvines left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jstarry @garious @mvines @solana-grimes