Book: Add blockhash to terminology#6711
Conversation
There was a problem hiding this comment.
You're right that blockhash isn't in terminology, but I think what happened is all the block id fields got renamed to blockhash without updating the block id entry in the book.
Let's start by renaming the block id listing to blockhash -- and entry id to entry hash -- and then make additions/modifications to those definitions...?
|
|
||
| ## blockhash | ||
|
|
||
| The hash over a bank's state upon freeze. Used to historically identify the block originated from the bank. |
There was a problem hiding this comment.
We don't define "freeze". Perhaps replace with "upon completion of a block"
There was a problem hiding this comment.
Just realized this definition isn't even correct. We use only the hash of the last entry, which I'm not sure is the RightThing(TM) to do. Seems like we should be covering more of the bank state than that. Is this a performance thing or can it be changed?
There was a problem hiding this comment.
I'm confused by your comment, since each entry hash extends from the entry hash before it.
Perhaps the current entry id definition, although a little ungrammatical, is helpful?
There was a problem hiding this comment.
Sorry, I wasn't clear. By covering I meant the blockhash should probably be generated using more of the bank state. This would, a) make its ID unambiguous, and b) allow verification of the included account states. I'm not sure of the intention of using only the EntryId
There was a problem hiding this comment.
How about: "A preimage resistant hash of the ledger state at a given block height." Some other part of the book can explain how it's calculated and/or how it's used. It might say, "A blockhash is calculated from a SHA256 hash of the previous blockhash and all changes to the ledger state since the previous block."
There was a problem hiding this comment.
Yes, that's a good definition.
It looks like I'm letting responsibilities meld a bit here... What we store in the blockhash_queue at https://github.com/solana-labs/solana/blob/master/runtime/src/bank.rs#L800-L804 is not the "blockhash" as is calculated at https://github.com/solana-labs/solana/blob/master/runtime/src/bank.rs#L592-L605. The latter is what I would consider to be a "blockhash" so it's confusing that it's not what's going into the thing called blockhash_queue
There was a problem hiding this comment.
Mistake on my end. I mixed Blockhash and BankHash. The blockhash is not a hash of the ledger state. It's only a hash of the transactions (specifically, the fee payer's signature). Vote transactions tie together a blockhash and a bank hash.
There was a problem hiding this comment.
So... "A preimage resistant hash of the ledger at a given block height."
There was a problem hiding this comment.
Ok, yeah let's go with that. I'm still unclear as to why we don't use the BankHash as the BlockHash, but that's out side the scope of this PR
|
|
||
| ## blockhash | ||
|
|
||
| A preimage resistant [hash](terminology.md#hash) of the [ledger](terminology.md#ledger) state at a given [block height](terminology.md#block-height). |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
Hoping you're still planning to update/remove "block id" and update "entry id" 🤞
|
@CriesofCarrots ☝️ for your consideration |
| ## block height | ||
|
|
||
| The [entry id](terminology.md#entry-id) of the last entry in a [block](terminology.md#block). | ||
| The number of [blocks](terminology.md#block) beneath the current block. The first block after the [genesis block](terminology.md#genesis-block) has height zero. |
There was a problem hiding this comment.
Yes, it's currently true, but very confusing. We should probably not be zero-indexing block height.. #6779
There was a problem hiding this comment.
🤔 Must be some greedy diff aggregation going on here. That's not a line that I touched
t-nelson
force-pushed
the
blockhash_to_terminology
branch
from
540111e to
4183ff3
Compare
| * `h`, the tick height, as unsigned 64-bit integer | ||
| * `l`, the slot leader id, as base-58 encoded string | ||
| * `id`, the block id, as base-58 encoded string | ||
| * `id`, the [blockhash](terminology.md#blockhash), as base-58 encoded string |
There was a problem hiding this comment.
id also needs updating. It's actually hash now. Thanks for helping clean up :)
solana-grimes
removed
the
automerge
|
💔 Unable to automerge due to CI failure |
t-nelson
force-pushed
the
blockhash_to_terminology
branch
from
c1065eb to
4ae8f4c
Compare
6 participants
Problem
blockhashis missing from terminologySummary of Changes
Add it.
As requested in #6663