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.

Sign up for GitHub

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

Jump to bottom

Response to review on #1486 #1499

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Response to review on #1486 #1499

merged 2 commits into from
Jan 16, 2024

Conversation

anorth
Copy link
Member

@anorth anorth commented Jan 15, 2024

Implements changes to most of the comments in #1486.

@anorth anorth mentioned this pull request Jan 15, 2024
Merged
@rvagg rvagg mentioned this pull request Jan 16, 2024
Merged
rvagg
rvagg reviewed Jan 16, 2024
View reviewed changes
actors/market/src/state.rs
@@ -85,6 +88,7 @@ pub struct State {

/// IDs of deals associated with a single sector.
#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
#[serde(transparent)]
Copy link
Member

Choose a reason for hiding this comment

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

This would need a FIP change wouldn't it? Currently it's like this in FIP-0076:

struct SectorDeals {
    Deals: []DealID
}

But this change would effectively make it something like: SectorDeals []DealID

Copy link
Member

Choose a reason for hiding this comment

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

filecoin-project/go-state-types#236 makes this assumption too, we go from [[deal1,deal2,deal3,...]] to [deal1,deal2,deal3,...], which seems reasonable (although only saving 1 or 2 bytes), but the format seems to be assumed by a plain reading of 0076 with tuple structs.

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, I'll fix them too.

Copy link
Member Author

Choose a reason for hiding this comment

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

See filecoin-project/FIPs#917 and filecoin-project/go-state-types#238

Copy link
Member

Choose a reason for hiding this comment

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

NOTE: we can go either way. The only reason I bring this up is that, e.g., the FIP states that SectorContentChangedParams should be transparent (so I figured we might as well be consistent).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the reason it wasn't already is that an earlier version had another field in here.

Stebalien
Stebalien approved these changes Jan 16, 2024
View reviewed changes
actors/market/src/state.rs
@@ -85,6 +88,7 @@ pub struct State {

/// IDs of deals associated with a single sector.
#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
#[serde(transparent)]
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: we can go either way. The only reason I bring this up is that, e.g., the FIP states that SectorContentChangedParams should be transparent (so I figured we might as well be consistent).

actors/market/src/state.rs
@@ -93,7 +97,7 @@ pub type PendingDealAllocationsMap<BS> = Map2<BS, DealID, AllocationID>;
pub const PENDING_ALLOCATIONS_CONFIG: Config =
Config { bit_width: HAMT_BIT_WIDTH, ..DEFAULT_HAMT_CONFIG };

pub type ProviderSectorsMap<BS> = Map2<BS, Address, Cid>;
pub type ProviderSectorsMap<BS> = Map2<BS, ActorID, Cid>;
Copy link
Member

Choose a reason for hiding this comment

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

Also requires a FIP update.

@anorth anorth merged commit 5c9a4e4 into integration/direct-onboarding Jan 16, 2024
14 checks passed
@anorth anorth deleted the anorth/ddo-review branch January 16, 2024 20:05
anorth added a commit that referenced this pull request Jan 22, 2024
@anorth
Response to review on #1486 (#1499)
84b0c00 
Response to review on #1486
anorth added a commit that referenced this pull request Jan 23, 2024
@anorth
Response to review on #1486 (#1499)
ab46584 
Response to review on #1486
aarshkshah1992 added a commit that referenced this pull request Jan 31, 2024
@aarshkshah1992
Squashed commit of the following:
05f44ee 
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
Reviewers

@rvagg rvagg rvagg left review comments

@Stebalien Stebalien Stebalien approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anorth @Stebalien @rvagg