Block Validation: New Checks (#1693)

* feat: transaction counting and block-level duplicate input verification in block validation

* fix: dialyzer error

* fix: credo error

* feat: validate fee transactions in block

* test: fee transaction validation

* revert fee logic meant to be stateful

* remove: fee claimer check

* fix: place dummy fee claimer

* test names

* refactors: simplify duplicate input verification, use guards, etc.

* PR review: description changes

* fix: credo

* simplify duplicate input checking

* update documentation

* fix: dialyzer

* spacing

* suggested edits

apps/omg_watcher/lib/omg_watcher/block_validator.ex

@@ -20,15 +20,25 @@ defmodule OMG.Watcher.BlockValidator do
   alias OMG.Block
   alias OMG.Merkle
   alias OMG.State.Transaction
+  alias OMG.Utxo.Position
+
+  @transaction_upper_limit 2 |> :math.pow(16) |> Kernel.trunc()
 
   @doc """
   Executes stateless validation of a submitted block:
-  - Verifies that transactions are correctly formed.
+  - Verifies that the number of transactions falls within the accepted range.
+  - Verifies that (payment and fee) transactions  are correctly formed.
+  - Verifies that fee transactions are correctly placed and unique per currency.
+  - Verifies that there are no duplicate inputs at the block level.
   - Verifies that given Merkle root matches reconstructed Merkle root.
+
   """
   @spec stateless_validate(Block.t()) :: {:ok, boolean()} | {:error, atom()}
   def stateless_validate(submitted_block) do
-    with {:ok, recovered_transactions} <- verify_transactions(submitted_block.transactions),
+    with :ok <- number_of_transactions_within_limit(submitted_block.transactions),
+         {:ok, recovered_transactions} <- verify_transactions(submitted_block.transactions),
+         {:ok, _fee_transactions} <- verify_fee_transactions(recovered_transactions),
+         {:ok, _inputs} <- verify_no_duplicate_inputs(recovered_transactions),
          {:ok, _block} <- verify_merkle_root(submitted_block, recovered_transactions) do
       {:ok, true}
     end
@@ -48,7 +58,7 @@ defmodule OMG.Watcher.BlockValidator do
     end
   end
 
-  @spec verify_transactions(transactions :: list(Transaction.Recovered.t())) ::
+  @spec verify_transactions(transactions :: list(Transaction.Signed.tx_bytes())) ::
           {:ok, list(Transaction.Recovered.t())}
           | {:error, Transaction.Recovered.recover_tx_error()}
   defp verify_transactions(transactions) do
@@ -64,4 +74,65 @@ defmodule OMG.Watcher.BlockValidator do
       end
     end)
   end
