Changes compared to the implementation #7
Conversation
| Any available graph roots MUST be sent as an array (`[CID]`) by the Client, regardless of the Client's Bloom filter. These roots form the basis of subquery anchors in the session. | ||
| Any missing graph roots MUST be sent as an array (`[CID]`) by the Client, regardless of the Client's Bloom filter. These roots form the basis of subquery anchors in the session. |
There was a problem hiding this comment.
I'm actually sending the CIDs of missing subgraphs, not the available ones. This may be a typo in the spec?
There was a problem hiding this comment.
I think it makes sense, yeah. The go implementation did the opposite (rationale beingh that you need to request fewer specific CIDs), so there's some previous history of trying to keep specs in sync.
There was a problem hiding this comment.
Interesting, yeah. I see how this would optimize the amount of network traffic. However, it also complicates the bookkeeping required on the client side, since it now needs to remember "up to where" it sent blocks, and what the current "frontier" of the DAG is so far, to filter out the available graph roots, so it knows the missing graph roots.
| Every CID visited is added to the Server's session cache. Each visited block is matched against both the Client Bloom filter and the Server's session cache. If a block is found in either structure, it is RECOMMENDED that the Server exclude it from the payload, and stop walking that path. Once the relevant graph has been exhausted, the Server closes the channel. | ||
| Each visited CID is matched against the Client Bloom filter. If a CID matches it, it is RECOMMENDED that the Server excludes the corresponding block from the payload, except if the CID was explicitly requested as a missing graph root. |
There was a problem hiding this comment.
In my implementation, the server never stores any sessions. The only state the server keeps is the blockstore itself (and I'm planning on adding memoization tables), but that's it.
If we're walking a block, I don't actually skip walking the whole subgraph. I'm skipping only that particular block.
That's the whole "subgraph interpretation" vs. "individual block interpretation" thing I wrote in discord about.
Also, I ignore whatever the bloom says if the client requested the block specifically in the subgraph roots. This way I don't actually have a "cleanup phase" conceptually in the code, it just happens via the subgraph root array.
| On receipt of each block, the Client adds the block to its local store, and to its copy of the Bloom. If the Bloom crosses a saturation point, the Client MUST resize it before the next round. | ||
| On receipt of each block, the Client adds the block to its local store. | ||
|
|
||
| At the end of each round, the Client MUST inspect its graph for any missing subgraphs. If there are incomplete subgraphs, the Client begins again with the roots of the missing subgraphs. If the Server fails to send a root, the Requester marks it as unavailable from that Server. The Client excludes unavailable roots from future rounds in the session. | ||
| At the end of each round, the Client MUST inspect its graph. Any blocks it has are added into an appropriately sized Bloom filter. | ||
|
|
||
| For any missing subgraphs, the Client begins a new round with them as the roots. If the Server fails to send a root, the Requester marks it as unavailable from that Server. The Client excludes unavailable roots from future rounds in the session. |
There was a problem hiding this comment.
This is a different way of saying the same thing.
Instead of talking about "resizing blooms" (which I think can be misleading, since you can't actually do that with simple bloom filters), I changed it to say that you're computing a bloom filter everytime.
| On a partial cold call, the Server Graph Estimate MUST contain the entire graph minus CIDs in the initial payload. The Server MUST respond with a Bloom filter of all CIDs that match the Server Graph Estimate, which is called the Server Graph Confirmation. On subsequent rounds, the Server Graph Estimate continues to be refined until it is empty or the entire graph has been synchronized. | ||
|
|
There was a problem hiding this comment.
I mainly removed this ... to communicate that I don't have that in the implementation?
I'm not 100% sure what the server graph estimate is for. In any case - it doesn't exist in the implementation.
Similarly, the diagram above is changed to match that reality: Blooms are only ever sent from the receiver to the sender to inform about any blocks the receiver may already have.
| At the end of each round, the receiving Server MUST respond with a Bloom filter of likely future duplicates, and an array of CID roots for pending subgraphs. Either MAY be empty, in which case it is treated merely as an `ACK`. | ||
| At the end of each round, the receiving Server MUST respond with a Bloom filter of CIDs of blocks it has in the subgraph or blocks that are likely future duplicates, and an array of CID roots for pending subgraphs. The Bloom filter MAY be empty, in which case it is treated merely as an `ACK`. | ||
|
|
||
| If the server responds with an empty list of pending subgraphs, the protocol is finished. |
There was a problem hiding this comment.
I don't actually send a bloom of "likely future duplicates", but instead the bloom contains blocks that the receiver already has. These are gathered from walking the DAG from the root and seeing what's available already.
This means there's just less state to track on the sending end, since upon receiving that bloom, it simply walks from the root again, skipping a whole bunch of blocks that have already been confirmed to have arrived.
Oh and sending an empty list of missing subgraph roots is interpreted as "I have everything".
| NB: The Bloom filter's bucket order (index) MUST be interpreted big-endian and zero-indexed. All hashes generated for indexing MUST be interpreted as big-endian natural numbers. | ||
| NB: The Bloom filter's bucket order (index) MUST be interpreted zero-indexed. |
There was a problem hiding this comment.
The XXHash hashes are actually never serialized. They come out of the library as ... u64s.
XXHash has some recommendations on serializing these u64s as big-endian, but that's irrelevant for our purposes.
| This case uses the nearest rounded power of two as described in the [Power of Two] section. If the sampled number is less than $m$, then it MUST be used as the index. If the number is larger, then right shift the unmasked number, take the required number of bits (e.g. via AND-mask) and check again. Repeat this process until the number of digits is exhausted. | ||
|
|
||
| If none of the samples succeed, a new hash MUST be generated and this process begun again. Hash generation MUST be performed via [XXH3] with the rehashing generation used as a seed (a simple counter, starting at 0). | ||
| This case uses the nearest rounded power of two as described in the [Power of Two] section. If the sampled number is less than $m$, then it MUST be used as the index. If the number is larger, then a new hash MUST be generated and this process begun again. Hash generation MUST be performed via [XXH3] with the rehashing generation used as a seed (a simple counter, starting at 0). | ||
|
|
||
| For example, if the filter has 1000 bits, take the lowest 10 bits (max 1024). If the number is less than than 1000, use that number as the index. Otherwise, right shift and try again. If the bits have been exhausted, rehash the full value and begin the process again. | ||
| For example, if the filter has 1000 bits, take the lowest 10 bits (max 1024). If the number is less than than 1000, use that number as the index. Otherwise rehash the full value with seed 1 and begin the process again, etc. |
There was a problem hiding this comment.
The rejection sampling I added in the deterministic-blooms crate actually doesn't try to "exhaust" all bits from one hash.
XXH is just so fast and that was so much bittwiddling work that may have bugs, that I decided to make rejection sampling simpler by just incrementing the seed everytime we reject.
As discussed @expede, I've collected some changes I made in the implementation compared to the spec.
In general, there's a lot more in the spec than the implementation. E.g. the spec assumes keeping a lot of session state. The implementation is almost completely stateless. (The only state being the client keeping the most recent response it got from the server in the case of pushes, and otherwise only relying on the current "root CID" to sync and the state of the blockstore).
Another thing the implementation doesn't do is for example everything in section
3.5. Similarity.I'll add comments to the sections I changed to explain the changes better.