-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: store proof, round on block #45
Conversation
state/state.go
Outdated
b := make([]byte, 16) | ||
binary.PutVarint(b, state.LastBlockHeight) | ||
binary.PutVarint(b[8:], int64(round)) | ||
hash := tmhash.New() | ||
hash.Write(seed) | ||
return hash.Sum(b), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Meaning of this codes hash + height + round
?
Thus, can other language like java make hash by this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. hash.Write() and hash.Sum() is standard function of sha256 so other language can implement it.
tmhash
is widely used in tendermint. If tmhash is not possible in other language, all other hash function has same problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thmash
of Tendermint appears to be SHA-256, In a 64-bit environment, SHA-512 is significantly faster. How about it? (I think we should make a better decision than following Tendermint as long as it's substantially advantageous in the scope of standard technology.)
return types.NewErrInvalidProof(err.Error()) | ||
} | ||
_, val := state.Validators.GetByAddress(block.ProposerAddress) | ||
verified, err := vrf.Verify(val.PubKey.(ed25519.PubKeyEd25519), block.Proof.Bytes(), message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If err
is not nil
, it does not handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -345,6 +346,10 @@ type Header struct { | |||
// consensus info | |||
EvidenceHash tmbytes.HexBytes `json:"evidence_hash"` // evidence included in the block | |||
ProposerAddress Address `json:"proposer_address"` // original proposer of the block | |||
|
|||
// vrf info | |||
Round int `json:"round"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to define the type as uint32
since the int
in golang varies in size depending on the compilation environment, and we don't need a sign. Related this, it's also better to be the type of the parameter using round
in this PR from int
to uint32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RoundState.Round is already defined as int
. I think it's better to define the type same to RoundState.Round.
state/state.go
Outdated
} | ||
b := make([]byte, 16) | ||
binary.PutVarint(b, state.LastBlockHeight) | ||
binary.PutVarint(b[8:], int64(round)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PutVarint()
is a variable-length encoding feature, and the write range may exceed 8byte depending on the output value. Since variable-length encoding isn't particularly useful here and only unnecessarily reduces portability, I think it's better to be explicitly endian and size.
binary.LittleEndian.PutUint64(b, uint64(state.LastBlockHeight))
binary.LittleEndian.PutUint64(b[8:], uint64(round))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I apply it.
state/state.go
Outdated
b := make([]byte, 16) | ||
binary.PutVarint(b, state.LastBlockHeight) | ||
binary.PutVarint(b[8:], int64(round)) | ||
hash := tmhash.New() | ||
hash.Write(seed) | ||
return hash.Sum(b), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thmash
of Tendermint appears to be SHA-256, In a 64-bit environment, SHA-512 is significantly faster. How about it? (I think we should make a better decision than following Tendermint as long as it's substantially advantageous in the scope of standard technology.)
It's good suggestion. If we decide to use sha512, then we should just modify following code.
|
Besides this, I think there seems to be more to check. |
@egonspace, please change the merge target to |
Ah, thank you. Done. |
Sure. I think so. |
My previous comment was the idea that intended to make the SHA-512 only new features that we add with VRF. If we replace the Tendermint's |
Well, I think the reason they defined tmhash is to unify all hash functions in tendermint into one. There is no reason to use 512 in some cases and 256 in others. I think it's better to change |
closes #7
closes #8
Added two field of
round
andproof
in block.createBlock
adding vrf proofverifyBlock
verifying vrf proofTesting