+
+  @spec number_of_transactions_within_limit([Transaction.Signed.tx_bytes()]) :: :ok | {:error, atom()}
+  defp number_of_transactions_within_limit([]), do: {:error, :empty_block}
+
+  defp number_of_transactions_within_limit(transactions) when length(transactions) > @transaction_upper_limit do
+    {:error, :transactions_exceed_block_limit}
+  end
+
+  defp number_of_transactions_within_limit(_transactions), do: :ok
+
+  @spec verify_no_duplicate_inputs([Transaction.Recovered.t()]) :: {:ok, [map()]} | {:error, :block_duplicate_inputs}
+  defp verify_no_duplicate_inputs(transactions) do
+    all_inputs = Enum.flat_map(transactions, &Transaction.get_inputs/1)
+    uniq_inputs = Enum.uniq_by(all_inputs, &Position.encode/1)
+
+    case length(all_inputs) == length(uniq_inputs) do
+      true -> {:ok, all_inputs}
+      false -> {:error, :block_duplicate_inputs}
+    end
+  end
+
+  @spec verify_fee_transactions([Transaction.Recovered.t()]) :: {:ok, [Transaction.Recovered.t()]} | {:error, atom()}
+  defp verify_fee_transactions(transactions) do
+    identified_fee_transactions = Enum.filter(transactions, &is_fee/1)
+
+    with :ok <- expected_index(transactions, identified_fee_transactions),
+         :ok <- unique_fee_transaction_per_currency(identified_fee_transactions) do
+      {:ok, identified_fee_transactions}
+    end
+  end
+
+  @spec expected_index([Transaction.Recovered.t()], [Transaction.Recovered.t()]) :: :ok | {:error, atom()}
+  defp expected_index(transactions, identified_fee_transactions) do
+    number_of_fee_txs = length(identified_fee_transactions)
+    tail = Enum.slice(transactions, -number_of_fee_txs, number_of_fee_txs)
+
+    case identified_fee_transactions do
+      ^tail -> :ok
+      _ -> {:error, :unexpected_transaction_type_at_fee_index}
+    end
+  end
+
+  @spec unique_fee_transaction_per_currency([Transaction.Recovered.t()]) :: :ok | {:error, atom()}
+  defp unique_fee_transaction_per_currency(identified_fee_transactions) do
+    identified_fee_transactions
+    |> Enum.uniq_by(fn fee_transaction -> fee_transaction |> get_fee_output() |> Map.get(:currency) end)
+    |> case do
+      ^identified_fee_transactions -> :ok
+      _ -> {:error, :duplicate_fee_transaction_for_ccy}
+    end
+  end
+
+  defp is_fee(%Transaction.Recovered{signed_tx: %Transaction.Signed{raw_tx: %Transaction.Fee{}}}) do
+    true
+  end
+
+  defp is_fee(_), do: false
+
+  defp get_fee_output(fee_transaction) do
+    fee_transaction |> Transaction.get_outputs() |> Enum.at(0)
+  end
 end

apps/omg_watcher/test/omg_watcher/block_validator_test.exs

@@ -23,11 +23,16 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
 
   @alice TestHelper.generate_entity()
   @bob TestHelper.generate_entity()
+
   @eth OMG.Eth.zero_address()
+
   @payment_tx_type OMG.WireFormatTypes.tx_type_for(:tx_payment_v1)
 
+  @fee_claimer <<27::160>>
+  @transaction_upper_limit 2 |> :math.pow(16) |> Kernel.trunc()
+
   describe "stateless_validate/1" do
-    test "returns error if a transaction is not correctly formed (e.g. duplicate inputs)" do
+    test "returns an error if a transaction within the block is not correctly formed (e.g. duplicate inputs in this test)" do
       input_1 = {1, 0, 0, @alice}
       input_2 = {2, 0, 0, @alice}
       input_3 = {3, 0, 0, @alice}
@@ -67,26 +72,22 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
 
     test "accepts correctly formed transactions" do
       recovered_tx_1 = TestHelper.create_recovered([{1, 0, 0, @alice}, {2, 0, 0, @alice}], @eth, [{@bob, 10}])
+
       recovered_tx_2 = TestHelper.create_recovered([{3, 0, 0, @alice}, {4, 0, 0, @alice}], @eth, [{@bob, 10}])
 
       signed_txbytes_1 = recovered_tx_1.signed_tx_bytes
       signed_txbytes_2 = recovered_tx_2.signed_tx_bytes
 
-      merkle_root =
-        [recovered_tx_1, recovered_tx_2]
-        |> Enum.map(&Transaction.raw_txbytes/1)
-        |> Merkle.hash()
-
       block = %{
-        hash: merkle_root,
+        hash: derive_merkle_root([recovered_tx_1, recovered_tx_2]),
         number: 1000,
         transactions: [signed_txbytes_1, signed_txbytes_2]
       }
 
       assert {:ok, true} == BlockValidator.stateless_validate(block)
     end
 
-    test "returns error for non-matching Merkle root hash" do
+    test "returns an error if the given hash does not match the reconstructed Merkle root hash" do
       recovered_tx_1 = TestHelper.create_recovered([{1, 0, 0, @alice}], @eth, [{@bob, 100}])
       recovered_tx_2 = TestHelper.create_recovered([{2, 0, 0, @alice}], @eth, [{@bob, 100}])
 
