owned this note
owned this note
Published
Linked with GitHub
# Scroll RLP/Tx/PI Circuit Questions/Vulns
## 1. rlp_decoding_table.format not checked
There's a typo in the following gate:
```rs=
meta.create_gate("Conditions for when key = (tx_id, format, depth, al_idx, sk_idx) stays the same", |meta| {
let mut cb = BaseConstraintBuilder::default();
cb.condition(
not::expr(meta.query_advice(rlp_decoding_table.is_stack_init, Rotation::cur())),
|cb| {
cb.require_equal(
"Unless stack is init, no change on tx_id",
meta.query_advice(rlp_decoding_table.tx_id, Rotation::prev()),
meta.query_advice(rlp_decoding_table.tx_id, Rotation::cur()),
);
// BUG
cb.require_equal(
"Unless stack is init, no change on format",
meta.query_advice(rlp_decoding_table.tx_id, Rotation::prev()),
meta.query_advice(rlp_decoding_table.tx_id, Rotation::cur()),
);
}
);
```
The second constraint should require_equal `rlp_decoding_table.format`
## ~~2. Invalid constraint in "Decoding table stack op PUSH"~~
*Correction: this is fine.*
Following is the constraints which check the transition from depth 3 to depth 4 when there is a Stack Push opcode.
```rs=
cb.condition(
and::expr([
meta.query_advice(stack_op_id_diff, Rotation::cur()),
meta.query_advice(is_stack_depth_four, Rotation::cur()),
]),
|cb| {
cb.require_zero(
"If depth increments by 1, the al_idx is 1",
meta.query_advice(is_stack_depth_diff, Rotation::cur())
* (meta.query_advice(rlp_decoding_table.al_idx, Rotation::cur())
- 1.expr()),
);
cb.require_zero(
"If depth stays the same, the al_idx increments by 1",
(1.expr() - meta.query_advice(is_stack_depth_diff, Rotation::cur()))
* (meta.query_advice(rlp_decoding_table.al_idx, Rotation::cur())
- meta.query_advice(rlp_decoding_table.al_idx, Rotation::prev())
- 1.expr()),
);
cb.require_equal(
"For a new storage key list, sk_idx always starts at 1",
meta.query_advice(rlp_decoding_table.sk_idx, Rotation::cur()),
1.expr(),
);
},
);
```
If I understand correctly, when there is a change in depth from 3 to 4, this means that we are decoding a storage key list inside a address entry.
This doesn't necessarily mean that al_idx is reset to 1 or that it increments by 1?
## 3. (RLP) `assign_sm_end_row` leads to a fixed column not being fixed
```rust=
fn assign_sm_end_row(&self, region: &mut Region<'_, F>, row: usize) -> Result<(), Error> {
region.assign_fixed(
|| "q_enable",
self.rlp_table.q_enable,
row,
|| Value::known(F::zero()),
)?;
```
https://github.com/scroll-tech/zkevm-circuits/commit/e149aecbbba79cf1fae07abc31740be4bade9a4d#diff-d5ebfb16aeaf69b107c9ac4694dded2f756eb51fba0c537971e2552aa703942eR2831
The fixed value assigned was changed from 1 to 0 in the commit. However, this means that the `q_enable` column depends on how many `sm_end_row` there are - which differs from instance to instance.
## 4. (RLP) RLC for `rlp_decoding_table.id` is done out-of-circuit
Indeed, only assignment is done by raw computation in `rlp_fsm.rs`'s `stack_op_id()`
```rust=
fn stack_op_id<F: PrimeField>(components: [u64; 5], challenge: Value<F>) -> Value<F> {
components
.into_iter()
.fold(Value::known(F::ZERO), |mut rlc, num| {
rlc = rlc * challenge + Value::known(F::from(num));
rlc
})
}
```
More clarity on the intended behavior of this column would be nice.
## 5. (Tx) `value_is_zero` doesn't work for `is_access_list_addresses_len`
```rust=
meta.create_gate("lookup to access list dynamic section condition", |meta| {
let mut cb = BaseConstraintBuilder::default();
cb.require_equal(
"condition",
and::expr([
is_access_list_addresses_len(meta),
not::expr(value_is_zero.expr(Rotation::cur())(meta)),
]),
meta.query_advice(
lookup_conditions[&LookupCondition::TxAccessList],
Rotation::cur(),
),
);
cb.gate(meta.query_fixed(q_enable, Rotation::cur()))
});
```
Here, the `value_is_zero` chip is used on the `AccessListAddressesLen` row to check if the access list is not empty - in this case, a lookup would be used. However, the `value_is_zero` chip doesn't work as intended on this row - as the `q_enable` for this chip specifically handles the case where we are on the caller address, data length, calldata row. Therefore, on the access list address length row, we could just say `value_is_zero` is true even though the value is nonzero.
```rust=
let value_is_zero = IsZeroChip::configure(
meta,
|meta| {
and::expr(vec![
meta.query_fixed(q_enable, Rotation::cur()),
sum::expr(vec![
// if caller_address is zero, then skip the sig verify.
is_caller_addr(meta),
// if call_data_length is zero, then skip lookup to tx table for call data
is_data_length(meta),
// if call data byte is zero, then gas_cost = 4 (16 otherwise)
is_data(meta),
]),
])
},
tx_table.value,
|meta| meta.advice_column_in(SecondPhase), // value is at 2nd phase
);
```
## 6. (Tx) `AccessListAddressLen = 0` should force `AccessListStorageKeysLen = 0` and `AccessListRLC = 0`
This is similar to the enforcements that are done in calldata. There's a check that if calldata is none, then call data length and call data gas cost is zero. A similar approach should be done here - especially considering that putting AccessListAddressLen zero leads to no TxAccessList lookup being done - so one can put any value for the access list storage keys len and RLC on the fixed part of the table and then put nothing on the dynamic side.
## 7. (Tx) AccessList section RLC is incorrectly done
right now
- `section_rlc' = section_rlc * r^20 + field_rlc'` is done on access list address
- `section_rlc' = section_rlc * r^32 + field_rlc'` is done on access list storage key
This is incorrect - the degree of `r` to multiply should depend on the byte length of the next field. In the intuitive case, right after a access list address row is an access list storage key row. Therefore, `field_rlc'` would be an RLC of 32 bytes. This means that a formula of `section_rlc' = section_rlc * r^32 + field_rlc'` makes much more sense. In conclusion - whether or not to multiply `r^20` or `r^32` should depend on what type of row `field_rlc'` is an RLC of, not what type of row `field_rlc` is an RLC of.
```rust=
// current tag is AccessListStorageKey
cb.condition(
and::expr([
not::expr(is_final_cur.clone()),
meta.query_advice(is_access_list_storage_key, Rotation::cur())
]),
|cb| {
cb.require_equal(
"index = sks_acc",
meta.query_advice(sks_acc, Rotation::cur()),
meta.query_advice(tx_table.index, Rotation::cur()),
);
cb.require_equal(
"section_rlc accumulation: r = rand^32, section_rlc' = section_rlc * r + field_rlc'",
meta.query_advice(section_rlc, Rotation::next()),
meta.query_advice(section_rlc, Rotation::cur()) * r32
+ meta.query_advice(field_rlc, Rotation::next()),
);
});
```
There's also an interesting observation about how RLC behaves like a abi-encodePacked. The RLC is over the entire set of access list address and storage keys, so for example if there's a access list of two address and three storage keys like
- byte 1 to 20 address 1
- byte 21 to 52 storage 1
- byte 53 to 84 storage 2
- byte 85 to 104 address 2
- byte 105 to 136 storage 3
One can actually work around this like
- byte 1 to 20 address 1
- byte 21 to 52 storage 1
- byte 53 to 72 address 2
- byte 73 to 104 storage 2
- byte 105 to 136 storage 3
which is a different set of access list but have same RLC, address len, and same storage key length. Of course, if the actual addresses and keys are looked up to the RLP table correctly, this may not be an issue.
## 8. `tx_table.tag` cannot be a fixed column
Previously, there was a "fixed part" and a "dynamic part", and the dynamic part was all calldata. Therefore, for the `tx_table.tag` part, it was possible to make the column fixed (as the fixed part clearly had fixed tags and the dynamic part was all calldata anyway). However, now we have added access list address and storage keys to the dynamic part - so `tx_table.tag` is now actually dynamic.
in general, you need to handle how calldata -> accesslist address -> accesslist storage key transitions (also note that for a storage key to exist, the address must exist, and right after the address, non-zero number of storage key must exist - this should be all handled with a advice column)
## 9. `tx_id_gt_prev_cnt` is never actually handled
The chip is configured, but no constraints on `lt` was made.
## 10. `tx_nonce` should also be constrained to be equal over the fixed part of a single tx
```rust=
// if tag_next != Nonce, then tx_id' = tx_id, tx_type' = tx_type
cb.condition(
not::expr(tag_bits.value_equals(Nonce, Rotation::next())(meta)),
|cb| {
cb.require_equal(
"tx_id does not change",
meta.query_advice(tx_table.tx_id, Rotation::next()),
meta.query_advice(tx_table.tx_id, Rotation::cur()),
);
// tx meta infos that extracted at some row and need to be copied to all rows of
// same tx
let tx_meta_info_fields = vec![
("tx_type", tx_type), // extracted at SigV row
("is_padding_tx", is_padding_tx), // extracted at CallerAddress row
("sv_address", sv_address), // extracted at ChainID row
("block_num", block_num), // extracted at BlockNum row
("total_l1_popped_before", total_l1_popped_before),
("num_txs", num_txs),
("cum_num_txs", cum_num_txs),
("num_all_txs_acc", num_all_txs_acc),
// is_l1_msg does not need to spread out as it's extracted from tx_type
// these do not need to spread out as they are related to tx_table.tag
// (which is fixed col) is_chain_id,
// is_caller_address, is_tag_block_num, is_calldata
];
for (col_name, meta_info) in tx_meta_info_fields {
cb.require_equal(
col_name,
meta.query_advice(meta_info, Rotation::next()),
meta.query_advice(meta_info, Rotation::cur()),
);
}
},
);
```
Here, `tx_nonce` should be in `tx_meta_info_fields`. It can be seen that in the `num_all_txs_acc` constraints, the `tx_nonce` column gets queried in the `tag = BlockNum` row. (search for `##### HERE #####`)
```rust=
meta.create_gate("num_all_txs in a block", |meta| {
// ....
// last tx in cur block (next tx is the first tx in next block)
// and cur block is not the last block (s.t. we can init next block's num_all_txs)
cb.condition(
and::expr([
// We need this condition because if this is the last tx of fixed part of tx
// table, not(block_num_unchanged.expr()) is very likely to
// be true. Since it does not make sense to assign values
// to `num_all_txs` col in the calldata part of tx table.
// Therefore we can skip assign any values to fixed part related cols
// (e.g. block_num, tx_type, is_padding_tx, ....). The witness assignment of
// calldata part need only make sure that (is_final,
// calldata_gas_cost_acc) are correctly assigned.
not::expr(meta.query_advice(is_calldata, Rotation::next())),
not::expr(block_num_unchanged.expr()),
]),
|cb| {
cb.require_equal(
"total_l1_popped' = tx.is_l1_msg ? queue_index + 1 : total_l1_popped",
meta.query_advice(total_l1_popped_before, Rotation::next()),
select::expr(
meta.query_advice(is_l1_msg, Rotation::cur()),
meta.query_advice(queue_index, Rotation::cur()) + 1.expr(), // ##### HERE #####
meta.query_advice(total_l1_popped_before, Rotation::cur()),
),
);
// init new block's num_all_txs
// num_all_txs_acc' = is_l1_msg' ? queue_index' - total_l1_popped_before' + 1 :
// 1
cb.require_equal(
"init new block's num_all_txs",
meta.query_advice(num_all_txs_acc, Rotation::next()),
select::expr(
meta.query_advice(is_l1_msg, Rotation::next()),
meta.query_advice(tx_nonce, Rotation::next())
- meta.query_advice(total_l1_popped_before, Rotation::next())
+ 1.expr(),
1.expr(),
),
);
},
);
// no constraints on last tx in the fixed part of tx table
cb.gate(and::expr([
meta.query_fixed(tx_table.q_enable, Rotation::cur()),
// we are in the fixed part of tx table
not::expr(meta.query_advice(is_calldata, Rotation::cur())),
// calculate num_all_txs at tag = BlockNum row
meta.query_advice(is_tag_block_num, Rotation::cur()),
]))
});
```
## 11. Question on `is_padding_tx`
Should `is_padding_tx` true force `is_padding_tx'` to be true?
(also should `tx_id == 0` force `tx_id' == 0`)
## 12. First transaction having no calldata but access list is not handled properly
It seems that with the `tx call data init` constraints with the fixed column `q_calldata_first`, we are kinda assuming that the first transaction in the dynamic part has nonzero calldata length.
## 13. End condition for dynamic access list where the next transaction has no calldata doesn't force `section_rlc' = field_rlc'`
```rust=
// End conditions for the dynamic access list section, if the dynamic calldata section for the next tx doesn't exist
// For a regular access list section that immediately follows a calldata section, the init idx
// conditions are defined using the tail location of calldata (in the previous constraint block)
cb.condition(
and::expr([
is_final_cur,
not::expr(tx_id_is_zero.expr(Rotation::next())(meta)),
meta.query_advice(is_access_list, Rotation::next()),
]),
|cb| {
cb.require_equal(
"al_idx starts with 1",
meta.query_advice(al_idx, Rotation::next()),
1.expr()
);
cb.require_zero(
"sks_acc starts with 0",
meta.query_advice(sks_acc, Rotation::next()),
);
}
);
```
I believe this should include `section_rlc' = field_rlc'` as well.
## 14. There's no transition condition for dynamic access list -> dynamic calldata
```rust=
cb.condition(
and::expr([
is_final_cur.clone(),
not::expr(meta.query_advice(is_tx_id_zero, Rotation::next())),
meta.query_advice(is_calldata, Rotation::next()),
]),
|cb| {
let value_next_is_zero = value_is_zero.expr(Rotation::next())(meta);
let gas_cost_next = select::expr(value_next_is_zero, 4.expr(), 16.expr());
cb.require_equal(
"index' == 0",
meta.query_advice(tx_table.index, Rotation::next()),
0.expr(),
);
cb.require_equal(
"calldata_gas_cost_acc' == gas_cost_next",
meta.query_advice(calldata_gas_cost_acc, Rotation::next()),
gas_cost_next,
);
cb.require_equal(
"section_rlc' == byte'",
meta.query_advice(section_rlc, Rotation::next()),
meta.query_advice(tx_table.value, Rotation::next()),
);
},
);
```
This "type" of constraints, which force the next row (which is the first row of the next transaction's calldata) to start correctly, is not added for the case where we are in a `is_final` row for a dynamic access list.
Note that the above code (line 1411) is enforced with a condition
```rust=
cb.gate(and::expr(vec![
meta.query_fixed(q_enable, Rotation::cur()),
meta.query_advice(is_calldata, Rotation::cur()),
not::expr(meta.query_advice(is_tx_id_zero, Rotation::cur())),
]))
```
which means that this is applied only when the current row is a calldata. Therefore, this part of the code doesn't handle the dynamic access list to dynamic calldata transition.
## 15. Initialization for `num_all_txs_acc` and relevant columns is not done
```rust=
// first tx in tx table
cb.condition(meta.query_fixed(q_first, Rotation::next()), |cb| {
cb.require_equal(
"num_all_txs_acc = is_l1_msg ? queue_index - total_l1_popped_before + 1 : 1",
meta.query_advice(num_all_txs_acc, Rotation::cur()),
select::expr(
meta.query_advice(is_l1_msg, Rotation::cur()),
// first tx is l1 msg
meta.query_advice(queue_index, Rotation::cur())
- meta.query_advice(total_l1_popped_before, Rotation::cur())
+ 1.expr(),
1.expr(),
),
);
});
```
This queries `q_first` on the next column - and the gate is over
```rust=
cb.gate(and::expr([
meta.query_fixed(tx_table.q_enable, Rotation::cur()),
// we are in the fixed part of tx table
not::expr(meta.query_advice(is_calldata, Rotation::cur())),
// calculate num_all_txs at tag = BlockNum row
meta.query_advice(is_tag_block_num, Rotation::cur()),
]))
```
So this constraint is actually never enforced, as there is no case where `q_first' = 1`.
## 16. Question about dynamic rows.
if `is_final_cur` is false and we are on `is_calldata`, we should force `is_calldata' = true` (I don't think this is added in?)
Also feel like one should enforce either `is_calldata` or `is_access_list` on all dynamic rows.
## 17. Missing checks on ecdsa signature v for eip1559
The `v \in \{0, 1\}` (sign) check for EIP1559 and EIP2930 are not added.
(tx circuit, line 1666)
```
// TODO:
// 4. eip1559 tx: v Є {0, 1}
// 5. eip2930 tx: v Є {0, 1}
```
## 18. Ensure tx_id is the same over access list section in tx circuit.
There's also no check that `tx_id` remains the same over the access list section when `is_final` is false. (Also, that `tx_id` changes in the access list section when `is_final` is true.)
## 19. Overconstrained tx_id_unchanged conditions
```rust=
cb.condition(
and::expr([
is_final_cur.expr(),
meta.query_advice(is_access_list, Rotation::next()),
]),
|cb| {
cb.require_zero(
"tx_id stays the same for access list section at is_final == 1",
tx_id_unchanged.is_equal_expression.clone() - 1.expr(),
);
cb.require_equal(
"al_idx starts with 1",
meta.query_advice(al_idx, Rotation::next()),
1.expr(),
);
cb.require_zero(
"sks_acc starts with 0",
meta.query_advice(sks_acc, Rotation::next()),
);
cb.require_equal(
"section_rlc::cur == field_rlc::cur",
meta.query_advice(section_rlc, Rotation::next()),
meta.query_advice(field_rlc, Rotation::next()),
);
},
);
```
What would happen if tx 1 have non-zero calldata but no access list, and tx 2 have no calldata but non-zero access list? Then wouldn't we have tx 2's access list right after tx 1's calldata? This would violate the `tx_id_unchanged` condition here.
## 20. depth shouldn't change in DecodeTagStart -> LongList
```rust=
// DecodeTagStart => LongList
meta.create_gate("state transition: DecodeTagStart => LongList", |meta| {
let mut cb = BaseConstraintBuilder::default();
let (bv_gt_0xf8, bv_eq_0xf8) = byte_value_gte_0xf8.expr(meta, None);
let cond = and::expr([
sum::expr([bv_gt_0xf8, bv_eq_0xf8]),
not::expr(is_tag_end_expr(meta)),
]);
cb.condition(cond.expr(), |cb| {
// assertions.
do_not_emit!(meta, cb);
constrain_eq!(meta, cb, is_tag_begin, true);
// state transitions
update_state!(meta, cb, tag_length, byte_value_expr(meta) - 0xf7.expr());
update_state!(meta, cb, tag_idx, 1);
update_state!(meta, cb, tag_value_acc, byte_value_next_expr(meta));
update_state!(meta, cb, state, State::LongList);
constrain_unchanged_fields!(meta, cb; rlp_table.tx_id, rlp_table.format, tag, tag_next);
});
cb.gate(and::expr([
meta.query_fixed(q_enabled, Rotation::cur()),
is_decode_tag_start(meta),
]))
});
```
Here, `depth` should be in `constrain_unchanged_fields`.
## 21. booleans for reducing degree part 4, 5 don't constrain the booleans out of `is_decode_tag_start` or `is_tag_end`
```rust=
meta.create_gate("booleans for reducing degree (part four)", |meta| {
let mut cb = BaseConstraintBuilder::default();
cb.require_equal(
"is_new_access_list_address",
meta.query_advice(is_new_access_list_address, Rotation::cur()),
is_access_list_address(meta),
);
cb.require_equal(
"is_new_access_list_storage_key",
meta.query_advice(is_new_access_list_storage_key, Rotation::cur()),
is_access_list_storage_key(meta),
);
cb.require_boolean(
"is_new_access_list_address is boolean",
meta.query_advice(is_new_access_list_address, Rotation::cur()),
);
cb.require_boolean(
"is_new_access_list_storage_key is boolean",
meta.query_advice(is_new_access_list_storage_key, Rotation::cur()),
);
cb.gate(and::expr([
meta.query_fixed(q_enabled, Rotation::cur()),
is_decode_tag_start(meta),
]))
});
meta.create_gate("boolean for reducing degree (part five)", |meta| {
let mut cb = BaseConstraintBuilder::default();
cb.require_equal(
"is_access_list_end",
meta.query_advice(is_access_list_end, Rotation::cur()),
depth_eq_two.is_equal_expression.expr(),
);
cb.require_equal(
"is_storage_key_list_end",
meta.query_advice(is_storage_key_list_end, Rotation::cur()),
depth_eq_four.is_equal_expression.expr(),
);
cb.require_boolean(
"is_access_list_end is boolean",
meta.query_advice(is_access_list_end, Rotation::cur()),
);
cb.require_boolean(
"is_storage_key_list_end is boolean",
meta.query_advice(is_storage_key_list_end, Rotation::cur()),
);
cb.gate(and::expr([
meta.query_fixed(q_enabled, Rotation::cur()),
is_tag_end_vector(meta),
]))
});
```
Right now, `is_new_access_list_address` is constrained to be `Tag == AccessListAddress` only when `is_decode_tag_start` is true.
I believe it should be that `is_new_access_list_address` is true iff `Tag == AccessListAddress` AND `state = DecodeTagStart`. This goes for all constraints inside the boolean for reducing degree parts 4 and 5. Also note that `is_access_list_address(meta)` is already boolean, so the `require_boolean` checks in this section are redundant.
basically it's something like `c * (a - b) == 0` vs `a == b * c` - both have same degree so..
## 22. Initial values for `access_list_idx` and `storage_key_idx` is never constrained to be zero
There's no constraint for the very first `access_list_idx` and `storage_key_idx` to be equal to zero. The constraints right now are
- if `is_new_access_list`, it increments 1
- if `is_new_access_list` is false and `is_access_list_end` is false, remains the same
- if `is_access_list_end` is true, goes back to 0
which means there's no constraint for the very first row's `is_new_access_list`.
## 23. Correlation between rlp_decoding_table and RLP Circuit
The stack operations in the RLP circuit are filled up in the rlp_decoding_table during witness generation.
Are there any constraints to ensure that these stack operations correspond to valid rows in the RLP circuit, and are not just stack operations for some random tx?
This seems to difficult to do currently as the stack operations are stored in a sorted order different from the byte_idx order for the RLP circuit.
## 24. RLP Lookup for PUSH and POP is not sufficient.
```rust
meta.lookup_any(
"Each stack POP must correspond exactly to a PUSH onto a higher depth",
|meta| {
let enable = and::expr([
meta.query_fixed(q_enabled, Rotation::cur()),
meta.query_advice(rlp_decoding_table.is_stack_pop, Rotation::cur()),
]);
let input_exprs = vec![
meta.query_advice(rlp_decoding_table.tx_id, Rotation::cur()),
meta.query_advice(rlp_decoding_table.format, Rotation::cur()),
meta.query_advice(rlp_decoding_table.depth, Rotation::cur()) + 1.expr(),
1.expr(), // is_push = true
meta.query_advice(rlp_decoding_table.value_prev, Rotation::cur())
- meta.query_advice(rlp_decoding_table.value, Rotation::cur())
- 1.expr(),
];
let table_exprs = vec![
meta.query_advice(rlp_decoding_table.tx_id, Rotation::cur()),
meta.query_advice(rlp_decoding_table.format, Rotation::cur()),
meta.query_advice(rlp_decoding_table.depth, Rotation::cur()),
meta.query_advice(rlp_decoding_table.is_stack_push, Rotation::cur()),
meta.query_advice(rlp_decoding_table.value, Rotation::cur()),
];
input_exprs
.into_iter()
.zip(table_exprs)
.map(|(input, table)| (input * enable.expr(), table))
.collect()
},
);
```
The above lookup checks to ensure that every POP corresponds to a another PUSH at a higher depth. However, this doesn't ensure the uniqueness property as there can be multiple POPs that correspond to the same PUSH and vice-versa.
## 25. More rigourous constraints to ensure that stack_ops in RlpDecodingTable are sorted.
The witness assignment in `tx.rs` sorts the generated stack operations before assignment them to the `rlp_decoding_table`. It would be ideal if there were some direct constraints to ensure that the values in the table are sorted properly according to how they were assigned in the witness generation:
```rust=
// Sort the RlpStackOps and assign to the RlpDecodingTable part of witness
stack_ops.sort_by(|a, b| {
if (
a.tx_id,
a.format as u64,
a.depth,
a.al_idx,
a.sk_idx,
a.byte_idx,
a.stack_op.clone() as u64,
) > (
b.tx_id,
b.format as u64,
b.depth,
a.al_idx,
a.sk_idx,
b.byte_idx,
b.stack_op.clone() as u64,
) {
std::cmp::Ordering::Greater
} else {
std::cmp::Ordering::Less
}
});
```