FIP-0083: Add built-in Actor events in the Verified Registry, Miner and Market Actors #872
Conversation
aarshkshah1992
requested review from
momack2,
arajasek,
jennijuju,
kaitlin-beegle,
anorth,
raulk and
jsoares
as code owners
aarshkshah1992
changed the title
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. |
There was a problem hiding this comment.
Nit: "that should be" -> "to be" or "that will be". Directly describe the end state after this is correctly implemented.
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 | ||
| ``` |
There was a problem hiding this comment.
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.
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 |
FIPS/fip-xxxx.md
Outdated
| ``` | ||
| 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. | ||
| ``` |
There was a problem hiding this comment.
I think this is unnecessary and can be omitted.
FIPS/fip-xxxx.md
Outdated
| rolled back if the Actor method emitting the event itself aborts and rolls back. | ||
| ``` | ||
|
|
||
| ### FVM should support CBOR encoding |
There was a problem hiding this comment.
| ### FVM should support CBOR encoding | |
| ### FVM support for CBOR encoding |
FIPS/fip-xxxx.md
Outdated
| ``` | ||
| 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. | ||
| ``` |
There was a problem hiding this comment.
The text doesn't refer to specific activation methods, so doesn't need to refer to future ones either.
FIPS/fip-xxxx.md
Outdated
| 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. |
There was a problem hiding this comment.
All of this is the same for verifreg too, so we should end up with one rationale for both. Discussion here.
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 |
FIPS/fip-xxxx.md
Outdated
| 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. |
There was a problem hiding this comment.
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.
FIPS/fip-xxxx.md
Outdated
| 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. |
There was a problem hiding this comment.
Nit: omit the "we will" part. You could say "future FIPs could ...", but that's also obvious so I don't think worth stating.
Applying editorial changes. Co-authored-by: Jorge Soares <547492+jsoares@users.noreply.github.com>
|
Please take FIP number 0083. Please update the headers, change the filename, and add a line to the root README linking to it. |
aarshkshah1992
changed the title
|
@anorth Addressed your review. |
|
|
||
| | flags | key | value | | ||
| | --- | --- |-------------------------| | ||
| | Index Key + Value | “$type” | “allocation” (string) | |
There was a problem hiding this comment.
out of curiosity, is there any reason why this can't just be an index key allocation?
There was a problem hiding this comment.
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) | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
similarly, would be great if we can also expose the pieceCID, thinking of FIL+ bot and such tools can take advantage of it
There was a problem hiding this comment.
Captured in FIP discussion at #754 (comment).
|
with the goal to monitor datacap events, I think we could use this chance to add events to datacap actor as well, starting with |
| | 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) | |
There was a problem hiding this comment.
similar suggestion on adding the amount
There was a problem hiding this comment.
Captured in FIP discussion at #754 (comment).
|
@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) | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
how much gas are we talking about here?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
Co-authored-by: Jiaying Wang <42981373+jennijuju@users.noreply.github.com>
|
@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 ? |
|
@jennijuju @anorth Please can one of you merge this PR ? Looks like I don't have the perms. |
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. |
|
@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. |
| _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. |
There was a problem hiding this comment.
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:
- 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.
- 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.
There was a problem hiding this comment.
@rvagg Thanks for this ! I've added this consideration to the FIP discussion
#754 (comment)
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