FilOzone / pdp

Smart contracts and utilities for Proof of Data Possession
Other
2 stars 2 forks source link

Allow Spark to observe added/removed roots #79

Open bajtos opened 3 days ago

bajtos commented 3 days ago

Hello from the Spark team :wave:

I reviewed the docs and the PDPVerifier contract to understand how can Spark maintain a list of Root CIDs that are covered by PDP checks.

I found the following four events - I think they cover all state changes relevant to us πŸ‘πŸ»

https://github.com/FilOzone/pdp/blob/19993af01548f90a8ee0d8822f6ce885dd1cc16d/src/PDPVerifier.sol#L31-L34

I can imagine how a PDP observer can use ProofSetCreated and ProofSetDeleted events to maintain a view of all active proof sets πŸ‘πŸ»

However, I don't understand how we can maintain a view of all active roots.

Question 1: observing when roots are added to a root-set

https://github.com/FilOzone/pdp/blob/14fd1c9225471aab4a517abdaf4791c8d9963fd4/contracts/src/PDPVerifier.sol#L280-L282

Let's say somebody calls addRoots() with the following arguments:

setId=1
rootData=[ {cid: 'baf1', rawSize: 64}, {cid: 'bafy2', rawSize: 256 } ]

The event RootsAdded provides only firstAdded argument, which seems to be the rootId index into the three per-proof-set mappings: https://github.com/FilOzone/pdp/blob/19993af01548f90a8ee0d8822f6ce885dd1cc16d/src/PDPVerifier.sol#L323-L325

Using my example above, how can I reconstruct setId=1 and rootData=[ {cid: 'baf1', rawSize: 64}, {cid: 'bafy2', rawSize: 256 } ] from the firstAdded field reported by the RootsAdded event?

I think you need to add at least setId field to the event payload.

If we can assume that addRoots() will map individual roots to root ids that are in sequence (i.e. firstRoot, firstRoot+1, ... firstRoot+N-1), then the combination of setId and firstRoot allows the observer to iterate over all roots. The remaining question is how to know when to stop the iteration (what was the length of the rootData array).

If my reasoning is correct, then we need to change the event signature to the following one:

event RootsAdded(uint256 indexed setId, uint256 indexed firstAdded, uint256 addedCount);

While thinking about this more:

(1) I don't understand why is firstAdded indexed - how do you envision the client queries that will use this index? It makes sense to me to look up events linked to a specific root set (via setId). I struggle to find a use case for looking up the events using the firstAdded value. (If I wanted to look up the event by root id, then how would I go from a particular root id to the "firstAdded" value emitted when the root was aded?)

Proposal:

event RootsAdded(uint256 indexed setId, uint256 firstAdded, uint256 addedCount);

(2) I find the name "firstAdded" very confusing, it does not tell me how to interpret the value. Something like idOfTheFirstRootAdded is easier to understand, but also rather verbose.

Here is a suggestion that works well for me:

event RootsAdded(uint256 indexed setId, uint256 firstRootId, uint256 lastRootId);

Then I can run the following code to reconstruct the root CIDs & sizes that were added to the proof set:

for (let rootId = firstRootId; rootId <= lastRootId; rootId++) {
  const cid = rootCids[setId][rootId]
  const size = rootLeafCounts[setId][rootId] * LEAF_SIZE
  // add {rootId, cid, size} to our view of roots
}

Alternatively, can we model the RootsAdded event after RootsRemoved event that provides an array of root ids? I think that's the most elegant solution. (But I understand it could be more expensive on the gas fees.)

event RootsAdded(uint256 indexed setId, uint256[] rootIds);

Question 2: observing when roots are removed from a root-set

https://github.com/FilOzone/pdp/blob/14fd1c9225471aab4a517abdaf4791c8d9963fd4/contracts/src/PDPVerifier.sol#L329-L331

This is similar to the previous question.

First, the event RootsRemoved does not seem to be emitted at all. Huh?? There is only one source code line containing the string RootsRemoved: https://github.com/search?q=repo%3AFilOzone%2Fpdp%20RootsRemoved&type=code

Let's say somebody calls scheduleRemovals() with the following arguments:

setId=1
rootIds = [10, 20]

How can a party observing PDP state learn:

bajtos commented 2 days ago

Question 3: linking new root-sets to their owners

I imagine we will need to filter which root-sets we watch for changes, e.g. to watch only root-sets created by Storacha.

A possible solution is to use the root-set owner address as the filter (e.g. watch only root-sets owned by one of Storacha wallet addresses).

We need two features to enable that:

  1. When a new root-set is created, get the information about the owner.
  2. When an owner is changed, get a notification (event) about that.

The first requirement can be met with the current PDPVerifier version by calling getProofSetOwner in the handler for the event ProofSetCreated.

It would be more ergonomic for the consumers if ProofSetCreated included the owner in the payload. IMO, it makes sense to index this field, so that consumers can filter ProofSetCreated events using the owner address.

event ProofSetCreated(address indexed owner, uint256 indexed setId);

I don't see how to meet the second requirement with the current PDPVerifier version. I propose adding a new event emitted by claimProofSetOwnership():

event ProofSetOwnerChanged(address indexed oldOwner, uint256 indexed setId, uint256 indexed newOwner);