Updates to SSZ Merkle proofs phase 0 doc #1186
Conversation
vbuterin
changed the title
|
|
|
Where's the "let's discuss on the next call" emoji where you need one 😄 |
|
fyi: the |
|
What's the status here @vbuterin ? |
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
specs/light_client/merkle_proofs.md
Outdated
| """ | ||
| o = GeneralizedIndex(1) | ||
| for i in indices: | ||
| o = o * get_previous_power_of_2(i) + i |
There was a problem hiding this comment.
Do I read this right? We do padding by the length that does not include the leading 1 bit, but not actually remove the bit from the index before appending the bits to the output?
Example of what it looks like is happening here (assuming get_previous_power_of_2(i) just returns the closest power of 2 at i or lower):
indices = [ # (in binary for example purposes)
0b10001
0b1011
0b110001
]
o = 1
o = 1 0000 # pad
o = 1 0 0001 # add
o = 1 0001 000 # pad
o = 1 0010 011 # add
o = 1 0001 011 00000 # pad
o = 1 0001 100 10001 # addWhat I expected:
o = 1
o = 1 0000 # pad
o = 1 0001 # add
o = 1 0001 000 # pad
o = 1 0001 011 # add
o = 1 0001 011 00000 # pad
o = 1 0001 011 10001 # addThere was a problem hiding this comment.
Fixed! For inputs (10001, 1011, 110001) the output should indeed be 1 0001 011 10001 now.
specs/light_client/merkle_proofs.md
Outdated
| """ | ||
| Returns the length of a path represented by a generalized index. | ||
| """ | ||
| return log(index) |
There was a problem hiding this comment.
Maybe use bit_length(), or at least log2()? And maybe explicitly rounded to an int, if choosing the log function?
There was a problem hiding this comment.
Fixed to log2. I was assuming we have log2 explicitly defined elsewhere; if not happy to do bit_length() instead.
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
| raise Exception(f"Type not supported: {typ}") | ||
|
|
||
|
|
||
| def get_item_position(typ: SSZType, index: Union[int, str]) -> Tuple[int, int, int]: |
There was a problem hiding this comment.
Worried about the [int, str] a bit, since they are exclusively (from each other) used based on typ. So e.g. Container with int is invalid. It is likely something to revisit with partials/other uses, but is ok for now.
There was a problem hiding this comment.
I think in all cases where we expect to use paths, the path would not be serialized, and it would be explicitly constructed directly as an input to get_generalized_index or similar methods (ie. never saved as a variable). So I think it's fine for that reason.
There was a problem hiding this comment.
Yes agree there, but we can maybe make it less error prone. We have others that are unfamiliar with the interface and need to use it as well.
Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
No description provided.