Availability store subsystem guide#1424
Availability store subsystem guide#1424montekki merged 5 commits intoparitytech:masterfrom montekki:fs-availability-store-subsystem-guide
Conversation
montekki
added
A0-please_review
montekki
changed the title
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
I'm only looking at 997f2ff because I believe that is the only commit unique to this PR. If I'm wrong, please let me know.
roadmap/implementers-guide/src/node/utility/availability-store.md
Outdated
Show resolved
Hide resolved
| - Note any newly-included candidates backed in the block. Update pruning records for any stored availability chunks. | ||
|
|
||
| On block finality events: | ||
| On `OverseerSignal::BlockFinalized(_)` events: |
There was a problem hiding this comment.
If we take my suggestion for distributing block finality events, then we can revert this line of change.
roadmap/implementers-guide/src/node/utility/availability-store.md
Outdated
Show resolved
Hide resolved
| fr3 -> fr4 [label = "No backing"] | ||
| fr3 -> occ4 [label = "Backing"] | ||
| occ3 -> occ4 [label = "(no change)"] | ||
| fr4 -> occ4 [label = "Availability Timeout"] |
There was a problem hiding this comment.
I think that arrow was accidentally backwards in the old diagram, but it should be the other way around.
There was a problem hiding this comment.
And actually i think it makes most sense to change it to occ3 -> fr3 now that i look closer. Since timeout occurs before backing.
| | .. | .. | .. | .. | | ||
| | CandidateN_M Included | .. | Candidate1_K Included | Candidate2_L Included | | ||
|
|
||
| > TODO: It's likely we will have to have a way to go from block hash to `BlockNumber` to make this work. |
|
|
||
| These are used to update Chunk pruning and PoV pruning records upon finality: | ||
| When another block finality notification is received: | ||
| - For any record older than this block: |
There was a problem hiding this comment.
Is this O(n) in the difference in number between the last finalized block number and the new one?
There was a problem hiding this comment.
I am worried that, especially while syncing, we may have large gaps in finality. If the node is offline for a day, or something like that. Furthermore, the races against runtime APIs might get worse in that case.
There was a problem hiding this comment.
Can we really avoid linear time here? I think that this can optimize things a bit:
- Suppose we have to store this info on blocks
[1000..2000].We could do two things here:- shrink the data kept in this one big blob to only keeping hashes and block numbers:
[(hash1000, 1000)...(hash2000, 2000)]and query into single records by some key likecached_availability_info_key(hash: &Hash). - we can also keep this blob sorted at a cheap price since we are likely to only have to append new blocks to it.
- shrink the data kept in this one big blob to only keeping hashes and block numbers:
- As soon as we get a new finality notification say about
hash1500, we can find the exact place to chop off the recent blocks atO(log(n))(I guess?) - Chop it off, update the individual records.
- Now we have
[(hash1501, 1501)..(hash2000, 2000)]array.
But in any case we are going to "touch" every one of them, but hopefully only once
There was a problem hiding this comment.
in any case, i have to give it another thought
There was a problem hiding this comment.
Yeah, that is a good point. I guess what I'd want to avoid is that the subsystem gets stuck in a millions-of-blocks loop. It seems better to me to only adjust pruning some number of blocks behind the head and accidentally leak data when doing larger synchronizations. Maybe there is some room for a behind-the-scenes cleanup that does not tie up the message-processing task as well. Although I would want to defer that to a later issue, as leaking data is not a big issue and we are not implementing pruning right now yet.
|
Rebase on master? |
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
3 participants
This is based off #1401