diff --git a/.changeset/lovely-ghosts-beam.md b/.changeset/lovely-ghosts-beam.md new file mode 100644 index 00000000000..e5f21fbe2d4 --- /dev/null +++ b/.changeset/lovely-ghosts-beam.md @@ -0,0 +1,5 @@ +--- +"@atproto/pds": patch +--- + +Ensure we emit all relevant proof blocks for commit ops diff --git a/.changeset/twenty-worms-eat.md b/.changeset/twenty-worms-eat.md new file mode 100644 index 00000000000..3fe8af285ef --- /dev/null +++ b/.changeset/twenty-worms-eat.md @@ -0,0 +1,5 @@ +--- +"@atproto/repo": minor +--- + +Add relevant proof blocks to commit data diff --git a/.github/workflows/build-and-push-pds-ghcr.yaml b/.github/workflows/build-and-push-pds-ghcr.yaml index 8626816acfd..b11230ab531 100644 --- a/.github/workflows/build-and-push-pds-ghcr.yaml +++ b/.github/workflows/build-and-push-pds-ghcr.yaml @@ -3,7 +3,6 @@ on: push: branches: - main - - msieben/micro-optimizations env: REGISTRY: ghcr.io USERNAME: ${{ github.actor }} diff --git a/packages/pds/src/actor-store/repo/transactor.ts b/packages/pds/src/actor-store/repo/transactor.ts index a095e46cd4e..4f2ef192de9 100644 --- a/packages/pds/src/actor-store/repo/transactor.ts +++ b/packages/pds/src/actor-store/repo/transactor.ts @@ -128,12 +128,12 @@ export class RepoTransactor extends RepoReader { // find blocks that are relevant to ops but not included in diff // (for instance a record that was moved but cid stayed the same) - const newRecordBlocks = commit.newBlocks.getMany(newRecordCids) + const newRecordBlocks = commit.relevantBlocks.getMany(newRecordCids) if (newRecordBlocks.missing.length > 0) { const missingBlocks = await this.storage.getBlocks( newRecordBlocks.missing, ) - commit.newBlocks.addMap(missingBlocks.blocks) + commit.relevantBlocks.addMap(missingBlocks.blocks) } return commit } diff --git a/packages/pds/src/api/com/atproto/server/activateAccount.ts b/packages/pds/src/api/com/atproto/server/activateAccount.ts index 8d7c50476a4..d560bb763e0 100644 --- a/packages/pds/src/api/com/atproto/server/activateAccount.ts +++ b/packages/pds/src/api/com/atproto/server/activateAccount.ts @@ -50,6 +50,7 @@ export default function (server: Server, ctx: AppContext) { since: null, prev: null, newBlocks: blocks.blocks, + relevantBlocks: blocks.blocks, removedCids: new CidSet(), } }) diff --git a/packages/pds/src/scripts/rebuild-repo.ts b/packages/pds/src/scripts/rebuild-repo.ts index 05a9d534d1a..b02c627ab37 100644 --- a/packages/pds/src/scripts/rebuild-repo.ts +++ b/packages/pds/src/scripts/rebuild-repo.ts @@ -73,6 +73,7 @@ export const rebuildRepo = async (ctx: AppContext, args: string[]) => { since: null, prev: null, newBlocks, + relevantBlocks: newBlocks, removedCids: toDelete, } }) diff --git a/packages/pds/src/sequencer/events.ts b/packages/pds/src/sequencer/events.ts index aaa896c3819..4d7de593038 100644 --- a/packages/pds/src/sequencer/events.ts +++ b/packages/pds/src/sequencer/events.ts @@ -22,11 +22,15 @@ export const formatSeqCommit = async ( const blobs = new CidSet() let carSlice: Uint8Array + const blocksToSend = new BlockMap() + blocksToSend.addMap(commitData.newBlocks) + blocksToSend.addMap(commitData.relevantBlocks) + // max 200 ops or 1MB of data - if (writes.length > 200 || commitData.newBlocks.byteSize > 1000000) { + if (writes.length > 200 || blocksToSend.byteSize > 1000000) { tooBig = true const justRoot = new BlockMap() - const rootBlock = commitData.newBlocks.get(commitData.cid) + const rootBlock = blocksToSend.get(commitData.cid) if (rootBlock) { justRoot.set(commitData.cid, rootBlock) } @@ -46,7 +50,7 @@ export const formatSeqCommit = async ( } ops.push({ action: w.action, path, cid }) } - carSlice = await blocksToCarFile(commitData.cid, commitData.newBlocks) + carSlice = await blocksToCarFile(commitData.cid, blocksToSend) } const evt: CommitEvt = { diff --git a/packages/repo/src/mst/mst.ts b/packages/repo/src/mst/mst.ts index 8c567ca7b26..26db1f7315e 100644 --- a/packages/repo/src/mst/mst.ts +++ b/packages/repo/src/mst/mst.ts @@ -756,6 +756,20 @@ export class MST { return cids } + async addBlocksForPath(key: string, blocks: BlockMap) { + const serialized = await this.serialize() + blocks.set(serialized.cid, serialized.bytes) + const index = await this.findGtOrEqualLeafIndex(key) + const found = await this.atIndex(index) + if (found && found.isLeaf() && found.key === key) { + return + } + const prev = await this.atIndex(index - 1) + if (prev && prev.isTree()) { + await prev.addBlocksForPath(key, blocks) + } + } + // Matching Leaf interface // ------------------- diff --git a/packages/repo/src/repo.ts b/packages/repo/src/repo.ts index b1031cdd127..0a9bcf9ad0a 100644 --- a/packages/repo/src/repo.ts +++ b/packages/repo/src/repo.ts @@ -1,6 +1,7 @@ import { CID } from 'multiformats/cid' -import { TID } from '@atproto/common' +import { dataToCborBlock, TID } from '@atproto/common' import * as crypto from '@atproto/crypto' +import { lexToIpld } from '@atproto/lexicon' import { Commit, CommitData, @@ -69,6 +70,7 @@ export class Repo extends ReadableRepo { since: null, prev: null, newBlocks, + relevantBlocks: newBlocks, removedCids: diff.removedCids, } } @@ -140,11 +142,22 @@ export class Repo extends ReadableRepo { const newBlocks = diff.newMstBlocks const removedCids = diff.removedCids + const relevantBlocks = new BlockMap() + await Promise.all( + writes.map((op) => + data.addBlocksForPath( + util.formatDataKey(op.collection, op.rkey), + relevantBlocks, + ), + ), + ) + const addedLeaves = leaves.getMany(diff.newLeafCids.toList()) if (addedLeaves.missing.length > 0) { throw new Error(`Missing leaf blocks: ${addedLeaves.missing}`) } newBlocks.addMap(addedLeaves.blocks) + relevantBlocks.addMap(addedLeaves.blocks) const rev = TID.nextStr(this.commit.rev) const commit = await util.signCommit( @@ -157,21 +170,20 @@ export class Repo extends ReadableRepo { }, keypair, ) - const commitCid = await newBlocks.add(commit) - - // ensure the commit cid actually changed - if (commitCid.equals(this.cid)) { - newBlocks.delete(commitCid) - } else { + const commitBlock = await dataToCborBlock(lexToIpld(commit)) + if (!commitBlock.cid.equals(this.cid)) { + newBlocks.set(commitBlock.cid, commitBlock.bytes) + relevantBlocks.set(commitBlock.cid, commitBlock.bytes) removedCids.add(this.cid) } return { - cid: commitCid, + cid: commitBlock.cid, rev, since: this.commit.rev, prev: this.cid, newBlocks, + relevantBlocks, removedCids, } } @@ -208,6 +220,7 @@ export class Repo extends ReadableRepo { since: null, prev: null, newBlocks, + relevantBlocks: newBlocks, removedCids: new CidSet([this.cid]), } } diff --git a/packages/repo/src/sync/consumer.ts b/packages/repo/src/sync/consumer.ts index f09dc294d72..cf2716e24b0 100644 --- a/packages/repo/src/sync/consumer.ts +++ b/packages/repo/src/sync/consumer.ts @@ -93,6 +93,7 @@ export const verifyDiff = async ( prev: repo?.cid ?? null, since: repo?.commit.rev ?? null, newBlocks, + relevantBlocks: newBlocks, removedCids, }, } diff --git a/packages/repo/src/types.ts b/packages/repo/src/types.ts index abe20e04aaf..cda74bcbbbd 100644 --- a/packages/repo/src/types.ts +++ b/packages/repo/src/types.ts @@ -133,6 +133,7 @@ export type CommitData = { since: string | null prev: CID | null newBlocks: BlockMap + relevantBlocks: BlockMap removedCids: CidSet } diff --git a/packages/repo/tests/commit-data.test.ts b/packages/repo/tests/commit-data.test.ts new file mode 100644 index 00000000000..966290bdfbf --- /dev/null +++ b/packages/repo/tests/commit-data.test.ts @@ -0,0 +1,89 @@ +import { Secp256k1Keypair } from '@atproto/crypto' +import { MemoryBlockstore } from '../src/storage' +import { blocksToCarFile, Repo, verifyProofs, WriteOpAction } from '../src' + +describe('Commit data', () => { + // @NOTE this test uses a fully deterministic tree structure + it('includes all relevant blocks for proof in commit data', async () => { + const did = 'did:example:alice' + const collection = 'com.atproto.test' + const record = { + test: 123, + } + + const blockstore = new MemoryBlockstore() + const keypair = await Secp256k1Keypair.create() + let repo = await Repo.create(blockstore, did, keypair) + + const keys: string[] = [] + for (let i = 0; i < 50; i++) { + const rkey = `key-${i}` + keys.push(rkey) + repo = await repo.applyWrites( + [ + { + action: WriteOpAction.Create, + collection, + rkey, + record, + }, + ], + keypair, + ) + } + + // this test demonstrates the test case: + // specifically in the case of deleting the first key, there is a "rearranged block" that is necessary + // in the proof path but _is not_ in newBlocks (as it already existed in the repository) + { + const commit = await repo.formatCommit( + { + action: WriteOpAction.Delete, + collection, + rkey: keys[0], + }, + keypair, + ) + const car = await blocksToCarFile(commit.cid, commit.newBlocks) + const proofAttempt = verifyProofs( + car, + [ + { + collection, + rkey: keys[0], + cid: null, + }, + ], + did, + keypair.did(), + ) + await expect(proofAttempt).rejects.toThrow(/block not found/) + } + + for (const rkey of keys) { + const commit = await repo.formatCommit( + { + action: WriteOpAction.Delete, + collection, + rkey, + }, + keypair, + ) + const car = await blocksToCarFile(commit.cid, commit.relevantBlocks) + const proofRes = await verifyProofs( + car, + [ + { + collection, + rkey: rkey, + cid: null, + }, + ], + did, + keypair.did(), + ) + expect(proofRes.unverified.length).toBe(0) + repo = await repo.applyCommit(commit) + } + }) +})