Batch ClaimAllocations during ProveCommitAggregate

[alexytsu]

Work towards #1278.

TODOs

• Make existing tests pass with new expectations
• Fix return values so that a followup can support partially successful batches of DealActivation

Batching of claim allocations saves both messaging overheads and state mutation costs by only updating the DataCap actor once. The full stack traces can be found here.

The total gas costs for the PCA message drops approximately ~25%

Before:

Span[Root, self: {sum=0, none=0}, total: {sum=322,973,688, OnVerifyAggregateSeals=115,956,010, OnBlockLink=101,446,488, wasm_exec=43,387,915, OnBlockOpenBase=31,302,480, OnSyscall=14,112,000, wasm_memory_init=12,504,269, OnMethodInvocation=2,100,000, wasm_memory_grow=760,218, OnActorUpdate=475,000, OnBlockOpenPerByte=382,410, OnGetRandomness=344,160, OnBlockCreate=171,410, OnBlockRead=15,907, OnHashing=9,422, OnValueTransfer=6,000, OnGetActorCodeCid=0, OnBlockStat=0, OnNetworkContext=0, OnSelfBalance=0, OnGetBuiltinActorType=0, OnMessageContext=0, none=0}]

After:

Span[Root, self: {sum=0, none=0}, total: {sum=239,773,152, OnVerifyAggregateSeals=115,956,010, OnBlockLink=51,931,891, wasm_exec=32,281,749, OnBlockOpenBase=22,117,920, OnSyscall=8,526,000, wasm_memory_init=6,265,242, OnMethodInvocation=1,050,000, OnActorUpdate=475,000, wasm_memory_grow=393,216, OnGetRandomness=344,160, OnBlockOpenPerByte=308,670, OnBlockCreate=95,210, OnBlockRead=12,859, OnHashing=9,226, OnValueTransfer=6,000, none=0, OnGetBuiltinActorType=0, OnGetActorCodeCid=0, OnNetworkContext=0, OnBlockStat=0, OnMessageContext=0, OnSelfBalance=0}]

[anorth]

(Thinking out loud) I see how we need some change here: when batching across sectors, we need some way to return how much claimed space is associated with each sector, because the caller doesn't know.

The parameters are not grouped by sector, so it makes sense that the result is not grouped by sector either: there could be multiple claims for a single sector.

We don't need to return the sector number though, and there's now some redundancy between the BatchReturn and the list of results. I think the simplest thing would be to drop the BatchReturn, and return a vector of claim results that is parallel to (so the same length as) the parameter and contains only the claimed space. The space is zero for failures. The caller can line this up with the parameters to group by sector number.

(A similar but more complicated scheme would keep the batch return but drop the failed results, but then iterating to line up is hard. I don't think we should optimise for the failure case).

@ZenGround0 what do you think here?

[anorth]

Thanks this looks pretty good.

I am very reluctant to even temporarily introduce this duplication of the single/batch calls though. Thanks for attempting to keep this PR small. Can you try to do the refactor against master first though, so that both call sites are using a path that will allow batching, then add the batching?

[anorth]

This pattern makes me quite nervous. I wish we had try_map in stable.

For now please explicitly check that sector_claims ends up the right size, and abort (USR_ASSERTION_FAILED) if not.

[alexytsu]

I might be missing something here, but wouldn't map over params.allocations be sufficient to ensure that the result array stays parallel?

Also, given BatchReturn is no longer part of ClaimAllocationsReturn, I'll remove the ret_gen tracking, though this drops the nuance of specific error codes from the error message (it's already missing from the return value now).

[alexytsu]

I don't know if this is valid. Is it possible to have a valid but zero-sized claim?

[anorth]

I think map is hard to use if you want to bubble a result with ?, like at 433 (unless you want to deal with a Map<Result<>>. That's why try_map.

This logic of checking for zero size is valid at the moment, because the minimum Allocation size is positive. It's probably safe forever, just nervous.

[anorth]

I think map is hard to use if you want to bubble a result with ?, like at 433 (unless you want to deal with a Map<Result<>>. That's why try_map.

This logic of checking for zero size is valid at the moment, because the minimum Allocation size is positive. It's probably safe forever, just nervous.