Merge pull request #41 from worldcoin/0xkitsune/tx-validation

Update Validation to enforce nullifier uniqueness

world-chain-builder/src/node/test_utils.rs

@@ -601,7 +601,7 @@ where
     ) -> TransactionValidationOutcome<Self::Transaction> {
         if let Some(pbh_paylaod) = transaction.pbh_payload() {
             self.inner
-                .set_validated(&transaction, pbh_paylaod)
+                .set_validated(pbh_paylaod)
                 .expect("Error when writing to the db");
         }
 

world-chain-builder/src/payload/builder.rs

@@ -420,10 +420,6 @@ where
         executed_txs.push(sequencer_tx.into_signed());
     }
 
-    let db_tx = pbh_db
-        .tx_mut()
-        .map_err(|err| PayloadBuilderError::Internal(err.into()))?;
-
     if !attributes.no_tx_pool {
         let verified_gas_limit = (verified_blockspace_capacity as u64 * block_gas_limit) / 100;
         while let Some(pool_tx) = best_txs.next() {
@@ -491,26 +487,6 @@ where
                 }
             };
 
-            // add the nullifier to the db
-            if let Some(pbh_payload) = pool_tx.transaction.pbh_payload() {
-                match set_pbh_nullifier(&db_tx, pbh_payload.nullifier_hash) {
-                    Err(DatabaseError::Write(write))
-                        if write.operation == DatabaseWriteOperation::CursorInsert =>
-                    {
-                        // If the nullifier has already been seen then we can skip this transaction
-                        best_txs.mark_invalid(&pool_tx);
-                        warn!(target: "payload_builder",
-                            parent_hash=%parent_block.hash(),
-                            nullifier=%pbh_payload.nullifier_hash,
-                            "nullifier already seen, skipping transaction"
-                        );
-                        continue;
-                    }
-                    Err(e) => return Err(PayloadBuilderError::Internal(e.into())),
-                    _ => (),
-                }
-            }
-
             // drop evm so db is released.
             drop(evm);
             // commit changes
@@ -665,11 +641,6 @@ where
     let sealed_block = block.seal_slow();
     debug!(target: "payload_builder", ?sealed_block, "sealed built block");
 
