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-0083: Add built-in Actor events in the Verified Registry, Miner and Market Actors #872

Merged
merged 11 commits into from
Jan 4, 2024

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Nov 27, 2023

This FIP defines and proposes the implementation of a meaningful subset of fire-and-forget externally observable events that should be emitted by the built-in Verified Registry, Storage Market and Storage Miner Actors. These enable external agents (henceforth referred to as “clients”) to observe and track a specific subset of on-chain data-cap allocation/claims, deal lifecycle and sector lifecycle state transitions.

While this FIP explicitly delineates the implementation of events tied to specific transitions, it does not encompass the full spectrum of potential built-in actor events. The introduction and integration of additional built-in actor events will be addressed in subsequent FIPs.

Discussion: #754

@aarshkshah1992 aarshkshah1992 changed the title [WIP] Draft FIP for built-in Actor events: Yet to be numbered FIP-XXX: Add built-in Actor events in the Verified Registry, Miner and Market Actors Nov 27, 2023
@aarshkshah1992 aarshkshah1992 marked this pull request as ready for review November 27, 2023 11:00
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

A bunch of small editorial requests, otherwise looks good. Will be happy to approve after a round of tweaks.

FIPS/fip-xxxx.md Outdated
---

## Summary
This FIP defines and proposes the implementation of a meaningful subset of fire-and-forget externally observable events that should be emitted by the built-in Verified Registry, Storage Market and Storage Miner Actors. These enable external agents (henceforth referred to as *“clients”*) to observe and track a specific subset of on-chain data-cap allocation/claims, deal lifecycle and sector lifecycle state transitions.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "that should be" -> "to be" or "that will be". Directly describe the end state after this is correctly implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
Filecoin network observability tools, Block explorers, SP monitoring dashboards, deal brokering software etc need to rely on complex low-level techniques such as State tree diffs to source information about on-chain activity and state transitions. Emitting the select subset of built-in actor events defined in this FIP will make it easy for these clients to source the information they need by simply subscribing to the events and querying chain state.

## Specification
```
Copy link
Member

Choose a reason for hiding this comment

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

I suggest against using code blocks for asides. This would be a new convention that I don't think is justified here. Just plain text is fine, or maybe an italic for the first sentence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated

## Specification
```
Please see FIP-0049 for a detailed explanation of the FVM event schema and the possible values each field
Copy link
Member

Choose a reason for hiding this comment

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

Please link FIP-0049

