Skip to content
This repository was archived by the owner on May 3, 2024. It is now read-only.

add rlp decode circuit #126

Closed
wants to merge 9 commits into from
Closed

Conversation

smtmfft
Copy link
Collaborator

@smtmfft smtmfft commented Jul 10, 2023

RLP decoding circuit for txlist. It supports both correct rlp bytes and incorrect rlp bytes.
Design doc: https://www.notion.so/taikoxyz/RLP-circuit-design-doc-e59ddfda4b5a40e6ac0f170f23154ed8

So far the majority decoding logic seems ok, rest jobs are:
TODOs:

  • lookup to keccak table to insure txlist input, which may also involve hi-lo change.
  • design output params (maybe a lookup table??) for other circuits.
  • logic completeness check, for valid bytes at least.
  • refactory using circuit-tool.
  • more test coverage, especially for invalid cases.
  • 1559 tx type support, with or without legacy depends on future design.

Sorry, something went wrong.

Copy link

@ggkitsas ggkitsas left a comment

Choose a reason for hiding this comment

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

first non-extensive and shallow pass - minor comments

Comment on lines +694 to +698
RlpDecoderTable,
/// Tx field switch table
TxFieldSwitchTable,
/// valid len/value heading byte
HeadingByteValidTable,

Choose a reason for hiding this comment

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

Question for my understanding :)

I see these 3 tables belong to different regions. Is it necessary to distinguish them with tags?

I genuinely don't know!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think lookup uses columns which contains all regions it covers. like this:
image

So, based on my understanding, I can setup region A/B/C separately, and lookup will take them all into account.
correct me if I am wrong @Brechtpd .

Comment on lines +440 to +459
// output txlist hash check
// meta.lookup_any("comsumed all bytes correctly", |meta| {
// let is_enabled = meta.query_fixed(q_first, Rotation::next());
// let input_rlc = meta.query_advice(acc_rlc_value, Rotation::cur());
// let input_len = meta.query_advice(rlp_remain_length, Rotation::cur());
// let hash_rlc = meta.query_advice(value, Rotation::cur());

// let table = &aux_tables.keccak_table;
// let table_is_enabled = meta.query_advice(table.is_enabled, Rotation::cur());
// let table_input_rlc = meta.query_advice(table.input_rlc, Rotation::cur());
// let table_input_len = meta.query_advice(table.input_len, Rotation::cur());
// let table_hash_rlc = meta.query_advice(table.output_rlc, Rotation::cur());

// vec![
// (is_enabled.expr(), table_is_enabled.expr()),
// (is_enabled.expr() * input_rlc.expr(), table_input_rlc.expr()),
// (is_enabled.expr() * input_len.expr(), table_input_len.expr()),
// (is_enabled.expr() * hash_rlc.expr(), table_hash_rlc.expr()),
// ]
// });

Choose a reason for hiding this comment

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

Suggested change
// output txlist hash check
// meta.lookup_any("comsumed all bytes correctly", |meta| {
// let is_enabled = meta.query_fixed(q_first, Rotation::next());
// let input_rlc = meta.query_advice(acc_rlc_value, Rotation::cur());
// let input_len = meta.query_advice(rlp_remain_length, Rotation::cur());
// let hash_rlc = meta.query_advice(value, Rotation::cur());
// let table = &aux_tables.keccak_table;
// let table_is_enabled = meta.query_advice(table.is_enabled, Rotation::cur());
// let table_input_rlc = meta.query_advice(table.input_rlc, Rotation::cur());
// let table_input_len = meta.query_advice(table.input_len, Rotation::cur());
// let table_hash_rlc = meta.query_advice(table.output_rlc, Rotation::cur());
// vec![
// (is_enabled.expr(), table_is_enabled.expr()),
// (is_enabled.expr() * input_rlc.expr(), table_input_rlc.expr()),
// (is_enabled.expr() * input_len.expr(), table_input_len.expr()),
// (is_enabled.expr() * hash_rlc.expr(), table_hash_rlc.expr()),
// ]
// });

Comment on lines +80 to +81
/// RlpDecodeTypeNum,
RlpDecodeTypeNum,

Choose a reason for hiding this comment

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

What is the intention behind this value? I see it being used as a default value but there are no constraints using it.

Is it mean to be seen in the witness at all? If not, would it make sense to prohibit this value from appearing using constraints?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, changed to a unified way of telling length/size of a enum.

@smtmfft
Copy link
Collaborator Author

smtmfft commented Jul 31, 2023

Created a new PR for 1559 tx support, all fixes are already there.
smtmfft#15
This one is closed!

@smtmfft smtmfft closed this Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants