From c36f2f2fc458d90a7616290eb170b098987dc77f Mon Sep 17 00:00:00 2001
From: ian <[email protected]>
Date: Wed, 27 Mar 2024 03:27:15 +0000
Subject: [PATCH] rgbpp-lock review by ian

---
 contracts/rgbpp-lock/src/main.rs | 59 ++++++++++++++++++++++++++++++++
 tests/src/tests.rs               | 23 +++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/contracts/rgbpp-lock/src/main.rs b/contracts/rgbpp-lock/src/main.rs
index 08d1d48..2b0cb7f 100644
--- a/contracts/rgbpp-lock/src/main.rs
+++ b/contracts/rgbpp-lock/src/main.rs
@@ -45,18 +45,49 @@ pub fn program_entry() -> i8 {
 
 fn main() -> Result<(), Error> {
     // parse config and witness
+    // ℹ️ Informing: RGBPP lock args
+    // struct RGBPPLock {
+    //   out_index: Uint32,
+    //   btc_txid: Byte32,
+    // }
     let lock_args = {
         let rgbpp_lock = load_script()?;
         RGBPPLock::from_slice(&rgbpp_lock.args().raw_data()).map_err(|_| Error::BadRGBPPLock)?
     };
     let ckb_tx = load_transaction()?;
+    // ℹ️ Informing: RGBPP config
+    // struct RGBPPConfig {
+    //   version: Uint16,
+    //   # Type hash of bitcoin light client
+    //   btc_lc_type_hash: Byte32,
+    //   # Type hash of bitcoin time lock contract
+    //   btc_time_lock_type_hash: Byte32,
+    // }
     let config = load_config::<RGBPPConfig>(&ckb_tx)?;
+    // ℹ️ Informing: RGBPP unlock witness
+    // table RGBPPUnlock {
+    //   version: Uint16,
+    //   extra_data: ExtraCommitmentData,
+    //   btc_tx: Bytes,
+    //   btc_tx_proof: Bytes,
+    // }
     let unlock_witness = fetch_unlock_from_witness()?;
 
     // parse bitcoin transaction
     let raw_btc_tx = unlock_witness.btc_tx().raw_data();
     let btc_tx: BTCTx = parse_btc_tx(&raw_btc_tx)?;
 
+    // 💭 Discussing: Current implementation allows unlocking all cells locked by the same BTC
+    // UTXO. Is this the intended behavior?
+
+    // ℹ️ Informing: In case of multiple input rgbpp cells bound to different BTC UTXOs, they
+    // must be associated with the same BTC transaction. This is because the commitment
+    // calculation is based on the entire CKB transaction, not individual script groups.
+    // Consequently, while the witnesses remain identical, they must be reiterated for each
+    // script group. The verification logic for these script groups is largely identical as
+    // well. A possible optimization is to run the verification only once for all rgbpp input
+    // cells, for example, running only when the script group contains the first input rgbpp
+    // cell in the tx.
     verify_unlock(&config, &lock_args, &unlock_witness, &btc_tx, &ckb_tx)?;
     verify_outputs(&config, &btc_tx)?;
     Ok(())
@@ -67,12 +98,16 @@ fn verify_outputs(config: &RGBPPConfig, btc_tx: &BTCTx) -> Result<(), Error> {
     let rgbpp_lock = load_script()?;
     for (index, type_hash) in QueryIter::new(load_cell_type_hash, Source::Output).enumerate() {
         // ignore non-type cells
+        // 💭 Discussing: The plain CKB cells may be locked by rgblock with any BTC UTXO,
+        // or btc time lock with any BTC tx. Is this the anticipated effect?
         if type_hash.is_none() {
             continue;
         }
 
         let lock = load_cell_lock(index, Source::Output)?;
         // check RGB++ lock
+        // 💭 Discussing: The output cells may have the same bound BTC UTXO.
+        // Is this the expected outcome?
         if is_script_code_equal(&lock, &rgbpp_lock) {
             // check new seal txid + index is valid
             let lock_args =
@@ -118,6 +153,30 @@ fn verify_unlock(
     ckb_tx: &Transaction,
 ) -> Result<(), Error> {
     // check bitcoin transaction inputs unlock RGB++ cell
+    // ℹ️ Informing: A possible attacking scenario:
+    // - Alice has CKB cell CellA bound to BTC UTXO CoinA.
+    // - Bob has CKB cell CellB bound to BTC UTXO CoinB.
+    //
+    // Alice create a BTC transaction
+    //
+    // alice_btc_tx:
+    //   inputs:
+    //   - CoinA
+    //   outputs:
+    //   - OP_RETURN commitment(rgbpp_tx, input_len=2, output_len=1)
+    //   - CoinC
+    //
+    // alice_rgbpp_tx:
+    //   inputs:
+    //   - CellA(args=CoinA)
+    //   - CellB(args=CoinB)
+    //   outputs:
+    //   - CellC(args=CoinC)
+    //   witnesses:
+    //   - lock: alice_btc_tx
+    //   - lock: alice_btc_tx
+    //
+    // The lock of CellB can pass all the verifications except the following one.
     let expected_out_point: (Byte32, u32) = (lock_args.btc_txid(), lock_args.out_index().unpack());
     let is_found = btc_tx
         .inputs
diff --git a/tests/src/tests.rs b/tests/src/tests.rs
index 64ac178..9d6dc96 100644
--- a/tests/src/tests.rs
+++ b/tests/src/tests.rs
@@ -248,6 +248,29 @@ fn test_rgbpp_unlock() {
     let new_seal_out_index = 1;
 
     // prepare a rgbpp tx
+    // ℹ️ Informing:
+    //
+    // rgbpp_tx:
+    //   inputs:
+    //     - lock: rgbpp_lock(args=btcutxo1.txid||btcutxo1.index)
+    //       type: sudt
+    //       data: 1000
+    //   outputs:
+    //     - lock: rgbpp_lock(args=btcutxo2.txid||btcutxo2.index)
+    //       type: sudt
+    //       data: 300
+    //     - lock: rgbpp_lock(args=btcutxo2.txid||btcutxo2.index)
+    //       type: sudt
+    //       data: 700
+    //   witnesses:
+    //     - lock: {version, input_len, output_len, btc_tx, btc_tx_proof}
+    //
+    // btc_tx:
+    //   inputs:
+    //     - out_point: btcutxo1
+    //   outputs:
+    //     - OP_RETURN commitment(rgbpp_tx)
+    //     - btcutxo2
     let rgbpp_tx = {
         let tx_builder = Transaction::default().as_advanced_builder();
 
-- 
2.43.2