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

draft specs of invalid tx #2

Closed
wants to merge 4 commits into from

Conversation

smtmfft
Copy link

@smtmfft smtmfft commented Nov 10, 2022

According to https://github.com/taikochain/internal/issues/22, I made this draft specs.

Signed-off-by: smtmfft [email protected]

specs/invalid-tx-proof.md Show resolved Hide resolved
specs/invalid-tx-proof.md Outdated Show resolved Hide resolved
specs/invalid-tx-proof.md Outdated Show resolved Hide resolved
specs/invalid-tx-proof.md Outdated Show resolved Hide resolved
specs/invalid-tx-proof.md Outdated Show resolved Hide resolved
specs/invalid-tx-proof.md Outdated Show resolved Hide resolved
@Brechtpd
Copy link

We can probably depend on privacy-scaling-explorations#313 when it's available for the ETH balance case. I didn't think this case could be seen as a similar error case as when it happens in a smart contract execution.

Also because the PSE team wants it (privacy-scaling-explorations/zkevm-circuits#872 (comment)), please add support for the insufficient intrinsic gas when you can (if a future PR makes more sense that's okay).

@Brechtpd
Copy link

Brechtpd commented Nov 16, 2022

LGTM! But don't merge this in just yet because it does seem like everything we will do will end up in the shared repo so mentions of taiko specific stuff should be removed and should be put into a separate folder or something to reduce potential conflicts.

@smtmfft smtmfft requested a review from Brechtpd December 5, 2022 09:56
else:
# 1b. total_txs matches the tx_id that corresponds to the final step.
# 1b. total_valid_txs matches the tx_id that corresponds to the final step.
instruction.constrain_equal(
instruction.call_context_lookup(CallContextFieldTag.TxId), total_txs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
instruction.call_context_lookup(CallContextFieldTag.TxId), total_txs
instruction.call_context_lookup(CallContextFieldTag.TxId), total_valid_txs

As the comment above says?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the comments is replaced by mistake, here still using total_txs.
That is part of the reason why I wanted to use another new table in front of the existed tx table (if possible/necessary), as we need to carefully deal with existed tx table log now, and later circuit writers need to keep the invalid skip logic in mind once this flag is added. Not sure if it add extra burden to others, however, so far these changes seems workable and acceptable I guess?? What's your opinion?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay. Is the only difficulty recovering the "real" tx id (counting up using only the valid transactions)? If so seems easy enough to keep track of that (and perhaps even put it in the tx table).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I now think adding more flags (auxiliary table columns) to tx table if necessary is a better approach, although those who query tx table need to keep them in mind and adjust the way how to use the values accordingly.

Comment on lines 21 to 23
# gas_used & refund == 0 if tx is invalid
instruction.constrain_zero(is_tx_invalid * gas_used)
instruction.constrain_zero(is_tx_invalid * effective_refund)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm shouldn't this be enforced elsewhere, or maybe just be something like

gas_used = is_tx_invalid * gas_used;
effective_refund = is_tx_invalid * effective_refund;

Both gas_used and effective_refund seem to be calculated directly from other data sources so I don't know how this check helps. Either it's already always 0 because of some other checks in begin_tx or something, or this check is useless because gas_used and effective_refund still have their normal values when is_tx_invalid is true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be I think, let me check if they are inherited from begin_tx or not.

Copy link
Author

@smtmfft smtmfft Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be merged into gas handling logic in begin_tx, which makes gas_left equal tx.gas if tx is invalid, so that here gas_used == 0.
but refund is checked here only, left one enforcement here I think is OK??

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm yes seems like refund is read from the tx table but not checked anywhere else. If that's the case the new code makes sense to me.

@smtmfft
Copy link
Author

smtmfft commented Dec 6, 2022

Add access list logic, but current specs supports legacy tx type only, so the real access list is still a TODO.

ashWhiteHat and others added 4 commits December 29, 2022 23:40
* integrate lt gt eq

* Apply suggestions from @han0110's code review
* draft specs of invalid tx

Signed-off-by: smtmfft <[email protected]>

* fix python spec test

Signed-off-by: smtmfft <[email protected]>

* Adjust md format

* fix review comments

Signed-off-by: smtmfft <[email protected]>

* fix review comments

Signed-off-by: smtmfft <[email protected]>

* use post balance checking to save 1 read lookup

Signed-off-by: smtmfft <[email protected]>

* fix dup code

Signed-off-by: smtmfft <[email protected]>

* add more test

* fix test annotation

Signed-off-by: smtmfft <[email protected]>

* add invalid tx process to end_tx & end_block

Signed-off-by: smtmfft <[email protected]>

* handle receipt and log when tx is invalid

* fix test

* fix review comments

* add intrinsic gas handling, accesslist gas = 0 as a TODO

* add access list gas checking

* fix review comments

* remove unnecessary files

* fix typpe check

* fix type check

* Update specs/tables.md

Co-authored-by: Brecht Devos <[email protected]>

* Update specs/tables.md

Co-authored-by: Brecht Devos <[email protected]>

* Update src/zkevm_specs/evm/execution/begin_tx.py

Co-authored-by: Brecht Devos <[email protected]>

* Update src/zkevm_specs/evm/execution/begin_tx.py

Co-authored-by: Brecht Devos <[email protected]>

* Update src/zkevm_specs/evm/execution/begin_tx.py

Co-authored-by: Chih Cheng Liang <[email protected]>

* Update src/zkevm_specs/evm/execution/begin_tx.py

Co-authored-by: Chih Cheng Liang <[email protected]>

* fix lint

* fix lint

* fix pytest

* fix pytest

* fix pytest

Signed-off-by: smtmfft <[email protected]>
Co-authored-by: Brecht Devos <[email protected]>
Co-authored-by: Chih Cheng Liang <[email protected]>
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.

5 participants