Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VISION: Try..catch for substrate runtimes #370

Open
pepyakin opened this issue Jun 30, 2019 · 12 comments
Open

VISION: Try..catch for substrate runtimes #370

pepyakin opened this issue Jun 30, 2019 · 12 comments
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@pepyakin
Copy link
Contributor

pepyakin commented Jun 30, 2019

ℹ️ NOTE 2022-11-07:

This is a classical issue which I wrote with my contracts hat on. However, it is not specific to contracts.

At the moment, a runtime call modifies storage eagerly. I.e. the runtime performed some storage changes there is no way of rolling back these changes. Because of that, substrate modules leverage the first-check-then-modify approach. This means that all conditions should be checked before making any side-effects. This also has a serious ramification for the runtime, the runtime MUST NOT panic under any circumstances. If it were to do so, then it opens up a DoS vector, since all changes including the charging fees for a transaction are rolled back.

In the srml-contracts module we implement a system that emulates checkpoint functionality. It does this by storing all changes performed by a smart-contract in memory, committing them to the main storage if and only if the smart-contract successfully finishes the execution. This however is not ideal, since the rest of the runtime logic acts eagerly and changes the storage as soon as that logic is called and there is no way of rolling these changes back. This really poses a problem when interaction between the smart-contract module and the rest of the runtime comes into a play: the smart-contract should revert all changes in the case of failure, so that means we can't call the runtime directly and we need to delay all calls after the smart-contract finished successfully. That also implies that the smart-contracts can't easily get the result of the execution of such a runtime call.

This makes me think that it might be beneficial to lift the checkpointing logic to substrate itself. If we had substrate-wide support of such pattern, then we could fix the issue of interaction between smart-contracts, make some patterns safer and most importantly make the runtime more tolerable to panics.

There might be different designs of such a mechanism, but here is one:

  1. We introduce a new host function, called ext_try. It takes the name or the pointer to the function.
  2. It creates a new Ext that is linked with the parent's one.
  3. A new instance of the runtime is initialized and the specified function is called.
  4. All changes are collected in the nested Ext.
  5. If the nested call succeeds, then we commit all the changes to the parent's Ext. If the runtime traps (e.g. panics, overflows its stack or so on), the changes are just discarded.
  6. The status is conveyed to the caller of ext_try.

Having this functionality:

  1. We could express executive module as just charging the fee and ext_try-ing to dispatch transaction. There is a chance that this would open us a way to use more sanity checks (i.e using assert!s) further improving the security of a chain. It is worth noting that some of the path still have to be panic-free.
  2. We could simplify smart-contracts to just execute code directly without involving abstractions like AccountDb. Transfers would be implemented as regular transfers, without trying to replicate what the T::Currency trait does. Calling to the runtime would be as simple as dispatching the function and getting the result.
  3. It might open-up other patterns that can leverage this mechanism, where it is really hard to ensure all the invariants up-front (or at least without reingereering the whole runtime).
@xlc
Copy link
Contributor

xlc commented Jul 1, 2019

This will make paritytech/substrate#1791 possible

@shawntabrizi
Copy link
Contributor

To me, a feature like this is a huge gain in terms of simplicity to hack and build on top of other existing modules, and could add to the virality of the Substrate platform. Ultimately, the pattern that is challenging for runtimes is chaining multiple different pieces of logic together in a single module function.

Let's say for example buying a kitty in a custom runtime module. There may be two distinct steps which are involved here:

  1. Checking that balance transfer will succeed and executing that
  2. Checking that the token transfer will succeed and executing that

With the current module system, you must write the function like so:

  1. Check that the balance transfer will succeed
  2. Check that the token transfer will succeed
  3. Transfer balance
  4. Transfer token

Users cannot in this case consolidate the check/write pattern into a single functional component which then gets serially called by the runtime function.

This would often come up when you want to make a function which combines your own logic with multiple downstream runtime module calls. For example imagine a runtime function which calls 3 other modules to:

  • Complete a custom auction
  • Transfer some Balance
  • Transfer some Asset

All of the functions in this example would be dispatchable functions defined in different modules. As a developer, I would have no easy way to execute all 3 by calling the implementations in their respective runtime module. I would need to copy their implementations, separate it into check and write components, and reorder the logic. At this point, the user has just written a new module which re-implements all of this code in other places...

I had thought about recommending to users to write their module functions in two halves, a check function and a write function. So something like check_can_transfer and make_transfer. However, this fails to work when the actions of one call could affect the ability to successfully check another. For module function which does two transfers might naively be written like this:

  • check_can_transfer(10 units to Alice)
  • check_can_transfer(10 units to Bob)
  • make_transfer(10 units to Alice)
  • make_transfer(10 units to Bob)

But if the caller only has 10 units to begin with, then both checks will pass (since state is not updated between checks) but fail later after the transfer to Alice already happened, and cannot be reverted.

The ability to do patterns like this made Ethereum smart contracts and ethereum standards work so well. If we look at the way that contracts like ERC-721 were written by teams like OpenZepplin, you will see there is a base contract ERC721.sol and then multiple "extension" contracts like ERC721Enumerable.sol. These extension contracts work because users are able to add additional check/write patterns on top of an existing base, without having to get too dirty into someone else's code.

@vjoke
Copy link

vjoke commented Sep 23, 2019

when will this feature be available?

@pepyakin
Copy link
Contributor Author

One part of it, namely transactional storage, is being worked on in paritytech/substrate#3263.

@pepyakin
Copy link
Contributor Author

pepyakin commented Jun 20, 2020

↑ that was subsumed by paritytech/substrate#6269 and was merged.

@ggwpez
Copy link
Member

ggwpez commented May 30, 2022

I think this can be closed as we now have default transactional behaviour for extrinsics.

@shawntabrizi
Copy link
Contributor

Closed with: paritytech/substrate#10806

Would be interested to see if there is any panic catching thing we can do in the runtime too.

@pepyakin
Copy link
Contributor Author

The issue is meant to be tracking general try catch where storage transactions are a part of. Let's keep it reopened until we get there.

@pepyakin pepyakin reopened this May 31, 2022
@bkchr bkchr unassigned cheme May 31, 2022
@athei
Copy link
Member

athei commented Jun 6, 2022

Yeah storage transactions won't help with panics. Without panic safety we cannot ensure that we always collect fees.

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/future-of-webassembly-and-ink-smart-contracts/428/5

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/the-xpost-bot-is-awesome-but-can-be-make-it-prettier/448/1

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/eliminating-pre-dispatch-weight/400/14

@kianenigma kianenigma changed the title Try..catch for substrate runtimes VISION: Try..catch for substrate runtimes May 6, 2023
@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed J0-enhancement labels Aug 25, 2023
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
Bumps [ssri](https://github.com/npm/ssri) from 6.0.1 to 6.0.2.
- [Release notes](https://github.com/npm/ssri/releases)
- [Changelog](https://github.com/npm/ssri/blob/v6.0.2/CHANGELOG.md)
- [Commits](npm/ssri@v6.0.1...v6.0.2)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

10 participants