-
Notifications
You must be signed in to change notification settings - Fork 857
Conversation
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.
I added some comments on the style
geth-utils/gethutil/mpt/witness/gen_witness_transactions_test.go
Outdated
Show resolved
Hide resolved
geth-utils/gethutil/mpt/witness/gen_witness_transactions_test.go
Outdated
Show resolved
Hide resolved
geth-utils/gethutil/mpt/witness/gen_witness_transactions_test.go
Outdated
Show resolved
Hide resolved
|
||
proof = append(proof, c_rlp) | ||
if len(branchChild) == 1 { | ||
// no child at this position - 128 is RLP encoding for nil object |
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.
question, you said 128 is RLP encoding for nil object
, but you didn't do any checks like your previous code (node[0] == 128
). So, do we need to add node[0] == 128
? Or len(branchChild)
is enough to represent that there is no child for this branch?
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.
I excluded node[0] == 128
because the break is needed also in the case when c_rlp
is a transaction leaf (and not a branch or extension node). Would be good to have some more tests to verify this behaviour though. I added a comment here 16d93ad
[248 81 128 160 32 244 75 78 180 11 251 73 229 254 70 16 254 170 54 254 200 97 231 24 180 34 220 244 153 76 1 194 23 63 64 224 160 46 141 2 37 188 204 110 232 46 31 230 82 226 213 98 71 18 241 37 90 213 167 221 58 33 36 249 248 180 207 235 47 128 128 128 128 128 128 128 128 128 128 128 128 128 128] | ||
[248 200 2 131 4 147 224 131 1 226 65 148 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 4 184 100 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 37 160 163 65 77 46 20 3 175 162 34 86 182 41 225 43 90 92 158 249 153 67 193 148 178 63 8 81 26 169 176 56 242 78 160 21 37 82 209 254 5 113 171 235 198 244 52 17 233 162 51 76 151 85 224 28 101 146 160 197 216 253 237 240 187 19 184] | ||
|
||
Note that the first proof element is an extension node with nibble 0 = 16 - 16 (see |
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.
This explanation is really nice and clear, and I wish I could have them before I started this issue. Thanks for adding this!
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. Feel free to merge it after CC's feedback is addressed.
When further 13 transactions are added, the branch B gets populated at positions 3 - 15 | ||
(the position 0 remains nil). | ||
|
||
When the 16-th transaction is added (it has index 16, it gets changed to [1, 0, 16]), |
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.
Here should be 17-th
transaction, isn't it? Index is from 0 to 15 and index 16 means the 17th element.
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.
I think it's 16-th, because index 0 is not occupied.
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! Nice cleanup and adding docs.
Note: We found that actions/setup-go@v4
has cache built in, so that some existing golang caching steps can be removed.
Thanks @ChihChengLiang and @adria0 for helping with CI problems! |
Description
GetProof
methodTestTransactions
Issue Link
#1752
Type of change
How Has This Been Tested?
TestTransactions
andTestGetProof
inzkevm-circuits/geth-utils/gethutil/mpt/witness/gen_witness_transactions_test.go
.