FIPS/fip-xxxx.md Outdated
Comment on lines 51 to 54
```
As specified in FIP-0049, events emitted by Actor methods are not persisted to message receipts and
rolled back if the Actor method emitting the event itself aborts and rolls back.
```
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unnecessary and can be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
rolled back if the Actor method emitting the event itself aborts and rolls back.
```

### FVM should support CBOR encoding
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### FVM should support CBOR encoding
### FVM support for CBOR encoding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

FIPS/fip-xxxx.md Outdated
Comment on lines 191 to 194
```
A new ProveCommitSectors2 method will be added as part of FIP-0076 post which we will have to update
that method to emit the sector activation events as well.
```
Copy link
Member

Choose a reason for hiding this comment

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

The text doesn't refer to specific activation methods, so doesn't need to refer to future ones either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
Comment on lines 242 to 250
Once a deal is published, clients have access to the `dealId` of the deal. All Market Actor events except
for the “deal published” event have the dealId in their payload which clients can use to filter for events
they are interested in and then query the chain state for more information.

The `dealId` is not known to the storage client before the deal is actually published as it is generated by
the storage provider during the call to `PublishStorageDeals`. Therefore, to make the `deal published`
event useful to subscribers and storage clients, the payload for that event also includes the `client`
and `provider` Actors IDs so that listeners can filter this event by specific deal making parties they are
interested in.
Copy link
Member

Choose a reason for hiding this comment

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

All of this is the same for verifreg too, so we should end up with one rationale for both. Discussion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
schedules a cron job to actually activate the sector which means that the sector activation event will be
emitted by the cron job. This makes the event un-usable as events emitted by a cron job are not included
in the message receipt. However, we still emit the sector activation event in the Miner Actor for
completeness. A future FIP can ensure that sector activation is actually done as part of the
Copy link
Member

Choose a reason for hiding this comment

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

FIP-0076

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
Comment on lines 321 to 329
One other important incentive consideration to callout here is that this FIP increases the gas cost of
the `ProveCommitAggregate` method vis a vis the `ProveCommitSector` method even more as the former will
have to pay the gas costs for emitting the sector activation events whereas for the latter, the event
will be emitted in the corresponding sector activation cron job. We already have a pre-existing
*“protocol bug”* wherein storage providers are sometimes incentivised to use the `ProveCommitSector`
method to active a sector so that their sector activation gas costs get subsidised by the cron job and
this FIP slightly exacerbates the problem. However, we are well aware of this problem and there are plans
to fix this gas cost discrepancy by deprecating the `ProveCommitSector`
method once [FIP-0076](https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0076.md) lands.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is worth mentioning here, unless gas cost calculations show this to be a nontrivial part of the total. Just exclude it for now until we have measurements.

Note that phrases like "we have plans" don't belong in a FIP: just state the objective effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

FIPS/fip-xxxx.md Outdated
Comment on lines 339 to 342
Clients will still have to rely on pre-existing mechanisms for observability of transitions not captured
by events defined in this FIP. However, we will propose and implement FIPs in the future to capture the
entire spectrum of meaningful built-in actor events to enable clients to completely move away from
pre-existing introspection mechanisms.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: omit the "we will" part. You could say "future FIPs could ...", but that's also obvious so I don't think worth stating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@jsoares jsoares left a comment

Choose a reason for hiding this comment

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

Just minor editorial/language fixes

FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
FIPS/fip-xxxx.md Outdated Show resolved Hide resolved
aarshkshah1992 and others added 2 commits December 14, 2023 09:40
Applying editorial changes.

Co-authored-by: Jorge Soares <[email protected]>
@anorth
Copy link
Member

anorth commented Dec 14, 2023

Please take FIP number 0083. Please update the headers, change the filename, and add a line to the root README linking to it.

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

LGTM after adding the FIP number. I know there are some minor changes expected (e.g. CIDs in sector onboarding). Let's add them and any other changes from the discussion in a follow-up PR.

@aarshkshah1992 aarshkshah1992 changed the title FIP-XXX: Add built-in Actor events in the Verified Registry, Miner and Market Actors FIP-0083: Add built-in Actor events in the Verified Registry, Miner and Market Actors Dec 15, 2023
@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Dec 15, 2023

@anorth Addressed your review.

FIPS/fip-0083.md Outdated Show resolved Hide resolved
FIPS/fip-0083.md Outdated Show resolved Hide resolved
FIPS/fip-0083.md Outdated Show resolved Hide resolved
FIPS/fip-0083.md Outdated Show resolved Hide resolved

| flags | key | value |
| --- | --- |-------------------------|
| Index Key + Value | “$type” | “allocation” (string) |
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, is there any reason why this can't just be an index key allocation?

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Jan 2, 2024

Choose a reason for hiding this comment

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

We want users to be able to quickly query for/filter for events where (key="$type" AND value="allocation").

| Index Key + Value | “$type” | “allocation” (string) |
| Index Key + Value | “id” | <ALLOCATION_ID> (int) |
| Index Key + Value | “client” | <CLIENT_ACTOR_ID> (int) |
| Index Key + Value | “provider” | <SP_ACTOR_ID> (int) |
Copy link
Member

Choose a reason for hiding this comment

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

peer: I think there is a value to also expose amount as a value in this event for datacap explorers - so they can get metrics like total datacap allocated by simply subscribing the events without having to query the allocation state

Copy link
Member

Choose a reason for hiding this comment

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

similarly, would be great if we can also expose the pieceCID, thinking of FIL+ bot and such tools can take advantage of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Captured in FIP discussion at #754 (comment).

@jennijuju
Copy link
Member

with the goal to monitor datacap events, I think we could use this chance to add events to datacap actor as well, starting with mint, transfer, transfer_from methods. Im actually a bit surprised they aren't already emited and I am looking at implementation like here. @anorth please correct me if im wrong

| Index Key + Value | “$type” | “claim” (string) |
| Index Key + Value | “id” | <CLAIM_ID> (int) |
| Index Key + Value | “client” | <CLIENT_ACTOR_ID> (int) |
| Index Key + Value | “provider” | <SP_ACTOR_ID> (int) |
Copy link
Member

Choose a reason for hiding this comment

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

similar suggestion on adding the amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Captured in FIP discussion at #754 (comment).

@jennijuju
Copy link
Member

@aarshkshah1992 thank you for the FIP! in general the FIP is well motivated and clearly written, and I think it is okay to merge as a Draft for future iteration.

Left some nits, and peer Qs, main one being this.

| flags | key | value |
| --- | --- |-----------------------------|
| Index Key + Value | “$type" | "sector-activated" (string) |
| Index Key + Value | “sector” | <SECTOR_NUMER> (int) |
Copy link
Member

Choose a reason for hiding this comment

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

I think we could use more information here for sector <> data monitoring. i.e: adding a value of has-data boolean; piece informations (cid, associated claim, market actor address)

(in general, the reason why we are adding these information is to ease the integration of data storage toolings, so unless there is a reason for limiting the info expose, i.e: costly gas (I dont think its going to be $$ tho?), size.. otherwise I think we should try to expose necessary info to enable toolings to be able to get necessary information by just subscribing the events without further message monitoring &state diff needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are including more information to this event in #897 and #754 (reply in thread).

This includes piece-cid, piece size and unsealed cid. Wdym by the has-data boolean ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The discussion about the has-data flag is at #754 (comment).


## Design Rationale
Richer and more detailed payloads for the events were initially considered. However, a large event payload
increases the event emission gas costs are borne by users of the built-in actor methods. The payload of
Copy link
Member

Choose a reason for hiding this comment

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

how much gas are we talking about here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't expect this to be significant but the exact amount here has yet to be profiled.

So, we only need to include the `sector number` in the event payload for Miner Actor events to enable
clients to filter Miner Actor events by the Miner/Sector they are interested in.

An SP can activate a single sector by using the `ProveCommitSector` method. As of today, this method
Copy link
Member

Choose a reason for hiding this comment

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

editor: adding a note about #820? depends the sequence of fip acceptances and finalizations, this note might be less relevant

While this increased gas cost is borne by the users of the methods, the benefit of the emitted events
is enjoyed by networking monitoring tools, network accounting tools, block explorer tools and other
external agents that provide some value-added services to the Filecoin network.  Improved
implementation of these tools should indirectly benefit the network users.
Copy link
Member

Choose a reason for hiding this comment

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

remove should or -> will

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@aarshkshah1992
Copy link
Contributor Author

@jennijuju When we initially scoped this work out, we only considered the VerifReg, Market and Miner Actors. Can we do the work for DataCap events in a separate FIP after landing this ? I don't want to cause scope bloat on this workstream as we will need more FIPs anyways to include more built-in Actor events going ahead.

cc @anorth ?

@aarshkshah1992
Copy link
Contributor Author

@jennijuju @anorth Please can one of you merge this PR ? Looks like I don't have the perms.

@jennijuju
Copy link
Member

@jennijuju When we initially scoped this work out, we only considered the VerifReg, Market and Miner Actors. Can we do the work for DataCap events in a separate FIP after landing this ? I don't want to cause scope bloat on this workstream as we will need more FIPs anyways to include more built-in Actor events going ahead.

cc @anorth ?

I don’t mind it to be in a sequential fip. Let’s judge the urgency base off the use case plz. I.s survey fil+ community (tooling builders) to understand if there is anything that’s hard to build today could use datacap events. It’s possible the claim/allocate events are sufficient for now - idk.

@aarshkshah1992
Copy link
Contributor Author

@jennijuju Yeah, getting feedback from users would be great here. I have posted a message on #fil-builders at https://filecoinproject.slack.com/archives/CRK2LKYHW/p1704208937000969.

Comment on lines +45 to +47
_All values in the event payloads defined below are encoded with CBOR and so the codec field for each event_
entry will be set to 0x51.  The codec field is left empty in the payloads defined below only for the
sake of brevity.
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we don't want to introduce "CBOR" (0x51) to the chain, I don't believe we use that anywhere at the moment and it would be a shame to add that to the mix. I think what we want here is "DAG-CBOR" (0x71).

The two problems with just plain CBOR are:

  1. It's not strictly defined in any of our specs, we basically lean on the upstream CBOR specification which brings all sorts of wild additions (simple types, tagged extensions, lots of other things that are not so friendly to a content addressed and distributed world) and non-deterministic encoding forms.
  2. It doesn't materialise links (CIDs) and a generic "CBOR" encoder typically will reject them, even though we managed to reserve the tag in the upstream tag registry.

Also since 0x51 is a rarely used codec in our stack, applications that consume this data will often not even have a codec registered to decode it, thereby adding an additional (tho minor) burden for consuming or validating this data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvagg Thanks for this ! I've added this consideration to the FIP discussion
#754 (comment)

@jennijuju jennijuju merged commit 4b3a449 into master Jan 4, 2024
1 check passed
@jennijuju jennijuju deleted the aarshkshah1992/fip-built-in-actor-events branch January 4, 2024 11:12
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.

5 participants