-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
74ae683
to
9c8a18e
Compare
9c8a18e
to
e733e20
Compare
There was a problem hiding this 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).
} | ||
|
||
/// Pushes an entry with an indexed key and no value. | ||
pub fn label(mut self, name: &str) -> Self { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
actors/verifreg/src/events.rs
Outdated
rt.emit_event( | ||
&EventBuilder::new() | ||
.label("verifier-balance") | ||
.value_indexed("verifier", &verifier)? | ||
.value("balance", new_balance)? | ||
.build(), | ||
) |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 {
....
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
rt: &impl Runtime, | ||
id: AllocationID, | ||
alloc: &Allocation, | ||
) -> Result<(), ActorError> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Closed in favour of #1456. |
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.