qkc/types: add hashing and rlp helpers#25
Conversation
|
|
||
| func DeriveSha(list DerivableList) common.Hash { | ||
| keybuf := new(bytes.Buffer) | ||
| trie := trie.NewEmpty(triedb.NewDatabase(rawdb.NewMemoryDatabase(), nil)) |
There was a problem hiding this comment.
Do we need a trie database here? DeriveSha always starts from an empty trie, inserts all nodes in memory, and only calls Hash(). In the current geth implementation, NewReader short-circuits for EmptyRootHash, Trie.Hash does not write to the database, and geth's own trie.NewListHasher uses NewEmpty(nil).
This can be simplified to trie.NewEmpty(nil) (and likewise for EmptyTrieHash below), allowing the rawdb and triedb imports to be removed. The root should be identical; this just avoids unused allocation and makes the adaptation comment accurate.
There was a problem hiding this comment.
Good point, there's no need to pass database when persistence is not needed. Fixed in c44d366
| } | ||
| buf.reset() | ||
| defer bufPool.Put(buf) | ||
| serialize.SerializeStructWithout(reflect.ValueOf(val), buf.bytes, excludeList) |
There was a problem hiding this comment.
Please handle the serialization error before hashing. SerializeStructWithout can fail after having appended earlier fields (for example, an oversized Extra with a two-byte length prefix, or a failing big-int/custom serializer). Ignoring the error here makes getHash return the Keccak of a partial encoding, so distinct invalid Header/Meta values can receive the same apparent consensus hash.
This is the same failure mode already handled in CalculateMerkleRoot. Could this fail loudly as well, with a regression test?
| serialize.SerializeStructWithout(reflect.ValueOf(val), buf.bytes, excludeList) | |
| if err := serialize.SerializeStructWithout(reflect.ValueOf(val), buf.bytes, excludeList); err != nil { | |
| panic(err) | |
| } |
| Value chan int | ||
| } | ||
|
|
||
| func TestCalculateMerkleRootRejectsInvalidInput(t *testing.T) { |
There was a problem hiding this comment.
Could we add positive byte-compatibility golden vectors to this foundational PR? The current tests only exercise rejection/panic paths, so an implementation that returns an incorrect value for every valid input would still pass.
Because these helpers produce consensus commitments, it would be useful to lock down at least:
CalculateMerkleRoot: empty, single, odd, and even-length lists, including a pyquarkchain or historical-chain golden;DeriveShaandEmptyTrieHash: canonical receipt-trie roots;Uint32: exact0x84 || uint32-bebytes for zero and max values plus decode round trips;serHash: normal and excluded-field outputs.
Ideally the expected bytes/hashes should be hard-coded from Python or historical data rather than recomputed with the same Go helpers. This is a compatibility-proof gap, not an observed mismatch in the current positive-path implementation.
There was a problem hiding this comment.
Added more tests in 57836b9, the expected values are from python.
|
Process suggestion for future PRs: before submission, please run a review pass with Codex 5.5 using extra-high reasoning effort. In this review, the unchecked serialization error and the missing positive compatibility goldens were both identified by Codex in its initial review pass, before any issue-specific follow-up prompting. Running the same pass before opening the PR would likely catch this class of problem earlier and reduce review cycles. |
syntrust
left a comment
syntrust
left a comment
There was a problem hiding this comment.
Generally speaking, I prefer not to port goquarkchain verbatim:
- It also copy bugs and typos
- It ports modified geth from old version
- It is pyquarkchain we need to benchmark against, not goquarkchain
- The PR ported this way is not self-contained and hard to review
| return &hashBuf{bytes: new([]byte), hw: crypto.NewKeccakState()} | ||
| } | ||
|
|
||
| func (h *hashBuf) reset() { |
There was a problem hiding this comment.
Looks like we are reinventing crypto.Keccak256Hash here.
| } | ||
|
|
||
| if data[0] != prefixOfRlpUint32 { | ||
| return fmt.Errorf("preString is wrong, is %v should %v", data[0], lenOfRlpUint32) |
| Bytes(i int) []byte | ||
| } | ||
|
|
||
| func DeriveSha(list DerivableList) common.Hash { |
There was a problem hiding this comment.
May we reuse the well-maintained geth version of the same func?
There was a problem hiding this comment.
This version is more similar to the pyquarkchain/goquarkchain version, so I think it's ok to keep it for better alignment.
| lenOfRlpUint32 = 5 | ||
| ) | ||
|
|
||
| type Uint32 uint32 |
There was a problem hiding this comment.
This Uint32 will be used by mnt in core/types/, so there is no need to define it here. Furthermore, we should not have core/types/ reference qkc/types/ to avoid circular dependencies.
There was a problem hiding this comment.
Why is it necessary to put mnt changes inside core/types/ in the first place? IIRC we should modify out-place instead of in-place if possible.
There was a problem hiding this comment.
And if later we actually meet the dependency issue, we can move it to a proper position, hopefully the diff will be easy to review since it's just a movement. It's a bit hard to imagine the future use case at this PR.
4 participants
This is the subdivided pr of #20, 1/5 .
Adds the base hashing/RLP helpers used by QuarkChain types, including Merkle root and trie hash utilities.
Tests cover invalid Merkle inputs and serialization failures so consensus hashes fail loudly instead of silently hashing bad data.