FIP-0076: Merge integration/direct-onboarding into master

[anorth]

See https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0076.md

This code was originally reviewed when landing on integration/direct-onboarding in these PRs. For any blocking changes (or sufficient non-blocking ones), I'll make a new PR against that base to resolve them.

Reviewers: in addition to checking over the code please point to or suggest additional testing we can do following the merge.

Note this code also contains the following FIP amendments:

• FIP-0076: Rename ProveCommitSectors2/ProveReplicaUpdate2 to ProveCommitSectors3/ProveReplicaUpdates3 FIPs#886
• FIP-0076: Renumber the new methods to avoid collision with FIP-0070 FIPs#892
  We don't need to wait for them, but if they are changed or rejected, the code needs to change to match.

Closes #1342.

[Stebalien]

First pass of a code review.

[Stebalien]

Won't this always be all the deals in the sector? Given that:

1. Cron is paying for it.
2. An SP can create a sector with a lot of deals (thousands?).

This makes be a bit uncomfortable.

If we don't expect missing deals, we can sort then filter:

                    deals_to_remove.sort();
                    let deals_to_remove = deals_to_remove.iter().peekable();
                    let new_deals = existing_deal_ids // assume sorted?
                        .deals
                        .iter()
                        .filter(|deal_id| deals_to_remove.next_if_eq(deal_id).is_none())
                        .cloned()
                        .collect();

(we can skip missing deals, it's just slightly less clean).

[anorth]

Won't this always be all the deals in the sector?

Only if they all expire at around the same epoch.

Also there is a policy limit of 256 deals / sector, at least in the current precommit-based and non-DDO flow. You've inadvertently pointed to an interesting item that there is not a policy limit on the number of piece manifests activated with an individual sector in the new flow. This is relevant to concurrent discussion about removing artificial policy limits that could be better limited by gas.

With FIP-0074 cron will no longer pay for this (for new deals). The deal client will specify the deals to settle, which becomes deals_to_remove. We need to non-trivially rebase that code on FIP-0076 after landing this, but I expect the issue to become pretty minor.

[Stebalien]

Only if they all expire at around the same epoch.

It's more a matter of it being under the control of the miner (who can self-deal here).

Also there is a policy limit of 256 deals / sector

Ok, that makes me feel a lot better (although 256^2 is still a fair amount).

With FIP-0074 cron will no longer pay for this (for new deals). The deal client will specify the deals to settle, which becomes deals_to_remove. We need to non-trivially rebase that code on FIP-0076 after landing this, but I expect the issue to become pretty minor.

Ah, is that slated for this upgrade? If so, I have no more concerns here.

[Stebalien]

Another chunk (lots of nits you can ignore, some you probably shouldn't).

[anorth]

Thanks for the review. I think the only significant item I haven't addressed in #1499 is #1486 (comment) . I propose that we leave it as-is until after rebasing and landing #1483 on top of this. Then it will be merely a question of efficiency of large batches, which we should probably improve but isn't critical. (I would say it must handle missing deals, so can't directly use the code you suggested).

[anorth]

I've merged the changes from #1499 into here and will mark the associated comments as resolved.

[Stebalien]

Suggested change

	batch_return.successes(&update_sector_infos).iter().map(|x| (*x).clone()).collect();
	batch_return.successes(&update_sector_infos).into_iter().cloned().collect();

[Stebalien]

note: we do import the itertools crate, so you can write this as:

Suggested change

	let data_activation_inputs: Vec<DealsActivationInput> =
	update_sector_infos.iter().map(|x| x.into()).collect();
	let data_activation_inputs: Vec<DealsActivationInput> =
	update_sector_infos.iter().map_into().collect();

(but that is relying on an external crate, so up to you)

[Stebalien]

SGTM. Just wanted to make sure this was intentional.

[Stebalien]

Only if they all expire at around the same epoch.

It's more a matter of it being under the control of the miner (who can self-deal here).

Also there is a policy limit of 256 deals / sector

Ok, that makes me feel a lot better (although 256^2 is still a fair amount).

With FIP-0074 cron will no longer pay for this (for new deals). The deal client will specify the deals to settle, which becomes deals_to_remove. We need to non-trivially rebase that code on FIP-0076 after landing this, but I expect the issue to become pretty minor.

Ah, is that slated for this upgrade? If so, I have no more concerns here.

[Stebalien]

Probably my last comments.

[Stebalien]

Suggested change

	sector_infos.push(sectors.must_get(update.sector)?);
	let sector_type = sector_infos.last().unwrap().seal_proof;
	let sector = sectors.must_get(update.sector)?
	let sector_type = sector.seal_proof;
	sector_infos.push(sector);

[Stebalien]

nit:

Suggested change

	let mut notifications: Vec<ActivationNotifications> = vec![];
	for (update, sector_info) in successful_manifests {
	notifications.push(ActivationNotifications {
	sector_number: update.sector,
	sector_expiration: sector_info.expiration,
	pieces: &update.pieces,
	});
	}
	let notifications: Vec<ActivationNotifications> = successful_manifests
	.into_iter()
	.map(|(update, sector_info)| ActivationNotifications {
	sector_number: update.sector,
	sector_expiration: sector_info.expiration,
	pieces: &update.pieces,
	})
	.collect();

(for simple cases, not really critical I guess).

[Stebalien]

            match &precommit.unsealed_cid {
                Some(Some(compact_commd)) if !is_unsealed_sector(&commd) => {
                    return Err(actor_error!(illegal_argument, "unsealed CID had wrong prefix"));
                }
                None => return Err(actor_error!(
                                illegal_argument,
                                "unspecified CompactCommD not allowed past nv21, need explicit None value for CC or CommD")),
                _ => {},
            }

[Stebalien]

(slightly easier to read, IMO)

[rvagg]

but won't work because there's a tuple struct in the middle, maybe this?

            match precommit.unsealed_cid.as_ref().and_then(|c| c.0.as_ref()) {
                Some(compact_commd) if !is_unsealed_sector(&compact_commd) => {

[Stebalien]

Ah. It can also be written as:

            match &precommit.unsealed_cid {
                Some(CompactCommD(Some(commd))) if !is_unsealed_sector(commd) => {
                    return Err(actor_error!(illegal_argument, "unsealed CID had wrong prefix"));
                }
                None => return Err(actor_error!(
                                illegal_argument,
                                "unspecified CompactCommD not allowed past nv21, need explicit None value for CC or CommD")),
                _ => {},
            }

[Stebalien]

Will this not be an issue during a migration period? Can there not be "null" pre-commits on-chain right now?

[anorth]

This is checked as a precondition earlier in this method (the code block that your previous comment is attached to). But thanks - you've pointed out that we can remove this Option now. I'll do it in a follow-up.

[Stebalien]

Perf nit:

Suggested change

	&[update.sector_info.clone()],
	&[new_sector_info.clone()],
	std::slice::from_ref(&update.sector_info),
	std::slice::from_ref(&new_sector_info),

(you can transform a reference into a size-one slice with no copying)

[anorth]

#1508 has landed with final review responses.