Miner Actor events for Sector lifecycle #1462

aarshkshah1992 · 2023-10-25T11:54:56Z

This PR implements the following sector lifecycle events for the Miner Actor:

Sector Precommitted
Sector Activated
Sector Updated
Sector Temrinated

codecov-commenter · 2023-10-25T13:17:17Z

Codecov Report

❗ No coverage uploaded for pull request base (integration/actor-events@7500bab). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

@@                     Coverage Diff                     @@
##             integration/actor-events    #1462   +/-   ##
===========================================================
  Coverage                            ?   91.03%           
===========================================================
  Files                               ?      150           
  Lines                               ?    28145           
  Branches                            ?        0           
===========================================================
  Hits                                ?    25622           
  Misses                              ?     2523           
  Partials                            ?        0

aarshkshah1992 · 2023-10-25T15:19:11Z

actors/miner/src/lib.rs

@@ -4935,7 +4973,17 @@ fn activate_new_sector_infos(
    data_activations: Vec<DataActivationOutput>,
    pledge_inputs: &NetworkPledgeInputs,
    info: &MinerInfo,
+    emit_event: bool,


@anorth We need this as we only want the emit here if this call is from the cron callback to CONFIRM_SECTOR_PROOFS_VALID which in turn is scheduled by the vanilla prove commit method.

For the prove commit aggregate method, we don't want to emit the event here as we also need to check if we have enough funds for gas fees. Doing it this way makes it work nicely with the tests.

I don't think this is the right trade-off. Emitting the events here is a good place to ensure that future code changes will still emit events exactly according to activations, and will simplify code elsewhere. There are more paths into sector activation coming in integration/direct-onboarding.

We don't need to make checks about funds etc before emitting events - if those checks fail the whole thing will abort anyway. We may need to update testing support code to allow us to write the main code in the straightforward way.

This is done. We no longer have this flag.

anorth

Please change the testing patterns to remove the boolean configuration of which expectations to set, and instead reset the mock after a method aborts.

We also need events for data activation via ProveReplicaUpdates. I can't tell from the PR description whether you intended this to be the only PR to miner actor, or would follow-up. But I think it probably makes sense to do it all at once here so we can confirm the same patterns work for both.

anorth · 2023-10-25T18:56:27Z

actors/miner/src/emit.rs

+use fvm_shared::sector::SectorID;
+
+/// Indicates a sector has been pre-committed.
+pub fn sector_precommited(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {


Suggested change

pub fn sector_precommited(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {

pub fn sector_precommitted(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {

anorth · 2023-10-25T18:56:44Z

actors/miner/src/emit.rs

+pub fn sector_precommited(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {
+    rt.emit_event(
+        &EventBuilder::new()
+            .typ("sector-precommited")


Suggested change

.typ("sector-precommited")

.typ("sector-precommitted")

anorth · 2023-10-25T19:00:07Z

actors/miner/src/emit.rs

+use fvm_shared::sector::SectorID;
+
+/// Indicates a sector has been pre-committed.
+pub fn sector_precommited(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {


I know the SectorID type has exactly what you want, but please pass two separate parameters here for the miner actor ID and sector number separately. The SectorID type only exists as part of SealVerifyInfo; it's defined outside of our repo and is unused except when forced by the FVM SDK API. I was genuinely surprised to see it as I'd forgotten it exists.

This is done.

anorth · 2023-10-25T19:01:38Z

actors/miner/src/emit.rs

+}
+
+/// Indicates a sector has been proven.
+pub fn sector_proven(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {


Please rename to sector-activated. Today these always happen at the same time, but in the future we may separate them. If we do, it's likely that most users of the event really care about the activation, rather than an un-activated proof. We'll reserve sector-proven for that case.

anorth · 2023-10-25T19:04:26Z

actors/miner/src/lib.rs

@@ -852,12 +854,16 @@ impl Actor {
            activate_deals(rt, &data_activations, compute_commd)?;
        let successful_activations = batch_return.successes(&precommits_to_confirm);

+        let proven_sector_nums =


activated_sector_nums

anorth · 2023-10-25T19:14:41Z

actors/miner/src/lib.rs

+            for sector_num in precommited_sectors.into_iter() {
+                let sector_id = SectorID{
+                    miner: miner_actor_id,
+                    number: sector_num,
+                };
+                emit::sector_precommited(rt, sector_id)?;
+            }


Move this up a few lines so it's directly after the state updates.

anorth · 2023-10-25T19:19:01Z

actors/miner/src/lib.rs

@@ -4935,7 +4973,17 @@ fn activate_new_sector_infos(
    data_activations: Vec<DataActivationOutput>,
    pledge_inputs: &NetworkPledgeInputs,
    info: &MinerInfo,
+    emit_event: bool,


I don't think this is the right trade-off. Emitting the events here is a good place to ensure that future code changes will still emit events exactly according to activations, and will simplify code elsewhere. There are more paths into sector activation coming in integration/direct-onboarding.

We don't need to make checks about funds etc before emitting events - if those checks fail the whole thing will abort anyway. We may need to update testing support code to allow us to write the main code in the straightforward way.

anorth · 2023-10-25T19:19:08Z

actors/miner/src/lib.rs

 ) -> Result<(), ActorError> {
+    let miner_actor_id: u64 = if let Payload::ID(i) = rt.message().receiver().payload() {


Just unwrap

anorth · 2023-10-25T19:23:15Z

actors/miner/tests/compact_sector_numbers_tests.rs

@@ -49,7 +49,7 @@ mod compact_sector_numbers_test {
            );
            expect_abort(
                ExitCode::USR_ILLEGAL_ARGUMENT,
-                h.pre_commit_sector(&rt, precommit, util::PreCommitConfig::default(), false),
+                h.pre_commit_sector(&rt, precommit, util::PreCommitConfig::default(), false, false),


We shouldn't have to pass this flag. This pattern doesn't scale to all the possible expectations and abort points that a method might hit.

The mock should always set expectations for the events. If the method aborts before emitting them, then at line 54 you should rt.reset() to void any remaining expectations. Most places will probably already do this, but this one forgot.

This has been fixed in all tests. Done.

anorth · 2023-10-25T19:24:54Z

integration_tests/src/tests/commit_post_test.rs

@@ -720,7 +720,7 @@ pub fn aggregate_one_precommit_expires_test(v: &dyn VM) {
    apply_ok(
        v,
        &worker,
-        &robust_addr,
+        &miner_addr,


Why was this change necessary? It shouldn't be

@anorth This is because the Test Runtime always expects the receiver address to be an ID Address.

builtin-actors/test_vm/src/messaging.rs

Line 643 in 7500bab

.push(EmittedEvent { emitter: self.msg.to.id().unwrap(), event: event.clone() });

Can we change the emitter field here to be just an Address instead ? We can then worry about what to do with it once we get to the integration tests.

The event emitter must always be an ID. The runtime should always give the actor an ID address as receiver(). The problem is somewhere in our TestVM which should resolve an address to an ID address before making it available as the receiver (but not too soon, in case the actor doesn't yet exist and needs to be created on the fly to receive the message). It's important that we can support calls to non-ID addresses like this but then have everything still work.

aarshkshah1992 · 2023-10-26T12:53:09Z

@anorth

I have addressed your review and have added events for the Sector Updated and Sector Terminated transitions. Some notes/questions:

The failing integration tests are because of Miner Actor events for Sector lifecycle #1462 (comment). Should we change the test runtime to accept non-ID addresses when events are emitted or should we change the tests to only use ID addresses ? I am not sure of the tradeoffs here.
There are no unit tests for the ProveReplicaUpdate* method and so no event emission tests for that. I think we will have to cover this when we get to integration tests as we do have integration tests for this work. Any suggestions/concerns ?

anorth

I want us to figure out the address resolution in tests, but otherwise this looks good.

anorth · 2023-10-26T20:03:07Z

actors/miner/src/lib.rs

@@ -878,6 +880,7 @@ impl Actor {
        }
        burn_funds(rt, aggregate_fee)?;
        state.check_balance_invariants(&rt.current_balance()).map_err(balance_invariants_broken)?;
+


Nit: unnecessary blank line. Some others similar below.

This is done.

anorth · 2023-10-26T20:03:19Z

actors/miner/src/lib.rs

@@ -1484,6 +1495,8 @@ impl Actor {
        rt: &impl Runtime,
        sectors: Vec<SectorPreCommitInfoInner>,
    ) -> Result<(), ActorError> {
+        let miner_actor_id: u64 = rt.message().receiver().id().unwrap();
+


Nit: unnecessary blank

anorth · 2023-10-26T20:08:08Z

actors/miner/tests/prove_commit.rs

    let st = h.get_state(&rt);
    assert!(st.get_sector(&rt.store, sector_no_a).unwrap().is_none());
    assert!(st.get_sector(&rt.store, sector_no_b).unwrap().is_some());
    h.check_state(&rt);
+    rt.reset();


This call didn't abort, so this should rt.verify() instead. Or you can skip it because it's automatic when dropped, which happens immediately next. If you added this because expectations are failing, something is wrong.

Fixed, we don't need this. It's a remanent of something I was debugging earlier.

anorth · 2023-10-26T20:11:53Z

actors/miner/tests/util.rs

+                    .typ("sector-precommitted")
+                    .field_indexed("miner", &RECEIVER_ID)
+                    .field_indexed("sector", &si.sector_number)
+                    .build()?,


There's enough repeated code here that it's probably just worth adding a method to factor it out, like expect_event(rt, "sector-precommitted", &RECEIVER_ID, &si.sector_number). This can be broadly useful because all the miner events have the same schema.

Refactored this into it's own function.

anorth · 2023-10-26T20:17:49Z

integration_tests/src/tests/commit_post_test.rs

@@ -720,7 +720,7 @@ pub fn aggregate_one_precommit_expires_test(v: &dyn VM) {
    apply_ok(
        v,
        &worker,
-        &robust_addr,
+        &miner_addr,


The event emitter must always be an ID. The runtime should always give the actor an ID address as receiver(). The problem is somewhere in our TestVM which should resolve an address to an ID address before making it available as the receiver (but not too soon, in case the actor doesn't yet exist and needs to be created on the fly to receive the message). It's important that we can support calls to non-ID addresses like this but then have everything still work.

aarshkshah1992 · 2023-10-27T07:30:29Z

@anorth I've fixed the problem of the receiver address not being an ID address in the TestVM. Things work well now without any changes to the integration tests and so have rolled those changes back.

aarshkshah1992 added 3 commits October 25, 2023 15:53

Miner Actor pre-commit event

e0bf67e

use Miner ID addrss in itests

578feea

de dup miner actor id

cfe4325

aarshkshah1992 mentioned this pull request Oct 25, 2023

Actor events for the Verified Registry, Miner and Market Actors [Meta Issue] #1464

Closed

2 tasks

events for sector proven

c3b4ccb

aarshkshah1992 changed the title ~~[WIP] Emit Sector Pre-commited and Sector Proven events for the Miner Actor~~ Emit Sector Pre-commited and Sector Proven events for the Miner Actor Oct 25, 2023

aarshkshah1992 requested review from anorth and Stebalien October 25, 2023 15:15

aarshkshah1992 commented Oct 25, 2023

View reviewed changes

anorth requested changes Oct 25, 2023

View reviewed changes

aarshkshah1992 added 8 commits October 26, 2023 10:41

changes as per review

41457ab

tests work before refactoring tests

cbcae6e

removed flag from precommit batch harness

a7285e8

precommit flags removed

2213996

no flag for emit events in tests

8840cd2

fix event comment

2e5a4be

sector updated event

2c9464f

sector terminated event

9ec5cec

aarshkshah1992 changed the title ~~Emit Sector Pre-commited and Sector Proven events for the Miner Actor~~ Miner Actor events for Sector lifecycle Oct 26, 2023

anorth reviewed Oct 26, 2023

View reviewed changes

aarshkshah1992 added 2 commits October 27, 2023 10:29

addressed review

439a32d

fix addressing in test vm

46e42f8

anorth approved these changes Oct 29, 2023

View reviewed changes

aarshkshah1992 merged commit af222d7 into integration/actor-events Oct 30, 2023
13 checks passed

aarshkshah1992 deleted the feat/miner-precommit-event branch October 30, 2023 07:22

	pub fn sector_precommited(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {
	pub fn sector_precommitted(rt: &impl Runtime, id: SectorID) -> Result<(), ActorError> {

		) -> Result<(), ActorError> {
		let miner_actor_id: u64 = if let Payload::ID(i) = rt.message().receiver().payload() {

Miner Actor events for Sector lifecycle #1462

Miner Actor events for Sector lifecycle #1462

Conversation

aarshkshah1992 commented Oct 25, 2023 • edited Loading

codecov-commenter commented Oct 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 26, 2023 • edited Loading

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Oct 27, 2023

aarshkshah1992 commented Oct 25, 2023 •

edited

Loading

codecov-commenter commented Oct 25, 2023 •

edited

Loading

aarshkshah1992 commented Oct 26, 2023 •

edited

Loading

aarshkshah1992 Oct 27, 2023 •

edited

Loading