feat(pdp): add transaction tracking for root additions (synapse branch)

[rvagg]

• Add Location header to root addition response with tx hash
• Add GET /proof-sets/{id}/roots/added/{txHash} endpoint for status checks
• Implement handleGetProofSetRoot for retrieving root details
• Return confirmed root IDs after transaction confirmation
• Use lowercase transaction hashes consistently in database operations

[rvagg]

@LexLuthr this is also probably appropriate for main branch too, let me know if you want me to open it there as well so we have less to reconcile in the future. There may be other things we want to look at where this branch has diverged because we've solved a bunch of problems already that are relevant to the main branch, but we have a few temporary compromises that shouldn't be ported across (like null auth and probably the shorter confirmation time, although shortening it from what it is on main would be good). I'm worried about them getting so out of sync that reconciling later on is going to be hard and I'm happy to help squash and pick things apart if you want.

@aarshkshah1992 would you mind reviewing this please?

[rvagg]

Forgot to mention this is related to this issue: #520 and it will be closed by merging.

[aarshkshah1992]

what is AddMessageIndex ?

[rvagg]

All quite well documented here actually:

curio/harmony/harmonydb/sql/20240930-pdp.sql

Lines 149 to 170 in 29f2fe9

	-- proofset root adds - tracking add-root messages which didn't land yet, so don't have a known root_id
	CREATE TABLE pdp_proofset_root_adds (
	proofset BIGINT NOT NULL, -- pdp_proof_sets.id
	root TEXT NOT NULL, -- root cid (piececid v2)
	add_message_hash TEXT NOT NULL REFERENCES message_waits_eth(signed_tx_hash) ON DELETE CASCADE,
	add_message_ok BOOLEAN, -- set to true when the add message is processed
	add_message_index BIGINT NOT NULL, -- index of root in the add message
	-- aggregation roots (aggregated like pieces in filecoin sectors)
	subroot TEXT NOT NULL, -- subroot cid (piececid v2), with no aggregation this == root
	subroot_offset BIGINT NOT NULL, -- offset of the subroot in the root (padded byte offset)
	subroot_size BIGINT NOT NULL, -- size of the subroot (padded piece size)
	pdp_pieceref BIGINT NOT NULL, -- pdp_piecerefs.id
	CONSTRAINT pdp_proofset_root_adds_root_id_unique PRIMARY KEY (proofset, add_message_hash, subroot_offset),
	FOREIGN KEY (proofset) REFERENCES pdp_proof_sets(id) ON DELETE CASCADE, -- cascade, if we drop a proofset, we no longer care about the roots
	FOREIGN KEY (pdp_pieceref) REFERENCES pdp_piecerefs(id) ON DELETE SET NULL -- sets null on delete so that it's easy to notice and clean up
	);

add_message_index BIGINT NOT NULL, -- index of root in the add message

So an "AddRoots" can be multiple roots, and this just gives you an index for that list. I'm not sure how useful that is to the end user, and we already use add_message_index as the ORDER BY, so 🤷 Is this just going to be 0...? I guess so.

[rvagg]

I'm open to not including this on the response btw, I think it's mainly useful for sorting and I think there's an implicit assumption that this will be 0...n in the results anyway that should hole true.

[aarshkshah1992]

Reviewed

[rvagg]

I added a new commit here after I ran into problems actually using this - the immediate deletion of the record when the tx lands means that querying for it fails. But we don't do this for createproofset so I've now aligned the impl to that:

fix(pdp): retain root addition records to prevent 404s on status queries

Previously, pdp_proofset_root_adds records were deleted after processing,
causing clients to receive 404 errors when querying transaction status
after successful root additions. This created a poor UX as clients couldn't
verify transaction outcomes if they queried too late.

This change aligns root addition tracking with proof set creation by:
- Adding roots_added column to mark processed records instead of deleting
- Updating watcher to UPDATE instead of DELETE when processing completes
- Including roots_added status in API response for client visibility

The fix ensures transaction status remains queryable indefinitely while
maintaining backward compatibility. Existing records will be processed
normally with roots_added=FALSE by default.

API enhancement: /pdp/proof-sets/{id}/roots/added/{txHash} now includes
"rootsAdded" field indicating if roots were fully processed.

[aarshkshah1992]

lgtm

[ZenGround0]

Comment misleading since in general its not JWT and its hardcoded to not be JWT right now

[rvagg]

Yeah, but I'm just following the pattern in the file that exists now:

$ grep 'Verify that the request' pdp/handlers.go 
        // Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT
        // Step 1: Verify that the request is authorized using ECDSA JWT

Do you want me to remove all of those, or just these new ones?

[ZenGround0]

I'm not so sure what value we get out of this. It feels like we're testing the consistency model of yugabyte and I don't really care as a user for the purposes of this call. I actually just care about the blockchain consistency model.

[ZenGround0]

Similar feeling about RootsAdded return value. As user I care about chain and upload commitment and this seems to be some extraneous other bookkeeping.

[rvagg]

OK, your call, Aarsh and I went back and forth on this and I don't think I care. I'll remove the checks and rootsAdded.

[rvagg]

Oh, actually no I remember now - we added this new field to the database, roots_added, it's the final check that tells us that Curio has fully processed it and it's onboarded. It's the direct equivalent of proofset_created. SO I need roots_added, it's just a question of whether we're OK trusting that the first row we get from the DB is representative of the whole lot. Perhaps there's a Curio-bug-reason that makes the first one false and the rest true; that's the logic that lead to all the looping and checking. So, your call on this but I'd really like the roots_added from for the SDK

[ZenGround0]

Whats the rationale on returning anyway with no root IDs? It would be simpler for the client if this were to just fail right?

[rvagg]

Because we could be in an in-between state. We get into this branch because add_message_ok is set on the pdp_proofset_root_adds table, but it's set because of a change in transaction status on the message_waits_eth table by the pdp_proofset_add_message_status_change stored procedure. Then, separate to that the WatcherRootAdd task polls pdp_proofset_root_adds to look for these add_message_ok and roots_added status changes and eventually updates pdp_proofset_roots which is what we are reading here.

So there is a moment where rootAdds[0].AddMessageOK is true but this SELECT statement won't return anything because our background task hasn't followed up on it. If we fail at that point then we're telling the user that there's a problem completing the root add, but in reality we're just closing it out and the next time they query it should return everything is 👍 .

[ZenGround0]

Overall I like it

[ZenGround0]

nit: comment