Fix missing delete blocks #3033
Conversation
|
Would be good to get some test cases for this. (Edit: oops, didn't spot there was a test case in this PR already!) I could see it happening when
(overall, 1 block has been deleted, and the new tree has 0 "new" blocks) Unconditionally including the new root MST block would solve for this case, but are there others? Edit: nope, of course it's not that simple 🙃 |
| const blocksToSend = new BlockMap() | ||
| blocksToSend.addMap(commitData.newBlocks) | ||
| blocksToSend.addMap(commitData.relevantBlocks) |
There was a problem hiding this comment.
Can we omit commitData.newBlocks here? If I understood correctly, the way things are currently written every "new" block is "relevant". Or perhaps this is just a way to be defensive around missing any blocks.
There was a problem hiding this comment.
This will de-duplicate fwiw so we won't actually be sending along double blocks.
The way it's currently written "new blocks" are not all necessarily "relevant". Maybe a better word for "relevant blocks" would be "proof blocks".
As an example take this edge case of a node split from the MST tests: https://github.com/bluesky-social/atproto/blob/main/packages/repo/tests/mst.test.ts#L328
The newly added record (f) has very few blocks that are relevant to it's proof (just the root of the tree). However the new MST has many new blocks in it to compensate for the split
Some events that we were sending out over the PDS firehose were missing blocks necessary to validate the included proofs. This seems to have been primarily happening on events that contained a single delete, but hypothetically could happen on any write.
It seems like this was being caused by a reorganization of some blocks in the tree. Such that a block that was in the proof chain for delete in question existing previously in the tree. The differ would not return it as a new block in that case which is strictly true but not sufficient for a non-fully-authorized relay consumer.
This adds in the notion of "relevant blocks" to commit data as well. This include exactly the blocks that make up the proofs for the included operations.
This is an overlapping set with new blocks (often the same set). But "new blocks" can contain blocks not necessary for the included proof & similarly relevant blocks can include already existing blocks that are relevant to the proof.
When sending an event from the PDS, we send the union of these two block maps.