Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIP-0076: Add ListAllocations/ListClaims verified registry methods. #865

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

anorth
Copy link
Member

@anorth anorth commented Nov 14, 2023

No description provided.

FIPS/fip-0076.md Outdated
### Verified registry actor

The built-in verified registry actor exports FRC-0042 methods to iterate all allocations and claims.
`ListAllocations` returns all allocation IDs up to some caller-specified limit,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker and would like to suggest maybe rename to GetAllAllocations and GetAllClaims to match the names we have been using for other functions ,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any GetAllXXX in the linked FIP, or are there any in the actor codebase. There are a bunch of GetXXX methods, but these all return a single field. Sometimes that field is a short list, but it's always returned in full. These are the first built-in methods that iterate over a large collection and return partial results, a very different semantic. These names are consistent with FRC-0053 though.

@jennijuju
Copy link
Member

editor review ✅

could you please add a link to the implementation PR? would love @Stebalien @aarshkshah1992 @arajasek @LesnyRumcajs to give the proposed implementation spec & implementation a peer review.

This is somewhat the first time we introduce an function params like the cursor data structure - it would be great if we can have an example for clients on how to actually use it.

`ListAllocations` returns all allocation IDs up to some caller-specified limit,
and an opaque cursor which can be used to continue the listing where the first response left off.
`ListClaims` similarly returns all claim IDs up to some limit, and a cursor for continuation.
The order of iteration is undefined to the caller, and will match the internal HAMT structure's ordering.
Copy link
Member

@raulk raulk Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Peer review)

(I may be lacking some context here, but @jennijuju requested my input).

Do we have a sense of the gas costs of iterating over all entries in mainnet today, for each method? And maybe a cost projection for expected growth? My train of thought:

  1. I assume these are publicly exported because we expect EVM contracts (and upcoming Wasm actors) will wish to invoke them on-chain. If these methods existed only to encapsulate the logic for observability purposes, they wouldn't need to be exported, hence my assumption.
  2. The cursor as designed will be interrupted on a state update. This design only works if (a) the caller can fit the intended enumeration access all within a single transaction [atomic read], or (b) across transactions iff something guarantees that the verifreg state won't change halfway. AFAIK there is no such latter mechanism, and thus this spec diff rightly notes that a cursor is only valid immediately after return.

How do you foresee this interface being generally useful? I think stability across transactions is a desirable property. But the underlying fields being HAMTs make that difficult.

A design that awards some stability at the expense of some extra complexity involves taking advantage of the fact that HAMTs are persistent data structures and retaining a bounded buffer of size S of past roots so that they remain reachable from actor state after S mutations occurred. But even that solution presents variability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #730 (comment)

@@ -561,6 +561,63 @@ struct GetDealSectorReturn {
}
```

### Verified registry actor

The built-in verified registry actor exports FRC-0042 methods to iterate all allocations and claims.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editorial comment.

Can we specify explicit signatures for all methods being added in this FIP, as well as their computed FRC-0042 method number? The former can be reasonably assumed from data structures, and the latter can be derived with some work. But simply adding these details would reduce spec indirection, would make signatures normative, and the spec more self-contained/clearer. Thinking about library implementers like filecoin-solidity devs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the missing computed method numbers.

I'm not sure what you're requesting for a more explicit type signature. The data structures provide it. Are you requesting adding lines like:

method ProveCommitSectors2(ProveCommitSectors2Params) -> ProveCommitSectors2Response

These seem a bit redundant to me, but I can do so if you wish.

@anorth
Copy link
Member Author

anorth commented Nov 27, 2023

I'm merging this to unblock Last Call. We can add some form of method signatures if a clear schema for them is suggested, and I don't think that will constitute a substantive change to the FIP.

@anorth anorth merged commit adb91f4 into master Nov 27, 2023
1 check passed
@anorth anorth deleted the anorth/76-verifreg-api branch November 27, 2023 19:10
anorth added a commit that referenced this pull request Dec 4, 2023
anorth added a commit that referenced this pull request Dec 5, 2023
…thods. (#865)" (#877)

* Revert "FIP-0076: Add ListAllocations/ListClaims verified registry methods. (#865)"
This reverts commit adb91f4.

* Keep SectorContentChanged method number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants