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

Feat package failed transactions #684

Merged
merged 9 commits into from
May 13, 2022

Conversation

jjyr
Copy link
Collaborator

@jjyr jjyr commented May 9, 2022

The behavior of only package success transactions caused some problems:

  1. Metamask increases sender's nonce when a transaction fail wether Godwoken packages the transaction. So
    the nonce becomes inconsistent once user send a failed transaction.
  2. DOS attacking may happen if we do not charging fee for failed transactions. There are other ways to fix DOS, but charging fee for failed transaction is the easiest one to implement. To implement this behavior also solve the compatibility issues for some EVM toolchains.

So this PR introduce a backward compatible way to fix this. When a transaction is failed(return a non-zero exit code), the written state of the transaction is reverted, the sender's nonce is increased by 1, and the transaction fee is paid to block-producer. For PolyjuiceTx the gas_used * gas_price amount of CKB is charged. For other transaction, the args.fee is charged.

Notice, we should update the readonly node first, then to update the sequencer, otherwise the readonly node may stop to work.

@jjyr jjyr requested review from zeroqn, Flouse and blckngm May 9, 2022 16:39
@jjyr jjyr requested a review from keroro520 May 9, 2022 16:46
@jjyr jjyr force-pushed the feat-package-failed-txs branch 2 times, most recently from 64428fc to 21e16a3 Compare May 10, 2022 01:38
@jjyr jjyr force-pushed the feat-package-failed-txs branch from 21e16a3 to 8c312b7 Compare May 10, 2022 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants