fix: update row estimation with scroll-prover v0.7.2 (#475)

* Fix row estimation. * Update libzkp. * more * prepare * finish * upgrade * bump version * fix tests * Update to scroll-prover `v0.7.2`. * fix tests * Update miner/worker.go Co-authored-by: Péter Garamvölgyi <[email protected]> * Update miner/worker.go Co-authored-by: Péter Garamvölgyi <[email protected]> * Reset ccc when skips first tx. * do not unnecessarily skip L1 message * fix ccc reset and improve code readability * seal block on circuitcapacitychecker.ErrUnknown --------- Co-authored-by: HAOYUatHZ <[email protected]> Co-authored-by: Péter Garamvölgyi <[email protected]>
scroll-tech · Aug 23, 2023 · 6992cfe · 6992cfe
1 parent 727f88b
commit 6992cfe
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 79 deletions.
diff --git a/miner/worker.go b/miner/worker.go
@@ -1068,40 +1068,38 @@ loop:
 			log.Trace("Skipping unsupported transaction type", "sender", from, "type", tx.Type())
 			txs.Pop()
 
+		// Circuit capacity check
 		case errors.Is(err, circuitcapacitychecker.ErrBlockRowConsumptionOverflow):
-			// Circuit capacity check: circuit capacity limit reached in a block,
-			// don't pop or shift, just quit the loop immediately;
-			// though it might still be possible to add some "smaller" txs,
-			// but it's a trade-off between tracing overhead & block usage rate
-			log.Trace("Circuit capacity limit reached in a block", "acc_rows", w.current.accRows, "tx", tx.Hash().String())
-			circuitCapacityReached = true
-			break loop
-
-		case (errors.Is(err, circuitcapacitychecker.ErrTxRowConsumptionOverflow) && tx.IsL1MessageTx()):
-			// Circuit capacity check: L1MessageTx row consumption too high, shift to the next from the account,
-			// because we shouldn't skip the entire txs from the same account.
-			// This is also useful for skipping "problematic" L1MessageTxs.
-			queueIndex := tx.AsL1MessageTx().QueueIndex
-			log.Trace("Circuit capacity limit reached for a single tx", "tx", tx.Hash().String(), "queueIndex", queueIndex)
-			log.Info("Skipping L1 message", "queueIndex", queueIndex, "tx", tx.Hash().String(), "block", w.current.header.Number, "reason", "row consumption overflow")
-			w.current.nextL1MsgIndex = queueIndex + 1
-
-			// after `ErrTxRowConsumptionOverflow`, ccc might not revert updates
-			// associated with this transaction so we cannot pack more transactions.
-			// TODO: fix this in ccc and change these lines back to `txs.Shift()`
-			circuitCapacityReached = true
-			break loop
-
-		case (errors.Is(err, circuitcapacitychecker.ErrTxRowConsumptionOverflow) && !tx.IsL1MessageTx()):
-			// Circuit capacity check: L2MessageTx row consumption too high, skip the account.
-			// This is also useful for skipping "problematic" L2MessageTxs.
-			log.Trace("Circuit capacity limit reached for a single tx", "tx", tx.Hash().String())
+			if w.current.tcount >= 1 {
+				// 1. Circuit capacity limit reached in a block, and it's not the first tx:
+				// don't pop or shift, just quit the loop immediately;
+				// though it might still be possible to add some "smaller" txs,
+				// but it's a trade-off between tracing overhead & block usage rate
+				log.Trace("Circuit capacity limit reached in a block", "acc_rows", w.current.accRows, "tx", tx.Hash().String())
+				circuitCapacityReached = true
+				break loop
+			} else {
+				// 2. Circuit capacity limit reached in a block, and it's the first tx: skip the tx
+				log.Trace("Circuit capacity limit reached for a single tx", "tx", tx.Hash().String())
+
+				if tx.IsL1MessageTx() {
+					// Skip L1 message transaction,
+					// shift to the next from the account because we shouldn't skip the entire txs from the same account
+					txs.Shift()
+
+					queueIndex := tx.AsL1MessageTx().QueueIndex
+					log.Info("Skipping L1 message", "queueIndex", queueIndex, "tx", tx.Hash().String(), "block", w.current.header.Number, "reason", "row consumption overflow")
+					w.current.nextL1MsgIndex = queueIndex + 1
+				} else {
+					// Skip L2 transaction and all other transactions from the same sender account
+					txs.Pop()
+				}
 
-			// after `ErrTxRowConsumptionOverflow`, ccc might not revert updates
-			// associated with this transaction so we cannot pack more transactions.
-			// TODO: fix this in ccc and change these lines back to `txs.Pop()`
-			circuitCapacityReached = true
-			break loop
+				// Reset ccc so that we can process other transactions for this block
+				w.circuitCapacityChecker.Reset()
+				log.Trace("Worker reset ccc", "id", w.circuitCapacityChecker.ID)
+				circuitCapacityReached = false
+			}
 
 		case (errors.Is(err, circuitcapacitychecker.ErrUnknown) && tx.IsL1MessageTx()):
 			// Circuit capacity check: unknown circuit capacity checker error for L1MessageTx,
@@ -1111,19 +1109,19 @@ loop:
 			log.Info("Skipping L1 message", "queueIndex", queueIndex, "tx", tx.Hash().String(), "block", w.current.header.Number, "reason", "unknown row consumption error")
 			w.current.nextL1MsgIndex = queueIndex + 1
 
-			// after `ErrUnknown`, ccc might not revert updates associated
-			// with this transaction so we cannot pack more transactions.
-			// TODO: fix this in ccc and change these lines back to `txs.Shift()`
+			// Normally we would do `txs.Shift()` here.
+			// However, after `ErrUnknown`, ccc might remain in an
+			// inconsistent state, so we cannot pack more transactions.
 			circuitCapacityReached = true
 			break loop
 
 		case (errors.Is(err, circuitcapacitychecker.ErrUnknown) && !tx.IsL1MessageTx()):
 			// Circuit capacity check: unknown circuit capacity checker error for L2MessageTx, skip the account
 			log.Trace("Unknown circuit capacity checker error for L2MessageTx", "tx", tx.Hash().String())
 
-			// after `ErrUnknown`, ccc might not revert updates associated
-			// with this transaction so we cannot pack more transactions.
-			// TODO: fix this in ccc and change these lines back to `txs.Pop()`
+			// Normally we would do `txs.Pop()` here.
+			// However, after `ErrUnknown`, ccc might remain in an
+			// inconsistent state, so we cannot pack more transactions.
 			circuitCapacityReached = true
 			break loop
 

diff --git a/miner/worker_test.go b/miner/worker_test.go
@@ -1010,10 +1010,10 @@ func TestOversizedTxThenNormal(t *testing.T) {
 		switch blockNum {
 		case 0:
 			// schedule to skip 2nd call to ccc
-			w.getCCC().ScheduleError(2, circuitcapacitychecker.ErrTxRowConsumptionOverflow)
+			w.getCCC().ScheduleError(2, circuitcapacitychecker.ErrBlockRowConsumptionOverflow)
 			return false
 		case 1:
-			// include #0, skip #1, then terminate
+			// include #0, fail on #1, then seal the block
 			assert.Equal(1, len(block.Transactions()))
 
 			assert.True(block.Transactions()[0].IsL1MessageTx())
@@ -1022,7 +1022,23 @@ func TestOversizedTxThenNormal(t *testing.T) {
 			// db is updated correctly
 			queueIndex := rawdb.ReadFirstQueueIndexNotInL2Block(db, block.Hash())
 			assert.NotNil(queueIndex)
-			assert.Equal(uint64(2), *queueIndex)
+			assert.Equal(uint64(1), *queueIndex)
+
+			// schedule to skip next call to ccc
+			w.getCCC().ScheduleError(1, circuitcapacitychecker.ErrBlockRowConsumptionOverflow)
+
+			return false
+		case 2:
+			// skip #1, include #2, then seal the block
+			assert.Equal(1, len(block.Transactions()))
+
+			assert.True(block.Transactions()[0].IsL1MessageTx())
+			assert.Equal(uint64(2), block.Transactions()[0].AsL1MessageTx().QueueIndex)
+
+			// db is updated correctly
+			queueIndex := rawdb.ReadFirstQueueIndexNotInL2Block(db, block.Hash())
+			assert.NotNil(queueIndex)
+			assert.Equal(uint64(3), *queueIndex)
 
 			return true
 		default:

diff --git a/params/version.go b/params/version.go
@@ -24,7 +24,7 @@ import (
 const (
 	VersionMajor = 4         // Major version component of the current release
 	VersionMinor = 3         // Minor version component of the current release
-	VersionPatch = 47        // Patch version component of the current release
+	VersionPatch = 48        // Patch version component of the current release
 	VersionMeta  = "sepolia" // Version metadata to append to the version string
 )
 

diff --git a/rollup/circuitcapacitychecker/impl.go b/rollup/circuitcapacitychecker/impl.go
@@ -87,17 +87,13 @@ func (ccc *CircuitCapacityChecker) ApplyTransaction(traces *types.BlockTrace) (*
 		log.Error("fail to apply_tx in CircuitCapacityChecker", "id", ccc.ID, "TxHash", traces.Transactions[0].TxHash, "err", result.Error)
 		return nil, ErrUnknown
 	}
-	if result.TxRowUsage == nil || result.AccRowUsage == nil {
+	if result.AccRowUsage == nil {
 		log.Error("fail to apply_tx in CircuitCapacityChecker",
 			"id", ccc.ID, "TxHash", traces.Transactions[0].TxHash,
-			"result.TxRowUsage==nil", result.TxRowUsage == nil,
 			"result.AccRowUsage == nil", result.AccRowUsage == nil,
-			"err", "TxRowUsage or AccRowUsage is empty unexpectedly")
+			"err", "AccRowUsage is empty unexpectedly")
 		return nil, ErrUnknown
 	}
-	if !result.TxRowUsage.IsOk {
-		return nil, ErrTxRowConsumptionOverflow
-	}
 	if !result.AccRowUsage.IsOk {
 		return nil, ErrBlockRowConsumptionOverflow
 	}

diff --git a/rollup/circuitcapacitychecker/libzkp/Cargo.lock b/rollup/circuitcapacitychecker/libzkp/Cargo.lock
diff --git a/rollup/circuitcapacitychecker/libzkp/Cargo.toml b/rollup/circuitcapacitychecker/libzkp/Cargo.toml
@@ -20,8 +20,8 @@ maingate = { git = "https://github.com/scroll-tech/halo2wrong", branch = "halo2-
 halo2curves = { git = "https://github.com/scroll-tech/halo2curves.git", branch = "0.3.1-derive-serde" }
 
 [dependencies]
-prover = { git = "https://github.com/scroll-tech/scroll-prover", tag = "v0.6.4" }
-types = { git = "https://github.com/scroll-tech/scroll-prover", tag = "v0.6.4" }
+prover = { git = "https://github.com/scroll-tech/scroll-prover", tag = "v0.7.2" }
+types = { git = "https://github.com/scroll-tech/scroll-prover", tag = "v0.7.2" }
 
 log = "0.4"
 env_logger = "0.9.0"

diff --git a/rollup/circuitcapacitychecker/libzkp/src/lib.rs b/rollup/circuitcapacitychecker/libzkp/src/lib.rs
@@ -14,7 +14,6 @@ pub mod checker {
     #[derive(Debug, Clone, Deserialize, Serialize)]
     pub struct RowUsageResult {
         pub acc_row_usage: Option<RowUsage>,
-        pub tx_row_usage: Option<RowUsage>,
         pub error: Option<String>,
     }
 
@@ -59,7 +58,11 @@ pub mod checker {
     #[no_mangle]
     pub unsafe extern "C" fn apply_tx(id: u64, tx_traces: *const c_char) -> *const c_char {
         let result = panic::catch_unwind(|| {
-            log::debug!("ccc apply_tx raw input, id: {:?}, tx_traces: {:?}", id, c_char_to_str(tx_traces));
+            log::debug!(
+                "ccc apply_tx raw input, id: {:?}, tx_traces: {:?}",
+                id,
+                c_char_to_str(tx_traces)
+            );
             let tx_traces_vec = c_char_to_vec(tx_traces);
             let traces = serde_json::from_slice::<BlockTrace>(&tx_traces_vec)
                 .unwrap_or_else(|_| panic!("id: {id:?}, fail to deserialize tx_traces"));
@@ -88,22 +91,19 @@ pub mod checker {
                 })
         });
         let r = match result {
-            Ok((acc_row_usage, tx_row_usage)) => {
+            Ok(acc_row_usage) => {
                 log::debug!(
-                    "id: {:?}, acc_row_usage: {:?}, tx_row_usage: {:?}",
+                    "id: {:?}, acc_row_usage: {:?}",
                     id,
                     acc_row_usage.row_number,
-                    tx_row_usage.row_number
                 );
                 RowUsageResult {
                     acc_row_usage: Some(acc_row_usage),
-                    tx_row_usage: Some(tx_row_usage),
                     error: None,
                 }
             }
             Err(e) => RowUsageResult {
                 acc_row_usage: None,
-                tx_row_usage: None,
                 error: Some(format!("{e:?}")),
             },
         };
@@ -114,7 +114,11 @@ pub mod checker {
     #[no_mangle]
     pub unsafe extern "C" fn apply_block(id: u64, block_trace: *const c_char) -> *const c_char {
         let result = panic::catch_unwind(|| {
-            log::debug!("ccc apply_block raw input, id: {:?}, block_trace: {:?}", id, c_char_to_str(block_trace));
+            log::debug!(
+                "ccc apply_block raw input, id: {:?}, block_trace: {:?}",
+                id,
+                c_char_to_str(block_trace)
+            );
             let block_trace = c_char_to_vec(block_trace);
             let traces = serde_json::from_slice::<BlockTrace>(&block_trace)
                 .unwrap_or_else(|_| panic!("id: {id:?}, fail to deserialize block_trace"));
@@ -136,22 +140,19 @@ pub mod checker {
                 })
         });
         let r = match result {
-            Ok((acc_row_usage, tx_row_usage)) => {
+            Ok(acc_row_usage) => {
                 log::debug!(
-                    "id: {:?}, acc_row_usage: {:?}, tx_row_usage: {:?}",
+                    "id: {:?}, acc_row_usage: {:?}",
                     id,
                     acc_row_usage.row_number,
-                    tx_row_usage.row_number
                 );
                 RowUsageResult {
                     acc_row_usage: Some(acc_row_usage),
-                    tx_row_usage: Some(tx_row_usage),
                     error: None,
                 }
             }
             Err(e) => RowUsageResult {
                 acc_row_usage: None,
-                tx_row_usage: None,
                 error: Some(format!("{e:?}")),
             },
         };

diff --git a/rollup/circuitcapacitychecker/types.go b/rollup/circuitcapacitychecker/types.go
@@ -8,12 +8,10 @@ import (
 
 var (
 	ErrUnknown                     = errors.New("unknown circuit capacity checker error")
-	ErrTxRowConsumptionOverflow    = errors.New("tx row consumption oveflow")
 	ErrBlockRowConsumptionOverflow = errors.New("block row consumption oveflow")
 )
 
 type WrappedRowUsage struct {
 	AccRowUsage *types.RowUsage `json:"acc_row_usage,omitempty"`
-	TxRowUsage  *types.RowUsage `json:"tx_row_usage,omitempty"`
 	Error       string          `json:"error,omitempty"`
 }