Subjectively fail transaction if contract bills RAM to another account within a notification handler

[arhag]

This PR introduces an optional (but enabled by default) subjective soft-fix for the issue of allowing notified contracts to charge RAM to any of the authorizers of the original action. It does so by subjectively (i.e. only during block production and not validation) failing a transaction if at any point in its execution an unprivileged contract executing under the context of handling a notification tries to bill additional RAM to an account other than itself (reducing RAM is still allowed).

This is a restriction in previously allowed behavior for a contract executing under the context of handling a notification. This means that if all active block producers of an EOSIO blockchain chose to enable this soft-fix, they would potentially restrict the ability of some existing contracts utilizing this behavior to fully function on that blockchain, at least until the contract code was updated to use a more appropriate pattern that does not rely on the restricted behavior. Note that block producers can still use a version of nodeos including these changes but without enforcing the soft-fix by setting the disable-ram-billing-notify-checks field in config.ini to true.

The most common pattern which utilizes the effected behavior is immediately creating a table row, if an appropriate one does not already exist, in response to receiving tokens from a user, and charging that user for the RAM costs of the table row. This is a pattern that was even used in an example exchange contract for depositing tokens into the exchange.

Unfortunately, supporting this particular pattern on EOSIO blockchains opens up contract developers and users to the potential of wasting their available RAM when interacting with accounts with malicious contracts deployed. While it is possible to mitigate the risk by carefully auditing the contracts deployed on accounts that will be notified as a result of a user executing an action, the existing behavior puts an unreasonable burden on contract developers and even users and can be easily argued to not be worth the benefits.

This is especially the case considering there is a straightforward alternative to the common pattern described above. Instead of creating a table row (and paying for that RAM then) in the notify handlers as a result of a transfer of tokens, the pattern could instead be:

1. The user first executes an action that creates a table row (perhaps initialized to a state that indicates it hasn't been fully initialized according to the application logic yet) of sufficient size to hold the later data that the table row will need to be mutated to hold.
2. Then, the transfer notification handler modifies that table row (which it will require to exist at that point) to hold the appropriate data.

Assuming the data modification of the table rows during the execution of the transfer notification handler does not require changing a primary key (there are future changes planned to address even this corner case) nor does it require increasing the size of the table row data, then the mutation should be successful even under the new stricter behavior. Note that these two actions can still occur atomically as before by putting them (in the right order) within the same transaction. Also note that another action may sometimes be desired to explicitly erase the table row (returning that RAM costs back to the user that payed for it) under certain circumstances; for example, if the table row tracks the balance of tokens deposited in the contract, then if all those tokens have been fully withdrawn, it would be sensible for the contract to allow the user to free that 0-balance table row.

Furthermore, the two-step pattern described above assumes the contract requires another user to pay for RAM costs of table row. If instead the contract can simply pay for the table row RAM costs itself, there is no need for any of the additional complication.

[xJonathanLEI]

This is gonna break most existing game contracts... Some of them have even yielded contract control by setting it to eosio.code or black-hole public keys.

[nsjames]

This is great.
I have been using atomic batching of transactions for row inserting + transfers and know that it works well. It's a small amount of added work for a huge benefit to the entire ecosystem.

As far as black hole contracts go, they can simply upload contracts to another account and change their UIs. Is this a hassle? Yes, but this fix takes out a user/dev experience issue which is more important than any single dapp imo. Those contracts are already most likely vulnerable to the ram exploit as well and need to be changed anyway.

[xJonathanLEI]

Those contracts are already most likely vulnerable to the ram exploit as well and need to be changed anyway.

Good point. Those black hole contracts would have no need for RAM though, as all RAM usage are paid by users.

To me, the main downside of an action batching would be that users can no longer use my dApp with just wallet transfers. But anyways, I'm changing to using separate actions for all my contracts whether this PR is merged or not.

[heifner]

Should this be moved to trx_context.add_ram_usage so that all cases are covered in one spot. This seems to miss eosio_contract.cpp apply_eosio_* call and transaction_context.schedule_transaction.

[arhag]

The apply_eosio_* calls are not a problem since they never run in the context of a notification handler. Similarly with transaction_context::schedule_transaction which does not even run in the context of executing an action. The payer in that latter case is the first authorizer of the transaction which needs to be allowed to be billed for the RAM of the delayed transaction.

I didn't put the check in transaction_context::add_ram_usage because it doesn't have access to the apply_context fields, such as receiver and act.account, needed to make the decision of whether or not to throw the subjective_block_production_exception.

[nsjames]

To me, the main downside of an action batching would be that users can no longer use my dApp with just wallet transfers.

Indeed this is the one sad reality about this.

[opcheese]

Sorry, if this is the wrong place to comment, but if possible I would like high impact features like this to be opt-in rather than opt-out for BPs for a "deprecation period" (make disable-ram-billing-notify-checks "true" by default for first release and change it to "false" in the next one). Onus is on the BPs either way, but this might give DApps like ours a bit more time to adapt

[vjoke]

Good news to hear that you guys have done something to resolve my issue #4824 raised one month ago.