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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9ffb9d
Add ListAllocations/ListClaims verified registry methods.
anorth Nov 14, 2023
361dbe9
Add computed method numbers and link to ListAllocations/ListClaims PR.
anorth Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions FIPS/fip-0076.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

`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.

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 •
edited
Loading

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)

A cursor is tied to the actor state from which it was generated.
Clients can only assume a cursor is valid for use immediately after it is returned.

```
// Exported API
struct ListAllocationsParams {
Cursor: RawBytes,
Limit: u64,
}

struct AllocationKey {
Client: ActorID,
ID: AllocationID,
}

struct ListAllocationsResponse {
Allocations: Vec<AllocationKey>,
NextCursor: Option<RawBytes>,
}

struct ListClaimsParams {
Cursor: RawBytes,
Limit: u64,
}

struct ClaimKey {
Provider: ActorID,
ID: ClaimID,
}

struct ListClaimsResponse {
Claims: Vec<ClaimKey>,
NextCursor: Option<RawBytes>,
}
```

A cursor is a serialized representation of the HAMT root CID, the next client/provider ID,
and the next allocation/claim ID for that client/provider.
A cursor is rejected with `USR_ILLEGAL_ARGUMENT` if the HAMT root does not match the verified registry state,
or the IDs do not exist.

```
// Internal structure
struct Cursor {
Root: Cid,
OuterKey: ActorID,
InnerKey: u64, // Allocation or claim ID
}

```
### Migration

The built-in market actor's `ProviderSectors` mapping is initialised from the existing deal state
Expand Down Expand Up @@ -641,6 +698,12 @@ as a side effect of publishing a deal.
The reason for this change is that verified allocations and deals are independent: a client can allocate DataCap
without necessarily involving the built-in market actor, and save the SP significant gas costs by doing so.

### Verified registry iteration APIs

The methods to list allocations and claims provide simpler accessibility of these structures,
since in many cases they may be the only on-chain representation of a "deal".
The methods are primarily intended to be invoked from off-chain, replacing direct state inspection.

## Backwards Compatibility

This proposal deprecated the miner methods `PreCommitSector` (method 6), `PreCommitSectorBatch` (method 25),
Expand Down