Default-reject value transfers

[Stebalien]

At the moment, built-in actors accept value transfers on all method numbers by default. To prevent unintentional value transfers and match expected behavior for users coming from solidity, we should consider forbidding value transfers on methods that don't expect them.

Proposed methods that should accept value transfers:

• All non-singleton constructors (for endowments).
• Init::Exec, Init::Exec4 -- actor endowment.
• Market::AddBalance
• Miner::SubmitWindowedPost -- may be useful for periodic top-up? It's unclear if this one should be allowed.
• Miner::DeclareFaults, Miner::RepayDebt, Miner::DeclareFaultsRecovered to repay debts/fees.
• All prove/pre-commit and snap/extension methods on the miner to cover deposits.
• Multisig::Propose -- (TODO: not sure if this is useful)
• Power::create_miner -- miner endowment.
• EAM::create* -- EVM smart contract endowments.
• EVM::invoke -- payments will be managed/rejected by the EVM smart contract itself.

Reasons not to do this:

1. Given method-0, there's no way to reliably reject all transfers, so rejecting transfers by default for all other methods doesn't add any security.
2. The motivation is a bit weak.

[arajasek]

So, I'm pretty strongly in favour of doing this, and have wanted to for a long while (unrelated to anything Eth / Solidity).

Do you have an idea for how you would implement this? That's honestly the reason I never proposed this -- we currently handle transfers before any actor-specific logic. I worry that this might need a non-trivial refactor of some critical code, which would then need careful review and testing, which...makes it not worth it. But if you have a good idea for how we could do this easily, I'm all ears.

[Stebalien]

@alexytsu has already done it: filecoin-project/builtin-actors#1241

[anorth]

I'm in favour of this. I don't think "doesn't add any security" is the right evaluation – it's a UX feature and it will almost certainly prevent some lost tokens.

I've a question for core devs and @filecoin-project/fips-editors whether we think this needs a FIP. It is an observable change, but only to make some calls that were either definitely or probably errors safe against losing tokens.

We could scope it down to reject only definitely-bad transfers, where we can prove the tokens are lost. But I think it's more useful and valuable to reject probable errors too (e.g. Miner::ReportConsensusFault - the tokens aren't lost, but giving them to the miner is almost certainly a mistake).