-    // commit the pbh nullifiers to the db
-    db_tx
-        .commit()
-        .map_err(|err| PayloadBuilderError::Internal(err.into()))?;
-
     // create the executed block data
     let executed = ExecutedBlock {
         block: Arc::new(sealed_block.clone()),

world-chain-builder/src/pbh/db.rs

@@ -16,21 +16,6 @@ use tracing::info;
 use reth_db::cursor::DbCursorRW;
 use reth_db::transaction::DbTxMut;
 
-/// Table for executed nullifiers.
-///
-/// This table stores the nullifiers of PBH transactions that have been
-/// included into a block after it has been sealed.
-#[derive(Debug, Clone, Default)]
-pub struct ExecutedPbhNullifierTable;
-
-impl Table for ExecutedPbhNullifierTable {
-    const NAME: &'static str = "ExecutedPbhNullifiers";
-
-    type Key = B256;
-
-    type Value = EmptyValue;
-}
-
 /// Table to store PBH validated transactions along with their nullifiers.
 ///
 /// When a trasnaction is validated before being inserted into the pool,
@@ -43,9 +28,9 @@ pub struct ValidatedPbhTransactionTable;
 impl Table for ValidatedPbhTransactionTable {
     const NAME: &'static str = "ValidatedPbhTransactions";
 
-    type Key = TxHash;
+    type Key = B256;
 
-    type Value = B256;
+    type Value = EmptyValue;
 }
 
 #[derive(Debug, Clone, Default, Serialize, Deserialize)]
@@ -68,7 +53,7 @@ impl Compress for EmptyValue {
 /// don't forget to call db_tx.commit() at the very end
 pub fn set_pbh_nullifier(db_tx: &Tx<RW>, nullifier: Field) -> Result<(), DatabaseError> {
     let bytes: FixedBytes<32> = nullifier.into();
-    let mut cursor = db_tx.cursor_write::<ExecutedPbhNullifierTable>()?;
+    let mut cursor = db_tx.cursor_write::<ValidatedPbhTransactionTable>()?;
     cursor.insert(bytes, EmptyValue)?;
     Ok(())
 }
@@ -90,11 +75,6 @@ pub fn load_world_chain_db(
         .begin_rw_txn()
         .map_err(|e| DatabaseError::InitTx(e.into()))?;
 
-    tx.create_db(
-        Some(ExecutedPbhNullifierTable::NAME),
-        DatabaseFlags::default(),
-    )
-    .map_err(|e| DatabaseError::CreateTable(e.into()))?;
     tx.create_db(
         Some(ValidatedPbhTransactionTable::NAME),
         DatabaseFlags::default(),

world-chain-builder/src/pool/validator.rs

@@ -19,7 +19,7 @@ use super::ordering::WorldChainOrdering;
 use super::root::WorldChainRootValidator;
 use super::tx::{WorldChainPoolTransaction, WorldChainPooledTransaction};
 use crate::pbh::date_marker::DateMarker;
-use crate::pbh::db::{ExecutedPbhNullifierTable, ValidatedPbhTransactionTable};
+use crate::pbh::db::{EmptyValue, ValidatedPbhTransactionTable};
 use crate::pbh::external_nullifier::ExternalNullifier;
 use crate::pbh::payload::{PbhPayload, TREE_DEPTH};
 
@@ -69,10 +69,10 @@ where
         &self.inner
     }
 
-    pub fn set_validated(&self, tx: &Tx, pbh_payload: &PbhPayload) -> Result<(), DatabaseError> {
+    pub fn set_validated(&self, pbh_payload: &PbhPayload) -> Result<(), DatabaseError> {
         let db_tx = self.pbh_db.tx_mut()?;
         let mut cursor = db_tx.cursor_write::<ValidatedPbhTransactionTable>()?;
-        cursor.insert(*tx.hash(), pbh_payload.nullifier_hash.to_be_bytes().into())?;
+        cursor.insert(pbh_payload.nullifier_hash.to_be_bytes().into(), EmptyValue)?;
         db_tx.commit()?;
         Ok(())
     }
@@ -126,41 +126,20 @@ where
         Ok(())
     }
 
-    pub fn validate_nullifier(
-        &self,
-        pbh_payload: &PbhPayload,
-    ) -> Result<(), TransactionValidationError> {
-        let tx = self.pbh_db.tx()?;
-        match tx.get::<ExecutedPbhNullifierTable>(pbh_payload.nullifier_hash.to_be_bytes().into()) {
-            Ok(Some(_)) => Err(WorldChainTransactionPoolInvalid::NullifierAlreadyExists.into()),
-            Ok(None) => Ok(()),
-            Err(e) => Err(TransactionValidationError::Error(
-                format!("Error while fetching nullifier from database: {}", e).into(),
-            )),
-        }
-    }
-
     pub fn validate_pbh_payload(
         &self,
         transaction: &Tx,
         payload: &PbhPayload,
     ) -> Result<(), TransactionValidationError> {
         // Create db transaction and insert the nullifier hash
         // We do this first to prevent repeatedly validating the same transaction
-        //
-        // This should prevent DOS attacks for tranasctions with the same hash
-        // However i'm not sure there's anything we can do for transactions with different hashes
         let db_tx = self.pbh_db.tx_mut()?;
         let mut cursor = db_tx.cursor_write::<ValidatedPbhTransactionTable>()?;
-        cursor.insert(
-            *transaction.hash(),
-            payload.nullifier_hash.to_be_bytes().into(),
-        )?;
+        cursor.insert(payload.nullifier_hash.to_be_bytes().into(), EmptyValue)?;
 
         let date = chrono::Utc::now();
         self.validate_root(payload)?;
         self.validate_external_nullifier(date, payload)?;
-        self.validate_nullifier(payload)?;
 
         let res = verify_proof(
             payload.root,
@@ -187,13 +166,15 @@ where
         origin: TransactionOrigin,
         transaction: Tx,
     ) -> TransactionValidationOutcome<Tx> {
+        let validation_outcome = self.inner.validate_one(origin, transaction.clone());
+
         if let Some(pbh_payload) = transaction.pbh_payload() {
             if let Err(e) = self.validate_pbh_payload(&transaction, pbh_payload) {
                 return e.to_outcome(transaction);
             }
         };
 
-        self.inner.validate_one(origin, transaction.clone())
+        validation_outcome
     }
 
     /// Validates all given transactions.
@@ -504,9 +485,6 @@ pub mod tests {
             proof,
         };
 
-        let mut tx = get_non_pbh_transaction();
-        tx.pbh_payload = Some(payload.clone());
-
-        validator.set_validated(&tx, &payload).unwrap();
+        validator.set_validated(&payload).unwrap();
     }
 }