chore: remove PRE_COMMIT_SECTOR_BATCH_MAX_SIZE and other gas-limited parameters

[kamuik16]

Closes #1268

[rvagg]

@kamuik16 make check is failing in CI, it runs clippy over the whole lot

[kamuik16]

@kamuik16 make check is failing in CI, it runs clippy over the whole lot

CI is passing now.

[rvagg]

Code seems good to me on a cursory review, but we need to draft a FIP for this because it's a protocol change, even though a minor one. There may be some unforeseen issues that come up during discussion in the FIP process.

[rvagg]

So I bundled this up, modified some itest infra in lotus and ran an experiment and I could precommit a little above 1,000 sectors before hitting the message size limit. Gas is 514,759,261 when doing 1,000, although it's a new miner with minimal existing state to mutate so that's the cheapest case.

Which all means that this isn't really "gas limited", at least for precommit batching. We'll have to get feedback before proceeding with this. I'll ask on Slack, maybe consider a draft FIP to gather discussion.

Either way, @kamuik16, this one isn't going to get resolved very quickly. You might be best to pick another item off the possible list and move on to something productive while we figure this out. Thanks for the work so far (I can at least confirm that it works in for precommits when integrated into lotus).

[kamuik16]

So I bundled this up, modified some itest infra in lotus and ran an experiment and I could precommit a little above 1,000 sectors before hitting the message size limit. Gas is 514,759,261 when doing 1,000, although it's a new miner with minimal existing state to mutate so that's the cheapest case.

Which all means that this isn't really "gas limited", at least for precommit batching. We'll have to get feedback before proceeding with this. I'll ask on Slack, maybe consider a draft FIP to gather discussion.

Either way, @kamuik16, this one isn't going to get resolved very quickly. You might be best to pick another item off the possible list and move on to something productive while we figure this out. Thanks for the work so far (I can at least confirm that it works in for precommits when integrated into lotus).

Ohh okay.

[Stebalien]

We discussed this on slack (TL;DR: probably fine) but I wanted to followup here. At the end of the day:

1. There are no concern with respect to message size as messages are included before they're even executed (size limits are applied at the message/block propagation layers via both protocol & gas restrictions).
2. Gas should cover the compute time. If stuffing 1,000 pre-commits into a single message takes less gas than the gas allocated to a single block, then it should be fine. If it isn't, that means our gas model is wrong.

The only things to watch out for are:

1. State written via the FVM. The maximum IPLD block size is 1MiB in the FVM but none of the built-in actors will ever create blocks this large due to other constraints. These large blocks are allowed because they should technically be fine, but I'd be uncomfortable going over ~256KiB without good reason until we get more edge-case testing.
2. Syscalls into the FVM. Specifically crypto::compute_unsealed_sector_cid, crypto::verify_aggregate_seals, crypto::verify_replica_update, and crypto::batch_verify_seals because they read CBOR supplied by the actor and they don't apply any limits on said CBOR and assume reasonable limits are applied by the actors. These are "very special" syscalls that should really be replaced by, e.g., low-level BLS and/or snark syscalls.

[Stebalien]

To expand on my comment WRT syscalls like crypto::verify_replica_update, removing the limit on the number of updates is fine because each update is applied with a separate call to crypto::verify_replica_update. However, e.g., removing the check on the length of the proof of each update would not be fine.

[Stebalien]

Also note, this is part of #1268 but doesn't fix it completely. E.g., there's no reason to check addressed_partitions_max and/or addressed_sectors_max in most cases.

[rvagg]

We're going to let this PR sit until we've got a FIP in for the changes. It should be straightforward but there's some uncertainty about what the FIP process might throw up regarding this change.

[jennijuju]

We're going to let this PR sit until we've got a FIP in for the changes. It should be straightforward but there's some uncertainty about what the FIP process might throw up regarding this change.

Why are we holding the FIP for this change? I think this is a straightforward protocol improvement.

[anorth]

We're going to let this PR sit until we've got a FIP in for the changes. It should be straightforward but there's some uncertainty about what the FIP process might throw up regarding this change.

Why are we holding the FIP for this change? I think this is a straightforward protocol improvement.

I think you've misunderstood. The PR is being held for the introduction of a FIP specifying the changes. It is straightforward, but does have functional effects so needs to follow the process.

[BigLep]

I'm moving this back to draft given it needs the corresponding FIP. We can mark ready for review/merge once the FIP is there. I don't have confirmation, but I heard discussion of doing the FIP work as part of filecoin-project/FIPs#1094

[rvagg]

See filecoin-project/FIPs#1098

[rvagg]

let's make sure we tie this with #1584 and close them both when we get it finally resolved

[BigLep]

Given we have https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0100.md , I'll mark this ready for review

[rvagg]

Retargeting this to the feat/fip-0100 branch and we'll bundle with that. I've also closed #1584 which was a dupe but has less dead-code removal than this one.