From a7b4188c938390b13d069f40d941507ca79faca3 Mon Sep 17 00:00:00 2001 From: Omar Kalouti Date: Fri, 21 Aug 2020 19:07:17 +0700 Subject: [PATCH] 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 --- .../lib/omg_watcher/block_validator.ex | 77 +++++++++- .../test/omg_watcher/block_validator_test.exs | 135 ++++++++++++++++-- .../swagger/security_critical_api_specs.yaml | 6 +- .../block/paths.yaml | 8 +- 4 files changed, 207 insertions(+), 19 deletions(-) diff --git a/apps/omg_watcher/lib/omg_watcher/block_validator.ex b/apps/omg_watcher/lib/omg_watcher/block_validator.ex index 8aae938ef1..0c9fd43661 100644 --- a/apps/omg_watcher/lib/omg_watcher/block_validator.ex +++ b/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 diff --git a/apps/omg_watcher/test/omg_watcher/block_validator_test.exs b/apps/omg_watcher/test/omg_watcher/block_validator_test.exs index baa367f656..c8d0e0f446 100644 --- a/apps/omg_watcher/test/omg_watcher/block_validator_test.exs +++ b/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,18 +72,14 @@ 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] } @@ -86,7 +87,7 @@ defmodule OMG.WatcherRPC.Web.Validator.BlockValidatorTest do 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 diff --git a/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs.yaml b/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs.yaml index 2e6b50b966..1fc23921bd 100644 --- a/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs.yaml +++ b/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.' diff --git a/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs/block/paths.yaml b/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs/block/paths.yaml index 25b25a76b9..f8e16ba1de 100644 --- a/apps/omg_watcher_rpc/priv/swagger/security_critical_api_specs/block/paths.yaml +++ b/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'