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

Implement actor events from the verified registry #1320

Closed
wants to merge 2 commits into from

Conversation

anorth
Copy link
Member

@anorth anorth commented Jun 23, 2023

This PR implements FIP-0049 actor events for the verified registry. We expect to implement such events for most of the built-in in actors in time, so this PR both adds generic event-building utilities and establishes conventions for future actor events. I chose the verified registry as a target because it's relatively simple, such events will be useful immediately, and are probably required for Direct FIL+.

We'll need a FIP before merging this in order to promote sufficient discussion before we commit. I judged that a concrete implementation is the best way to ensure sufficient detail, so here it is. I expect to draft a FIP after maintainers have indicated agreement with this approach.

@anorth anorth force-pushed the anorth/verifreg-events branch 2 times, most recently from 74ae683 to 9c8a18e Compare July 16, 2023 22:17
@anorth anorth changed the title Prototype actor events from the verified registry Implement actor events from the verified registry Jul 16, 2023
@anorth anorth marked this pull request as ready for review July 16, 2023 22:31
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Overall I'm a big fan, just quibbling on conventions.

We'll also have to do some gas profiling to see how much this changes gas costs (@raulk wrote some code for "dual execution" that should work pretty well here).

runtime/src/util/events.rs Outdated Show resolved Hide resolved
}

/// Pushes an entry with an indexed key and no value.
pub fn label(mut self, name: &str) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding this convention... it may be better to just define a common key ("label", "message", "type", etc.) and use that. Looking for the "valueless key" could be a bit annoying.

Copy link
Member Author

@anorth anorth Jul 19, 2023

Choose a reason for hiding this comment

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

I took this pattern from examples in FIP-0049, though it does have examples both ways. My interpretation was that it's the first position that identified the first field as a discriminator of the schema for remaining fields.

"Label" in this utility class is just a word to mean "indexed key with no value". It's not supposed to mean event type. An event could have multiple labels. I'm open to alternative words here. If we change the use of value to field, we could call this just key?

Discussion about whether we should actually use key-with-no-value in the events here belongs where this is called, and we could switch to value_indexed with some new key instead.

But this bikeshedding is what I'm here for!

Comment on lines 19 to 25
rt.emit_event(
&EventBuilder::new()
.label("verifier-balance")
.value_indexed("verifier", &verifier)?
.value("balance", new_balance)?
.build(),
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to be able to write:

Suggested change
rt.emit_event(
&EventBuilder::new()
.label("verifier-balance")
.value_indexed("verifier", &verifier)?
.value("balance", new_balance)?
.build(),
)
rt.build_event()
.label("verifier-balance") // or maybe _require_ a label and take it as a parameter to build_event?
.field_indexed("verifier", &verifier)
.field("balance", new_balance)
.emit()?;

If we also make the event builder an associated type on the runtime (so it can be different in test code versus when run inside the FVM), this could be really efficient and avoid a bunch of allocations. Once we land filecoin-project/ref-fvm#1807, we'll be able to build/emit events with very few copies/allocations.

Copy link
Member

Choose a reason for hiding this comment

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

(note: I'm calling them "fields" because that's the term used by most structured loggers)

Copy link
Member

Choose a reason for hiding this comment

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

NOTE: An associated type wouldn't preclude traits like WithAllocation. You'd just have to implement them as follows:

impl<E: EventBuilder> WithAllocation for E {
    ....
}

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 am not enthusiastic at all about adding anything to the runtime. The code in runtime that's not just directly calling through to syscalls is just library code and shouldn't have anything to do with the syscall interface, or have a privileged name. Everything inside it is basically untestable here and I think it should migrate towards being the thinnest possible layer.

We can of course rework patterns for emitting events if we get a new syscall for it.

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 change the naming to use "field".

actors/verifreg/src/events.rs Outdated Show resolved Hide resolved
rt: &impl Runtime,
id: AllocationID,
alloc: &Allocation,
) -> Result<(), ActorError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Im slightly concerned about adding all this state to receipts as opposed to just the ids. These days snapshots have about 1 G combined of churn data coming from allocations + claims over 2000 epochs while at the same time new message data in 2000 epochs come in closer to 300 - 350 MB per 2000 epochs. Alloc + claim churn measured in the linked table takes into account datastructure overhead too but I suspect a lot of it is the pure claim/alloc data given that these data values are big compared to HAMT internals.

~1% of snapshot state added to receipts is not a terrible price to pay if there's a good reason to have this data however it seems that querying chain state with an id would be acceptable for notification consumers. And this way we can stay away from unnecessary duplication in filecoin sync state.

If we really want the data inlined then it might be worth looking into how much data is stored for, say, 1 finality of syncing receipts and how much we predict will be added from this change at current alloc/claim rates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, great points. I've opened a FIP discussion to discuss the tradeoff in general: filecoin-project/FIPs#754

@aarshkshah1992
Copy link
Contributor

Closed in favour of #1456.

This pull request was closed.
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.

4 participants