Feat/#1752 fix witness generation - adding testing #1784
Conversation
github-actions
bot
added
the
crate-geth-utils
KimiWu123
force-pushed
the
feat/#1752-fix-witness-generation-test
branch
from
4c239d8 to
05f358f
Compare
| @@ -628,7 +635,7 @@ func (st *StackTrie) UpdateAndGetProofs(db ethdb.KeyValueReader, list types.Deri | |||
| // order that `list` provides hashes in. This insertion sequence ensures that the | |||
| // order is correct. | |||
| var indexBuf []byte | |||
| for i := 1; i < list.Len() && i <= 0x7f; i++ { | |||
| for i := 0; i < list.Len() && i <= 0x7f; i++ { | |||
There was a problem hiding this comment.
Hi @miha-stopar , have a question regarding the starting index.
Why doest the for loop start from 1 instead of 0? I saw the case of index 0 was handled in L649-L654. But there is only Update being called, no GetProof. Wondering what will happen if I change to start from 0 and comment out the L649-L654? There is also a simliar case in L655, there is no GetProof after index is 0x80. Wondering the reason why separate into many parts.
There was a problem hiding this comment.
Yeah, good question :). I tried to remember, but can't say for sure. I guess there was some bug in GetProof and I was debugging it. I think the index can start with 0 and we can delete these two if blocks.
There was a problem hiding this comment.
I had another look and I was wrong in my previous comment - it has to start with 1. The original code has it this way too: https://github.com/ethereum/go-ethereum/blob/00905f7dc406cfb67f64cd74113777044fb886d8/core/types/hashing.go#L118.
The reason for this is that for i = 0, we get indexBuf = [128] and the following line
k := KeybytesToHex(key)
gives us k = 8. So with i = 0, we insert the leaf into the branch at position 8.
Then, when we are inserting the transaction number 128 we again try to insert the leaf at position 8 (in this case k = [8, 0, 16], but it was already hashed and we cannot do this.
This is why the index 0 is skipped and it starts with 1. And it's why the following two blocks are needed:
if list.Len() > 0 {
indexBuf = rlp.AppendUint64(indexBuf[:0], 0)
value := encodeForDerive(list, 0, valueBuf)
hasher.Update(indexBuf, value)
}
for i := 0x80; i < list.Len(); i++ {
indexBuf = rlp.AppendUint64(indexBuf[:0], uint64(i))
value := encodeForDerive(list, i, valueBuf)
hasher.Update(indexBuf, value)
}
Sorry for the confusion!
There was a problem hiding this comment.
Do you mean it inserts at position 8 when i = 0? If so, which position will be inserted into when i = 128?
There was a problem hiding this comment.
For i = 128, we have key = [8, 1, 8, 0], that means it's inserted in the branch at position 8 (which is a branch that has the transaction with i = 0 at the position 0 - it was added in the block if list.Len() > 0). The second nibble is 1, so it's inserted at position 1 in the branch.
The state after i = 128 is:
- top level is a branch with branches at positions 0 - 7 (all these branches are fully filled),
- at position 8 in a top level branch we have a branch with two children at positions 0 (
i = 0) and 1 (i = 128).
There was a problem hiding this comment.
Thanks for detailed explanation again and I think I know how it works 🤞
KimiWu123
changed the title
|
Hey @miha-stopar , I think I fixed everything. Could you please give it a quick review again. |
| st.children[idx].keyOffset = st.keyOffset + 1 | ||
| st.children[idx] = newLeaf(st.keyOffset+1, key, value, st.db) | ||
| } else { | ||
| st.children[idx].insert(key, value) |
There was a problem hiding this comment.
Not sure what's the reason for this change?
There was a problem hiding this comment.
Oh, I just followed how geth implemented it (https://github.com/ethereum/go-ethereum/blob/00905f7dc406cfb67f64cd74113777044fb886d8/trie/stacktrie.go#L211-L215). Besides, it looks more self-explanation for me, one case is to new a leaf and the other is to insert a node. The original one seems too optimized and not easy to understand.
| @@ -631,33 +662,40 @@ func (st *StackTrie) UpdateAndGetProofs(db ethdb.KeyValueReader, list types.Deri | |||
| for i := 1; i < list.Len() && i <= 0x7f; i++ { | |||
There was a problem hiding this comment.
No change is requested here. I just comment to inform the future reviewers.
I saw the duplicated code chunks in L662's loop body, L674's condition body, and L684's loop body.
I was wondering why the duplication but the I saw your comment on L651, which says Geth is also implemented in the same style.
Description
closed #1752
GetProof(if input is an ext node, it always assign first child of the root ext. node,st.children[0])getNodeFromBranchRLP(if input is an leaf, it could throw an exception)getNodeFromBranchRLPIssue Link
#1752
Type of change