-
Notifications
You must be signed in to change notification settings - Fork 79
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 sector->deal mapping in built-in market actor #1347
Conversation
19910d2
to
076a04b
Compare
9f255e0
to
aa23218
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## integration/direct-onboarding #1347 +/- ##
=================================================================
+ Coverage 84.27% 85.86% +1.59%
=================================================================
Files 143 144 +1
Lines 29205 28443 -762
=================================================================
- Hits 24613 24424 -189
+ Misses 4592 4019 -573
|
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.
Haven't reviewed tests yet. Leaving approved to unblock you in case you address all review before I finish reviewing tests.
actors/market/src/state.rs
Outdated
/// IDs of deals associated with a single sector. | ||
#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] | ||
pub struct SectorDealIDs { | ||
pub sector_expiration: ChainEpoch, |
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.
Are we planning to send an expiration extension event from the miner actor on every extension? If not we should consider dropping this field or renaming it to "sector_expiration_hint" or something.
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.
Not every extension event, only when the SP wants to notify this market of it. The right interpretation of this is "the last expiration epoch for this sector that I heard about" or "the minimum committed duration".
However, it's not strictly necessary for direct data onboarding. I will remove it, but leave the SectorDealIDs struct wrapping the deal IDs so there's a place to insert it later if desired.
pub struct OnMinerSectorsTerminateParamsRef<'a> { | ||
pub epoch: ChainEpoch, | ||
pub deal_ids: &'a [DealID], | ||
pub sectors: BitField, |
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.
Do you have a good sense of the worst case of bitfield size vs a vector of integers? My bitfield knowledge is lacking but I'm thinking that terminations are more likely than other operations to be random access which could limit bitfield compression benefits.
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.
When there are many terminations together (e.g. partition faulting out) I think they're actually reasonably likely to be dense. Sectors in the same partition are likely activated around the same time, and there are small gas savings to the SP for dense bitfields.
RLE+ uses varint to encode ranges, so I believe in most cases is more concise than CBOR which uses a power-of-two number of bytes (e.g. 8 bytes for anything > 2^32). @Kubuxu do you have any more insight into cases where a bitfield may perform worse than a vector?
actors/miner/src/lib.rs
Outdated
const MAX_LENGTH: usize = 8192; | ||
for chunk in deal_ids.chunks(MAX_LENGTH) { | ||
if !sectors.is_empty() { | ||
// XXX REVIEW: should we chunk this call? |
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.
It is worth figuring out if we can hit internal parameter size limits and making that case impossible. If it's already impossible I don't see a reason to chunk. If we are worried about market call costs I think the right mechanism is to reduce the per-epoch termination processing limit.
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.
Looks like it's 1MiB from https://github.com/filecoin-project/ref-fvm/blob/ac2b9b499b9d6e7de4068bbbe8f26545557edb60/fvm/src/machine/mod.rs#L158
If the sector numbers were represented as a vec of 8-byte uints, this gives room for 130k of them (55 full partitions). Both cron and manual processing of early termination are currently limited by MAX_ADDRESSED_SECTORS = 25k, so I think we're good. I will add a note to that parameter though.
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.
@Kubuxu do we have a guarantee that in the worst case bitfield will degrade to <= 8 bytes per int? Are there any edge cases where it can perform worse we should worry about?
|
||
// Converted from: https://github.com/filecoin-project/specs-actors/blob/master/actors/builtin/market/market_test.go#L1448 | ||
#[test] | ||
fn fail_when_caller_is_not_the_provider_of_the_deal() { |
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.
These three tests cases still seem relevant and I think should still have tests after this change.
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.
fail_when_caller_is_not_the_provider_of_the_deal
: this is impossible now since the caller does not specify the deals, they are looked up. Only inconsistent internal state from activation could cause this. Specifying a non-existent sector is already tested.
fail_when_deal_has_been_published_but_not_activated
: impossible since deal IDs are not specified by the caller. Only activated deals exist in the provider_sectors map. Specifying a sector with an already-expired deal is tested.
termination_of_all_deals_should_fail_when_one_deal_fails
: impossible to specify a deal that can fail to be terminated, e.g. because not activated.
These removed tests reflect fundamental simplifications we gained from decoupling.
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.
Nice, thanks for explaining
aa23218
to
491dd95
Compare
491dd95
to
611c18e
Compare
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)
Adds
provider_sectors
to market state, aHAMT[Address]HAMT[SectorNumber]SectorDeals
. The value is a sorted vec of deal IDs and the associated sector's expiration epoch. Also adds sector number to deal state, to permit finding and removing that mapping when a deal expires.Prototype for part of direct data onboarding.