Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Add Chain Data tables #63
feat: Add Chain Data tables #63
Changes from 9 commits
26784de
3174eff
fd028cc
4385338
f309a94
e75b651
bd7fd25
8d0f1d9
6c09d46
2277a31
fd63d83
9d21f6f
15cced4
5f8fece
3c51fa2
f9e4bf4
7bc9e25
0538ac0
9897981
c478f5f
4fb3716
af741fe
3118750
4fa5f95
8c1bc03
18e57c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A general comment about
sync_state
function: my original intent was for it to work slightly differently. Specifically:The
sync_state
request to the node gives us the next block containing requested data. It also gives uschain_tip
which is the latest block number in the chain. So, unlessresponse.block_header.block_num == response.chain_tip
we haven't synced to the tip of the chain yet.The idea was that we'd make these requests in a loop until
response.block_header.block_num == response.chain_tip
, at which point we know that we've fully synchronized with the chain.Each request also brings us info about new notes, nullifiers etc. created. It also returns Chain MMR delta that we can use to update the state of Chain MMR. This includes both chain MMR peaks and chain MMR nodes.
A naive way to update chain MMR is to load full
PartialMmr
at the beginning of this method and then callapply
on it for every response. There is a better way to do it - though, the details require more thought.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.
We can add the naive implementation to leave it in a working state, and change it a better one if we can come up with some.
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.
As mentioned in the previous comments, this will need to take an additional parameter for
chain_mmr_peaks
(this would beVec<Digest>
).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.
Now thinking about this, we might also need to pass the
forest
(u64
) to this as well because our goal is to be able to reconstruct PartialMmr that the client keeps track of. This requires:forest
:u64
- this would be in theblock_headers
table.peaks
:Vec<RpoDigest>
- this would also be in theblock_headers
table.nodes
:BTreeMap<InOrderIndex, RpoDigest>
- this would come from thechain_mmr_nodes
table.track_latest
:bool
- not sure where this should come from yet.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 this still needed?
forest
can be derived from peaks and thePartialMmr
constructor takes only thepeaks
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.
Not sure these methods are correct.
chain_mmr_nodes
table is meant to store nodes fromPartialMmr
struct. Basically, every row in this table would be a single entry in thenodes
map ofPartialMmr
.We probably don't want to insert the whole partial MMR every time - but rather only insert the resulting from each update.
So, the methods here should probably be a bit lower-level. Something like:
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.
So, to check that I undestand correctly:
insert_chain_mmr_nodes
should receive only thenodes
derived from each update, iterating over them, inserting each pair (InOrderIndex
,Digest
) in the table.get_chain_mmr_nodes
retrieves all rows from the table, and puts them inside aBTreeMap
.get_chain_mmr_paths
only retrieves the rows on the table whichInOrderIndex
matches any of the elements ofblock_numbers
.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 is correct. In the naive implementation this should be pretty simple as we can just take everything that was added to the
nodes
map after the last inserted node (new nodes would always have a bigger index).Correct.
It is a bit more involved as for each block number we'll need to return all nodes in the path from the block to the root of the corresponding peak. Let's leave this for another PR.
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.
Looking at the implementation of
InOrderIndex
it seems like there's no way to serialize this type, and neither access it's innerusize
, am I missing something?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, currently missing but I'm adding it in 0xPolygonMiden/crypto#238.