Conversation
092c81e to
1f629e0
Compare
VojtechVitek
left a comment
There was a problem hiding this comment.
The very first "genesis block" (block=0) shouldn't have any transactions. But yeah, it makes sense we rely on it now because of parent hash.
https://polygonscan.com/block/0
LGTM
| "github.com/c2h5oh/datasize" | ||
| ) | ||
|
|
||
| const NoBlockNum = uint64(math.MaxUint64) |
There was a problem hiding this comment.
This choice might not represent a risk in terms of feasibility, because is such a high number that it won't be reached naturally in a very long time, but it's hard to manage: in order to write proper conditions we'll have to remember things like NoBlockNum > 0, and anyLegitimateBlock < NoBlockNum, or 0 <= anyLegitimateBlock < NoBlockNum
We would have to do mental gymnastics to use it properly:
if blockNum != NoBlockNum { // ok, no problem, this is readable
if blockNum > startBlock { // you forgot to include `blockNum != NoBlockNum`
if blockNum != NoBlockNum && blockNum > startBlock { // this would be the right way to check
// simple conditions would become complex
if blockNum >= minBlock && blockNum <= maxBlock { // this actually works, but by accident because we're not checking `NoBlockNum`
if currentBlock - blockNum < 10 { // no NoBlockNum check, and might even lead to underflow
If the intent is to represent block number hasn’t been set, maybe we could use a nil pointer, or a new descriptive type:
type BlockNumber struct{}
func (b BlockNumber) IsValid() bool { … }
func (b BlockNumber) Uint64() uint64 { … }
// or just
func (b BlockNumber) Value() (uint64, bool) { … }My point is that this technically works, but the cost is extra cognitive load because it's counterintuitive, I'd suggest to rethink this proposal.
|
Decided not to go forward with this. |
It turns out that the first block in the blockchain is block 0. Everywhere in our code we assume it's block number 1.
This assumption needs to be changed because otherwise we can not validate the sequential block hash against parent hash.