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

Transaction scheduler should use CheckTx #1963

Closed
kostko opened this issue Jul 30, 2019 · 4 comments · Fixed by #2502
Closed

Transaction scheduler should use CheckTx #1963

kostko opened this issue Jul 30, 2019 · 4 comments · Fixed by #2502
Assignees
Labels
c:security Category: security sensitive

Comments

@kostko
Copy link
Member

kostko commented Jul 30, 2019

As of #1555 the runtime exposes a CheckTx API for verifying incoming transactions but this is currently unused. The transaction scheduler should use the API to reject incoming transactions early instead of submitting them through the processing pipeline.

The runtime must ensure that any such checks are cheap enough that they can be performed quickly and not become a bottleneck of the whole pipeline.

Estimated cost: 1 sprint

@kostko kostko added c:security Category: security sensitive c:worker-txnsched labels Jul 30, 2019
@peterjgilbert
Copy link
Contributor

peterjgilbert commented Sep 3, 2019

The runtime must ensure that any such checks are cheap enough that they can be performed quickly and not become a bottleneck of the whole pipeline.

Currently the runtime checks are cheap (https://github.com/oasislabs/runtime-ethereum/blob/9847c5359d04a6ea423739f70f2f511ea37dbf66/src/methods.rs#L27):

  1. Decode the transaction
  2. Check the signature
  3. Check that gas limit and gas price have allowed values

@kostko
Copy link
Member Author

kostko commented Sep 17, 2019

The checks also need to make sure that there is enough balance to pay for gas in case the transaction is reverted due to a mismatched predicted read/write set (see #2012 (comment)).

@kostko
Copy link
Member Author

kostko commented Sep 24, 2019

As discussed the CheckTx API should be extended so that the return value includes additional metadata (e.g., predicted read/write sets for the transaction). The existing way of transporting read/write sets should be cleaned up.

@kostko
Copy link
Member Author

kostko commented Dec 10, 2019

Performing CheckTx should be a node configuration parameter so it can be disabled if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:security Category: security sensitive
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants