fix tag byte overlap issue #520 (#523)

* fix tag byte overlap issue

* fix clippy

* Update constraints and bit packing for 5 bit field tags

* Update comments

* minor update for comment

minor update for comment

Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

Co-authored-by: z2trillion <mason@scroll.tech>
Co-authored-by: z2trillion <z2trillion@users.noreply.github.com>
Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

Author: 4 people

zkevm-circuits/src/state_circuit/lexicographic_ordering.rs

@@ -60,8 +60,8 @@ use std::ops::Mul;
 
 // Packing the field into 480 bits:
 //   4 bits for tag,
-// + 4 bits for field_tag // TODO: this actually needs 5 bits. Either reduce id
-// + 24 bits for id       // to 23 bits, or add diff_3 etc.
+// + 5 bits for field_tag
+// + 23 bits for id
 // + 160 bits for address,
 // + 256 bits for storage key
 // + 32  bits for rw_counter
@@ -294,7 +294,7 @@ impl<F: Field> Queries<F> {
     }
 
     fn packed_tags(&self) -> Expression<F> {
-        (1u64 << 4).expr() * self.tag.clone() + self.field_tag.clone()
+        (1u64 << 5).expr() * self.tag.clone() + self.field_tag.clone()
     }
 
     fn storage_key_be_limbs(&self) -> Vec<Expression<F>> {
@@ -316,25 +316,27 @@ impl<F: Field> Queries<F> {
             .chain(self.rw_counter_limbs.iter().rev())
             .cloned()
             .collect();
-        // most significant byte of id should be 0, so safe to overwrite it with packed
-        // tags.
-        limbs[0] = limbs[0].clone() + self.packed_tags() * (1u64 << 8).expr();
+        // The packed tags are shifted left by 7 bits so that they occupy the most
+        // significant 9 bits of the first 16-bit limb.
+        limbs[0] = limbs[0].clone() + self.packed_tags() * (1u64 << 7).expr();
         limbs
     }
 }
 
 fn rw_to_be_limbs(row: &Rw) -> Vec<u16> {
+    let mut id = row.id().unwrap_or_default() as u32;
+    assert_eq!(id.to_be_bytes().len(), 4);
+    // The max value of `id` is 2^23 - 1, so the 9 most significant bits should be
+    // 0. We use these 9 bits to hold value of `tag` and `field_tag`.
+    assert!(id < (1 << 23));
+    id += (((row.tag() as u32) << 5) + (row.field_tag().unwrap_or_default() as u32)) << 23;
+
     let mut be_bytes = vec![];
-    be_bytes.extend_from_slice(&(row.id().unwrap_or_default() as u32).to_be_bytes());
+    be_bytes.extend_from_slice(&id.to_be_bytes());
     be_bytes.extend_from_slice(&(row.address().unwrap_or_default().0));
     be_bytes.extend_from_slice(&(row.storage_key().unwrap_or_default().to_be_bytes()));
     be_bytes.extend_from_slice(&((row.rw_counter() as u32).to_be_bytes()));
 
-    // check that the first byte of id is not used, and overwrites it with packed
-    // tags.
-    assert_eq!(be_bytes[0], 0);
-    be_bytes[0] = row.field_tag().unwrap_or_default() as u8 + ((row.tag() as u8) << 4);
-
     be_bytes
         .iter()
         .tuples()