@@ -101,16 +102,13 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
       assert {:error, :invalid_merkle_root} == BlockValidator.stateless_validate(block)
     end
 
-    test "accepts matching Merkle root hash" do
+    test "accepts a matching Merkle root hash" do
       recovered_tx_1 = TestHelper.create_recovered([{1, 0, 0, @alice}], @eth, [{@bob, 100}])
       recovered_tx_2 = TestHelper.create_recovered([{2, 0, 0, @alice}], @eth, [{@bob, 100}])
 
       signed_txbytes = Enum.map([recovered_tx_1, recovered_tx_2], fn tx -> tx.signed_tx_bytes end)
 
-      valid_merkle_root =
-        [recovered_tx_1, recovered_tx_2]
-        |> Enum.map(&Transaction.raw_txbytes/1)
-        |> Merkle.hash()
+      valid_merkle_root = derive_merkle_root([recovered_tx_1, recovered_tx_2])
 
       block = %{
         hash: valid_merkle_root,
@@ -120,5 +118,116 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do
 
       assert {:ok, true} = BlockValidator.stateless_validate(block)
     end
+
+    test "rejects a block with no transactions or more transactions than the defined limit" do
+      oversize_block = %{
+        hash: "0x0",
+        number: 1000,
+        transactions: List.duplicate("0x0", @transaction_upper_limit + 1)
+      }
+
+      undersize_block = %{
+        hash: "0x0",
+        number: 1000,
+        transactions: []
+      }
+
+      assert {:error, :transactions_exceed_block_limit} = BlockValidator.stateless_validate(oversize_block)
+
+      assert {:error, :empty_block} = BlockValidator.stateless_validate(undersize_block)
+    end
+
+    test "rejects a block that uses the same input in different transactions" do
+      duplicate_input = {1, 0, 0, @alice}
+
+      recovered_tx_1 = TestHelper.create_recovered([duplicate_input], @eth, [{@bob, 10}])
+      recovered_tx_2 = TestHelper.create_recovered([duplicate_input], @eth, [{@bob, 10}])
+
+      signed_txbytes_1 = recovered_tx_1.signed_tx_bytes
+      signed_txbytes_2 = recovered_tx_2.signed_tx_bytes
+
+      block = %{
+        hash: derive_merkle_root([recovered_tx_1, recovered_tx_2]),
+        number: 1000,
+        transactions: [signed_txbytes_1, signed_txbytes_2]
+      }
+
+      assert {:error, :block_duplicate_inputs} == BlockValidator.stateless_validate(block)
+    end
+  end
+
+  describe "stateless_validate/1 (fee validation)" do
+    test "rejects a block if there are multiple fee transactions of the same currency" do
+      input_1 = {1, 0, 0, @alice}
+      input_2 = {2, 0, 0, @alice}
+
+      payment_tx_1 = TestHelper.create_recovered([input_1], @eth, [{@bob, 10}])
+      payment_tx_2 = TestHelper.create_recovered([input_2], @eth, [{@bob, 10}])
+      fee_tx_1 = TestHelper.create_recovered_fee_tx(1, @fee_claimer, @eth, 1)
+      fee_tx_2 = TestHelper.create_recovered_fee_tx(1, @fee_claimer, @eth, 1)
+
+      signed_txbytes = Enum.map([payment_tx_1, payment_tx_2, fee_tx_1, fee_tx_2], fn tx -> tx.signed_tx_bytes end)
+
+      block = %{
+        hash: derive_merkle_root([payment_tx_1, payment_tx_2, fee_tx_1, fee_tx_2]),
+        number: 1000,
+        transactions: signed_txbytes
+      }
+
+      assert {:error, :duplicate_fee_transaction_for_ccy} = BlockValidator.stateless_validate(block)
+    end
+
+    test "rejects a block if fee transactions are not at the tail of the transactions' list (one fee currency)" do
+      input_1 = {1, 0, 0, @alice}
+      input_2 = {2, 0, 0, @alice}
+
+      payment_tx_1 = TestHelper.create_recovered([input_1], @eth, [{@bob, 10}])
+      payment_tx_2 = TestHelper.create_recovered([input_2], @eth, [{@bob, 10}])
+      fee_tx = TestHelper.create_recovered_fee_tx(1, @fee_claimer, @eth, 5)
+
+      invalid_ordered_transactions = [payment_tx_1, fee_tx, payment_tx_2]
+      signed_txbytes = Enum.map(invalid_ordered_transactions, fn tx -> tx.signed_tx_bytes end)
+
+      block = %{
+        hash: derive_merkle_root(invalid_ordered_transactions),
+        number: 1000,
+        transactions: signed_txbytes
+      }
+
+      assert {:error, :unexpected_transaction_type_at_fee_index} = BlockValidator.stateless_validate(block)
+    end
+
+    test "rejects a block if fee transactions are not at the tail of the transactions' list (two fee currencies)" do
+      ccy_1 = @eth
+      ccy_2 = <<1::160>>
+
+      ccy_1_fee = 1
+      ccy_2_fee = 1
+
+      input_1 = {1, 0, 0, @alice}
+      input_2 = {2, 0, 0, @alice}
+
+      payment_tx_1 = TestHelper.create_recovered([input_1], ccy_1, [{@bob, 10}])
+      payment_tx_2 = TestHelper.create_recovered([input_2], ccy_2, [{@bob, 10}])
+
+      fee_tx_1 = TestHelper.create_recovered_fee_tx(1, @fee_claimer, ccy_1, ccy_1_fee)
+      fee_tx_2 = TestHelper.create_recovered_fee_tx(1, @fee_claimer, ccy_2, ccy_2_fee)
+
+      invalid_ordered_transactions = [payment_tx_1, fee_tx_1, payment_tx_2, fee_tx_2]
+      signed_txbytes = Enum.map(invalid_ordered_transactions, fn tx -> tx.signed_tx_bytes end)
+
+      block = %{
+        hash: derive_merkle_root(invalid_ordered_transactions),
+        number: 1000,
+        transactions: signed_txbytes
+      }
+
+      assert {:error, :unexpected_transaction_type_at_fee_index} = BlockValidator.stateless_validate(block)
+    end
+  end
+
+  @spec derive_merkle_root([Transaction.Recovered.t()]) :: binary()
+  defp(derive_merkle_root(transactions)) do
+    transactions |> Enum.map(&Transaction.raw_txbytes/1) |> Merkle.hash()
   end
 end

apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs.yaml

@@ -915,7 +915,11 @@ paths:
         - Block
       summary: Verifies the stateless validity of a block.
       description: |
-        Validates that the given transactions are correctly formed, and that the given hash corresponds to the reconstructed Merkle root hash.
+        - Verifies that given Merkle root matches reconstructed Merkle root.
+        - Verifies that (payment and fee) transactions  are correctly formed.
+        - Verifies that there are no duplicate inputs at the block level.
+        - Verifies that the number of transactions falls within the accepted range.
+        - Verifies that fee transactions are correctly placed and unique per currency.
       operationId: validate
       requestBody:
         description: 'Block object with a hash, number and array of hexadecimal transaction bytes.'

apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs/block/paths.yaml

@@ -3,8 +3,12 @@ block.validate:
     tags:
       - Block
     summary: Verifies the stateless validity of a block. 
-    description: > 
-      Validates that the given transactions are correctly formed, and that the given hash corresponds to the reconstructed Merkle root hash.
+    description: |
+      - Verifies that given Merkle root matches reconstructed Merkle root.
+      - Verifies that (payment and fee) transactions  are correctly formed.
+      - Verifies that there are no duplicate inputs at the block level.
+      - Verifies that the number of transactions falls within the accepted range.
+      - Verifies that fee transactions are correctly placed and unique per currency.
     operationId: validate
     requestBody:
       $ref: 'request_bodies.yaml#/BlockValidateBodySchema'