Reject unexpected transfers

[alexytsu]

Closes #836

Adds payable to the runtime which must be called when funds are sent to a method.

It aborts the method if funds are received and payable was never called.

[alexytsu]

An alternate approach to structure the MockRuntime could look something like this https://github.com/helix-onchain/builtin-actors/pull/3. It might reduce boilerplate but requires drilling the value sent to the invocation of the call method.

[alexytsu]

Perhaps to reduce boilerplate, set_value could internally call expect_value. I can't think of a case where the values ought to be different though doing it implicitly is slightly more magical

[anorth]

I think the reverse of having expect_value always set the value should work. There appear to be some test cases that use set_value without expect_value, but they're rather suspect so maybe we can remove set_value altogether?

[Stebalien]

I believe we also need some storage provider methods to be payable:

• Maybe SubmitWindowedPoSt? IIRC, storage providers may use this for top-up? But I'm not sure (cc @arajasek).
• PreCommitSector/ProveCommitSector/ExtendSectorExpiration(2)/PreCommitSectorBatch(2)/ProveCommitAggregate/ProveREplicaUpdates(2) - To cover deposits.
• DeclareFaults, DeclareFaultsRecovered, RepayDebt - to pay back fee debt.

[alexytsu]

I've marked the methods mentioned from the miner as payable but not sure if there are any others in this repo that need to be reviewed. Is payability documented somewhere? else, we should compile a list now and I'll update tests to enforce these expectations.

[Stebalien]

I've marked the methods mentioned from the miner as payable but not sure if there are any others in this repo that need to be reviewed. Is payability documented somewhere? else, we should compile a list now and I'll update tests to enforce these expectations.

This never a concept before (by default, everything is "payable"), so no. The only reliable way to determine this would be to ask the sentinel team to tell us which methods tend to carry value.

Note: this will definitely require a FIP.

[codecov-commenter]

Codecov Report

Merging #1241 (095833c) into master (9cb8a6b) will decrease coverage by 0.11%.
The diff coverage is 89.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1241      +/-   ##
==========================================
- Coverage   90.97%   90.87%   -0.11%     
==========================================
  Files         133      133              
  Lines       26633    26679      +46     
==========================================
+ Hits        24230    24245      +15     
- Misses       2403     2434      +31     

Impacted Files	Coverage Δ
runtime/src/runtime/mod.rs	89.28% <ø> (ø)
test_vm/src/lib.rs	83.08% <68.75%> (-0.16%)	⬇️
runtime/src/test_utils.rs	82.65% <93.33%> (+0.10%)	⬆️
actors/eam/src/lib.rs	93.30% <100.00%> (ø)
actors/evm/src/lib.rs	92.74% <100.00%> (+0.02%)	⬆️
actors/init/src/lib.rs	92.55% <100.00%> (+0.33%)	⬆️
actors/market/src/lib.rs	91.13% <100.00%> (ø)
actors/miner/src/lib.rs	83.31% <100.00%> (+0.07%)	⬆️
actors/multisig/src/lib.rs	94.80% <100.00%> (-0.04%)	⬇️
actors/power/src/lib.rs	83.36% <100.00%> (-1.30%)	⬇️
... and 7 more

[anorth]

Note: this will definitely require a FIP.

I'm not so sure about that (and sorry @alexytsu if it turns out it does, I shouldn't have lined it up for you yet). Perhaps we can open a FIP discussion to determine this though.

[anorth]

I think the following should also be payable:

• miner::terminate_sectors (top up after penalties)
• miner::compact_partitions (no reason not to)
• miner::compact_sector_numbers (ditto)
• miner::apply_rewards (necessary, even though miner doesn't inspect the amount)

I could be argued into some more owner/worker-only ones too, though I think they would be quite rare.

[anorth]

I think the reverse of having expect_value always set the value should work. There appear to be some test cases that use set_value without expect_value, but they're rather suspect so maybe we can remove set_value altogether?