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

Implements SectorContentChanged method in built-in market actor #1353

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

anorth
Copy link
Member

@anorth anorth commented Aug 6, 2023

  • Write tests

@@ -47,6 +52,67 @@ pub mod miner {
pub struct IsControllingAddressParam {
pub address: Address,
}

Copy link
Member Author

Choose a reason for hiding this comment

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

These types will move to the miner actor when we implement sending of the notification, hence initial definition in ext here.

@@ -534,6 +538,9 @@ impl Actor {
/// extra info about verified deals.
/// Sectors' deals are activated in parameter-defined order.
/// Each sector's deals are activated or fail as a group, but independently of other sectors.
/// Note that confirming all deals fit within a sector is the caller's responsibility
/// (and is implied by confirming the sector's data commitment is derived from the deal peices).
// see https://github.com/filecoin-project/builtin-actors/issues/1308
fn batch_activate_deals(
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've refactored this to extract as much shared code as practical (in preactivate_deal). The methods follow very similar patterns, but batch_activate_deals is defined to succeed/fail on a per-sector basis, while sector_content_changed is just per-deal.

@anorth anorth requested a review from ZenGround0 August 7, 2023 02:30
actors/market/src/ext.rs Outdated Show resolved Hide resolved
actors/market/src/ext.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
@anorth anorth marked this pull request as ready for review August 8, 2023 03:03
@anorth anorth requested a review from ZenGround0 August 8, 2023 03:03
@anorth anorth force-pushed the anorth/ddo-market-notification branch from c6f8e2f to 71472eb Compare August 8, 2023 03:04
@anorth anorth force-pushed the integration/direct-onboarding branch from 65ec576 to af8f4cd Compare August 8, 2023 21:02
@anorth anorth force-pushed the anorth/ddo-market-notification branch from 71472eb to 9d25bb7 Compare August 8, 2023 21:22
@anorth anorth force-pushed the anorth/ddo-market-notification branch from 9d25bb7 to 839316f Compare August 9, 2023 02:08
@anorth anorth force-pushed the anorth/ddo-market-notification branch from 839316f to 887b7a9 Compare August 9, 2023 02:10
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Still looking at tests

actors/market/src/ext.rs Outdated Show resolved Hide resolved
actors/market/src/ext.rs Outdated Show resolved Hide resolved
actors/market/src/ext.rs Outdated Show resolved Hide resolved
actors/market/tests/sector_content_changed.rs Show resolved Hide resolved
actors/market/src/lib.rs Outdated Show resolved Hide resolved
actors/market/src/lib.rs Outdated Show resolved Hide resolved
let mut nonverified_deal_space: BigInt = BigInt::zero();
for deal_id in &p.deal_ids {
// Check each deal is present only once, within and across sectors.
if activated_deals.contains(deal_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking: are we filtering out for duplicates inside the sector deal_ids somewhere else? Since we only add to activated_deals per sector this check alone isn't doing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The activated_deals set is scoped outside the loop over sectors, so this is deduping over everything. It's tested (please verify).

Copy link
Contributor

Choose a reason for hiding this comment

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

so this is deduping over everything

I agree for other sectors, I'm concerned about within the same sector. I think we are no longer handling the case where a single sector has duplicate deal ids since we used to do this in get_proposals which is no longer called here. I might be missing something, will keep looking for tests / write something quick to check this out further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a fork of this branch with a test fails_to_activate_single_sector_duplicate_deals with different behavior on this branch compared to master: https://github.com/filecoin-project/builtin-actors/tree/test/duplicate-deal-ids-same-sector

We should include this test in this PR and an analogous one for SectorContentAdded with the expected behavior there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, thank you. I initially had the insertion into activated_deals earlier, inside the one and only loop over deal IDs. Since moving it out for the sector-atomic changes, the insertion is too late.

I've now fixed that and added the test you proposed, thanks. The tests for SectorContentChanged already covered this, but I've rearranged them to more closely follow the BatchActivateDeals arrangement.

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Looking good for the most part, I'll wait for another round of review response before a final approval review.

actors/market/src/lib.rs Show resolved Hide resolved
@anorth anorth force-pushed the anorth/ddo-market-notification branch from cdffe33 to 9361b02 Compare August 10, 2023 03:17
@anorth anorth requested a review from ZenGround0 August 10, 2023 03:17
@anorth anorth force-pushed the anorth/ddo-market-notification branch 2 times, most recently from faff771 to 0ea5021 Compare August 10, 2023 08:20
@anorth anorth force-pushed the anorth/ddo-market-notification branch from 0ea5021 to 9aa6ab7 Compare August 10, 2023 08:24
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Other than addressing the duplicate deal_ids within the same sector case and adding tests for that this looks good to go.

@anorth anorth force-pushed the anorth/ddo-market-notification branch from 4a27b20 to 784adde Compare August 15, 2023 02:31
@anorth anorth enabled auto-merge (squash) August 15, 2023 02:31
@anorth anorth merged commit 28104fa into integration/direct-onboarding Aug 15, 2023
@anorth anorth deleted the anorth/ddo-market-notification branch August 15, 2023 03:35
aarshkshah1992 added a commit that referenced this pull request Jan 31, 2024
commit f0a0631
Author: Aarsh Shah <[email protected]>
Date:   Wed Jan 31 11:57:31 2024 +0400

    emit event during settling payments

commit a35b1cc
Merge: 1722310 859c731
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 30 09:47:20 2024 +0400

    Merge remote-tracking branch 'origin/master' into feat/ddo-actor-events

commit 1722310
Author: Aarsh Shah <[email protected]>
Date:   Mon Jan 29 14:04:27 2024 +0400

    Apply suggestions from code review

    Co-authored-by: Rod Vagg <[email protected]>

commit b149550
Merge: 803f420 d0b8d9c
Author: Aarsh Shah <[email protected]>
Date:   Mon Jan 29 14:00:12 2024 +0400

    Merge remote-tracking branch 'origin/integration/manual-settlement' into feat/ddo-actor-events

commit d0b8d9c
Author: Alex Su <[email protected]>
Date:   Mon Oct 16 09:09:28 2023 +1100

    Use ExitCode::USR_NOT_FOUND for missing state in GetDealActivation (#1441)

    * hide deals that are asynchronously termianted in get_deal_activation

    * add test for EX_DEAL_EXPIRED behaviour

commit c6227a2
Author: Alex Su <[email protected]>
Date:   Mon Oct 9 13:00:46 2023 +1100

    Additional tests for synchronous termination and SettleDealPayments (#1423)

    * rename some tests and comments to no longer reference slashed_epoch which will never be observable

    * port cron tick tests for deal termination

    * move deal termination tests

    * modify generate and publish deal to return proposal inline

    * fix bug when settling payments between between publish and activation

    * deal termination edge cases

    * pr review

commit 21baa60
Author: Alex Su <[email protected]>
Date:   Fri Sep 8 12:29:14 2023 +1000

    Explicit deal settlement in built-in market (#1377)

commit 803f420
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 23 18:21:17 2024 +0400

    changes

commit 78ec9f6
Merge: c90fafe e671d69
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 23 18:21:02 2024 +0400

    Merge remote-tracking branch 'origin/integration/direct-onboarding' into feat/ddo-actor-events

commit e671d69
Author: Alex North <[email protected]>
Date:   Tue Jan 23 14:07:21 2024 +1300

    More review response for #1486. (#1508)

commit db65674
Author: Steven Allen <[email protected]>
Date:   Wed Jan 17 10:56:30 2024 -0800

    Cleanup batch return merge logic (#1503)

    This just avoids some awkward unwraps.

commit 84b0c00
Author: Alex North <[email protected]>
Date:   Wed Jan 17 09:05:16 2024 +1300

    Response to review on #1486 (#1499)

    Response to review on #1486

commit 049b9c7
Author: Steven Allen <[email protected]>
Date:   Mon Jan 15 12:48:17 2024 -0800

    chore: remove unnecessary clone of piece payload (#1494)

commit 9334a87
Author: Alex North <[email protected]>
Date:   Tue Jan 9 07:04:12 2024 +1300

    Add aggregate proof type parameter to ProveCommitSectors3 (#1489)

commit 4126ccb
Author: Alex North <[email protected]>
Date:   Fri Dec 22 07:17:28 2023 +1100

    Add missing vm_test annotations. (#1488)

commit 3ad21ac
Author: Alex North <[email protected]>
Date:   Tue Dec 19 11:49:23 2023 +1100

    Rename PCS2/PRU2 to PCS3/PRU3 (#1485)

commit 53cb47e
Author: Alex North <[email protected]>
Date:   Tue Dec 19 11:08:21 2023 +1100

    Transparent serialization for ProveReplicaUpdates2Return (#1484)

commit 5cdfc2f
Author: Alex North <[email protected]>
Date:   Tue Oct 31 13:49:37 2023 +1100

    Integration test for prove_replica_updates2 (#1453)

commit 0f8c593
Author: Alex North <[email protected]>
Date:   Tue Oct 31 12:52:33 2023 +1100

    Integration test for prove_commit_sectors2 (#1450)

commit 5cecdf6
Author: Alex North <[email protected]>
Date:   Tue Oct 31 11:52:06 2023 +1100

    Add GetDealSector exported API method to market actor (#1443)

commit 21c01a9
Author: Alex North <[email protected]>
Date:   Tue Oct 17 05:20:35 2023 +1100

    More tests for direct data onboarding (#1440)

    Tests that CommD computed from pieces must match that declared at precommit. Tests that updating an empty replica to be empty is permitted (later, when we allow updates to non-empty replicas, it won't be a no-op).

commit 47e1cf1
Author: Alex North <[email protected]>
Date:   Mon Oct 16 07:18:28 2023 +1100

    Tests for prove_commit_sectors2 (#1437)

commit a108360
Author: Alex North <[email protected]>
Date:   Thu Sep 28 03:11:22 2023 +1000

    Tests for ProveCommitSectors2 for cases that abort entirely (#1414)

commit ff66f79
Author: Alex North <[email protected]>
Date:   Sat Sep 23 05:05:35 2023 +1000

    Check state invariants in direct onboarding tests (#1416)

commit 3354d8a
Author: Alex North <[email protected]>
Date:   Sat Sep 16 04:23:58 2023 +1000

    Tests and fixes for ProveReplicaUpdates2 (#1411)

    * Tests and fixes for ProveReplicaUpdates2

    * Helper function

commit 869af32
Author: Alex North <[email protected]>
Date:   Mon Sep 11 10:17:48 2023 +1000

    Move prove_commit2 tests to separate file from pru tests (#1410)

commit 24b5cd6
Author: Alex North <[email protected]>
Date:   Fri Sep 8 10:19:52 2023 +1000

    PRU2 tests that abort the whole method (#1407)

commit ff472f2
Author: ZenGround0 <[email protected]>
Date:   Thu Sep 7 19:04:58 2023 -0400

    Prove commit 2 simple tests (#1408)

commit a861f02
Author: Shrenuj Bansal <[email protected]>
Date:   Wed Sep 6 21:47:36 2023 -0400

    Return error if success count for proof validation or data activations is 0 (#1406)

commit 0457d86
Author: Alex North <[email protected]>
Date:   Wed Sep 6 08:26:36 2023 +1000

    Remove allocation id from market deal state. Clean up and improve state checks. (#1403)

commit e0083f4
Author: ZenGround0 <[email protected]>
Date:   Mon Sep 4 20:48:54 2023 -0400

    Deprecate Deal IDs (#1402)

    Co-authored-by: zenground0 <[email protected]>

commit d78586f
Author: ZenGround0 <[email protected]>
Date:   Mon Sep 4 14:44:41 2023 -0400

    Deprecate unused Prove Replica update 2 (#1401)

    * Simplify input to activate_sectors_deals
    * Remove old PRU2 and parameter types
    * Reuse PRU2 name for DDO PRU fka PRU 3

    ---------

    Co-authored-by: zenground0 <[email protected]>

commit c1aa98f
Author: Alex North <[email protected]>
Date:   Mon Sep 4 11:09:24 2023 +1000

    Basic unit tests for ProveReplicaUpdate3 (#1394)

commit 5de9cae
Author: Alex North <[email protected]>
Date:   Fri Sep 1 08:24:52 2023 +1000

    Compute batch returns for direct onboarding methods (#1393)

commit 06f282a
Author: Alex North <[email protected]>
Date:   Thu Aug 31 05:10:48 2023 +1000

    Send SectorContentChanged from new onboarding methods (#1386)

    Send SectorContentChanged from new onboarding methods. Simplify return values to make notifications fire and forget.

commit 6a2a089
Author: Alex North <[email protected]>
Date:   Thu Aug 31 03:43:37 2023 +1000

    Miner notifies market of termination only of sectors with non-zero data (#1387)

    Co-authored-by: ZenGround0 <[email protected]>

commit ba9b579
Author: Alex North <[email protected]>
Date:   Wed Aug 30 20:07:45 2023 +1000

    Implement new ProveReplicaUpdates3 for direct data onboarding. (#1385)

commit 9c681c7
Author: Alex North <[email protected]>
Date:   Tue Aug 29 08:38:39 2023 +1000

    Implements new ProveCommit batch+aggregate for direct onboarding (#1380)

    Co-authored-by: zenground0 <[email protected]>

commit c5fd304
Author: Alex North <[email protected]>
Date:   Tue Aug 15 13:35:08 2023 +1000

    Implements SectorContentChanged method in built-in market actor (#1353)

commit e55498d
Author: Alex North <[email protected]>
Date:   Wed Aug 9 08:40:52 2023 +1000

    Remove deprecated precommit methods (#1357)

    - PreCommitSector and PreCommitBatch deprecated
    - PreCommitBatchV2 now requires specifying (compact) CommD
    - Move all testing to use PreCommitBatchV2

    Co-authored-by: ZenGround0 <[email protected]>
    Co-authored-by: zenground0 <[email protected]>

commit 181305e
Author: Alex North <[email protected]>
Date:   Fri Aug 4 04:33:49 2023 +1000

    Implement sector->deal mapping in built-in market actor (#1347)

commit c90fafe
Author: Aarsh Shah <[email protected]>
Date:   Thu Jan 18 20:06:10 2024 +0400

    changes as per steb's review

commit 7e0abca
Author: Alex North <[email protected]>
Date:   Fri Jan 12 11:24:13 2024 +1300

    Simply deal activation return and intermediate data flow

commit 6e3b5ca
Author: Aarsh Shah <[email protected]>
Date:   Thu Jan 11 21:41:05 2024 +0400

    finish review

commit 4d20e04
Author: Aarsh Shah <[email protected]>
Date:   Thu Jan 11 18:28:23 2024 +0400

    tests pass

commit 733a98f
Author: Aarsh Shah <[email protected]>
Date:   Wed Jan 10 17:51:41 2024 +0400

    fix itests

commit aef0fdd
Author: Aarsh Shah <[email protected]>
Date:   Wed Jan 10 17:21:41 2024 +0400

    batch activate deals should return piece info

commit 4a1ecdf
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 9 19:59:35 2024 +0400

    rustfmt

commit f92222d
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 9 19:53:59 2024 +0400

    itests green

commit 6f8be0d
Merge: 693338f aa0a168
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 9 17:28:53 2024 +0400

    Merge remote-tracking branch 'origin/integration/direct-onboarding' into feat/ddo-actor-events

commit 693338f
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 9 17:25:36 2024 +0400

    itests almost there

commit aa0a168
Author: Alex North <[email protected]>
Date:   Tue Jan 9 07:04:12 2024 +1300

    Add aggregate proof type parameter to ProveCommitSectors3 (#1489)

commit 9e19d32
Author: Aarsh Shah <[email protected]>
Date:   Mon Jan 8 19:32:09 2024 +0400

    itests partially complete

commit 0485317
Author: Aarsh Shah <[email protected]>
Date:   Mon Jan 8 17:09:44 2024 +0400

    itests work 1

commit 2ce92e6
Author: Aarsh Shah <[email protected]>
Date:   Mon Jan 8 14:29:03 2024 +0400

    miner actor events

commit 9a22c67
Author: Aarsh Shah <[email protected]>
Date:   Wed Jan 3 12:48:36 2024 +0400

    miner actor events

commit 6e5d70c
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 2 13:15:42 2024 +0400

    market actor events

commit f522274
Author: Aarsh Shah <[email protected]>
Date:   Tue Jan 2 10:02:11 2024 +0400

    verifreg events and itest harness

commit 1d85ec5
Author: Alex North <[email protected]>
Date:   Fri Dec 22 07:17:28 2023 +1100

    Add missing vm_test annotations. (#1488)

commit 0d103fe
Author: Alex North <[email protected]>
Date:   Tue Dec 19 11:49:23 2023 +1100

    Rename PCS2/PRU2 to PCS3/PRU3 (#1485)

commit e01a799
Author: Alex North <[email protected]>
Date:   Tue Dec 19 11:08:21 2023 +1100

    Transparent serialization for ProveReplicaUpdates2Return (#1484)

commit 13931e0
Author: Alex North <[email protected]>
Date:   Tue Oct 31 13:49:37 2023 +1100

    Integration test for prove_replica_updates2 (#1453)

commit 39b488b
Author: Alex North <[email protected]>
Date:   Tue Oct 31 12:52:33 2023 +1100

    Integration test for prove_commit_sectors2 (#1450)

commit 04f3843
Author: Alex North <[email protected]>
Date:   Tue Oct 31 11:52:06 2023 +1100

    Add GetDealSector exported API method to market actor (#1443)

commit e48433c
Author: Alex North <[email protected]>
Date:   Tue Oct 17 05:20:35 2023 +1100

    More tests for direct data onboarding (#1440)

    Tests that CommD computed from pieces must match that declared at precommit. Tests that updating an empty replica to be empty is permitted (later, when we allow updates to non-empty replicas, it won't be a no-op).

commit 7dfdfc2
Author: Alex North <[email protected]>
Date:   Mon Oct 16 07:18:28 2023 +1100

    Tests for prove_commit_sectors2 (#1437)

commit df2711e
Author: Alex North <[email protected]>
Date:   Thu Sep 28 03:11:22 2023 +1000

    Tests for ProveCommitSectors2 for cases that abort entirely (#1414)

commit 3f3a6c4
Author: Alex North <[email protected]>
Date:   Sat Sep 23 05:05:35 2023 +1000

    Check state invariants in direct onboarding tests (#1416)

commit c9fdbaa
Author: Alex North <[email protected]>
Date:   Sat Sep 16 04:23:58 2023 +1000

    Tests and fixes for ProveReplicaUpdates2 (#1411)

    * Tests and fixes for ProveReplicaUpdates2

    * Helper function

commit 6ef0eb2
Author: Alex North <[email protected]>
Date:   Mon Sep 11 10:17:48 2023 +1000

    Move prove_commit2 tests to separate file from pru tests (#1410)

commit 5f28bc2
Author: Alex North <[email protected]>
Date:   Fri Sep 8 10:19:52 2023 +1000

    PRU2 tests that abort the whole method (#1407)

commit 415bc7a
Author: ZenGround0 <[email protected]>
Date:   Thu Sep 7 19:04:58 2023 -0400

    Prove commit 2 simple tests (#1408)

commit d3e7256
Author: Shrenuj Bansal <[email protected]>
Date:   Wed Sep 6 21:47:36 2023 -0400

    Return error if success count for proof validation or data activations is 0 (#1406)

commit aaca941
Author: Alex North <[email protected]>
Date:   Wed Sep 6 08:26:36 2023 +1000

    Remove allocation id from market deal state. Clean up and improve state checks. (#1403)

commit 2724624
Author: ZenGround0 <[email protected]>
Date:   Mon Sep 4 20:48:54 2023 -0400

    Deprecate Deal IDs (#1402)

    Co-authored-by: zenground0 <[email protected]>

commit fbbdaf8
Author: ZenGround0 <[email protected]>
Date:   Mon Sep 4 14:44:41 2023 -0400

    Deprecate unused Prove Replica update 2 (#1401)

    * Simplify input to activate_sectors_deals
    * Remove old PRU2 and parameter types
    * Reuse PRU2 name for DDO PRU fka PRU 3

    ---------

    Co-authored-by: zenground0 <[email protected]>

commit 4b9042b
Author: Alex North <[email protected]>
Date:   Mon Sep 4 11:09:24 2023 +1000

    Basic unit tests for ProveReplicaUpdate3 (#1394)

commit 3a293df
Author: Alex North <[email protected]>
Date:   Fri Sep 1 08:24:52 2023 +1000

    Compute batch returns for direct onboarding methods (#1393)

commit 351a15a
Author: Alex North <[email protected]>
Date:   Thu Aug 31 05:10:48 2023 +1000

    Send SectorContentChanged from new onboarding methods (#1386)

    Send SectorContentChanged from new onboarding methods. Simplify return values to make notifications fire and forget.

commit 35efa32
Author: Alex North <[email protected]>
Date:   Thu Aug 31 03:43:37 2023 +1000

    Miner notifies market of termination only of sectors with non-zero data (#1387)

    Co-authored-by: ZenGround0 <[email protected]>

commit 8dd2c12
Author: Alex North <[email protected]>
Date:   Wed Aug 30 20:07:45 2023 +1000

    Implement new ProveReplicaUpdates3 for direct data onboarding. (#1385)

commit 079ebbc
Author: Alex North <[email protected]>
Date:   Tue Aug 29 08:38:39 2023 +1000

    Implements new ProveCommit batch+aggregate for direct onboarding (#1380)

    Co-authored-by: zenground0 <[email protected]>

commit 047c3ec
Author: Alex North <[email protected]>
Date:   Tue Aug 15 13:35:08 2023 +1000

    Implements SectorContentChanged method in built-in market actor (#1353)

commit 3e14b94
Author: Alex North <[email protected]>
Date:   Wed Aug 9 08:40:52 2023 +1000

    Remove deprecated precommit methods (#1357)

    - PreCommitSector and PreCommitBatch deprecated
    - PreCommitBatchV2 now requires specifying (compact) CommD
    - Move all testing to use PreCommitBatchV2

    Co-authored-by: ZenGround0 <[email protected]>
    Co-authored-by: zenground0 <[email protected]>

commit dd3b68e
Author: Alex North <[email protected]>
Date:   Fri Aug 4 04:33:49 2023 +1000

    Implement sector->deal mapping in built-in market actor (#1347)
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.

2 participants