diff --git a/CHANGELOG.md b/CHANGELOG.md index fd8f283a85..42ec586e8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ - [#2173](https://github.com/poanetwork/blockscout/pull/2173) - handle correctly empty transactions - [#2174](https://github.com/poanetwork/blockscout/pull/2174) - fix reward channel joining - [#2186](https://github.com/poanetwork/blockscout/pull/2186) - fix net version test +- [#2198](https://github.com/poanetwork/blockscout/pull/2198) - reduce transaction status and error constraint - [#2167](https://github.com/poanetwork/blockscout/pull/2167) - feat: document eth rpc api mimicking endpoints - [#2225](https://github.com/poanetwork/blockscout/pull/2225) - fix metadata decoding in Solidity 0.5.9 smart contract verification - [#2204](https://github.com/poanetwork/blockscout/pull/2204) - fix large contract verification diff --git a/apps/explorer/lib/explorer/chain/transaction.ex b/apps/explorer/lib/explorer/chain/transaction.ex index 19f04dee2d..3b109504eb 100644 --- a/apps/explorer/lib/explorer/chain/transaction.ex +++ b/apps/explorer/lib/explorer/chain/transaction.ex @@ -247,7 +247,7 @@ defmodule Explorer.Chain.Transaction do end @doc """ - A pending transaction has neither `block_hash` nor an `index` + A pending transaction does not have a `block_hash` iex> changeset = Explorer.Chain.Transaction.changeset( ...> %Transaction{}, @@ -267,42 +267,6 @@ defmodule Explorer.Chain.Transaction do iex> changeset.valid? true - A pending transaction (which is indicated by not having a `block_hash`) can't have `block_number`, - `cumulative_gas_used`, `gas_used`, or `index`. - - iex> changeset = Explorer.Chain.Transaction.changeset( - ...> %Transaction{}, - ...> %{ - ...> from_address_hash: "0xe8ddc5c7a2d2f0d7a9798459c0104fdf5e987aca", - ...> block_number: 34, - ...> cumulative_gas_used: 0, - ...> gas: 4700000, - ...> gas_price: 100000000000, - ...> gas_used: 4600000, - ...> hash: "0x3a3eb134e6792ce9403ea4188e5e79693de9e4c94e499db132be086400da79e6", - ...> index: 0, - ...> input: "0x6060604052341561000f57600080fd5b336000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055506102db8061005e6000396000f300606060405260043610610062576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff1680630900f01014610067578063445df0ac146100a05780638da5cb5b146100c9578063fdacd5761461011e575b600080fd5b341561007257600080fd5b61009e600480803573ffffffffffffffffffffffffffffffffffffffff16906020019091905050610141565b005b34156100ab57600080fd5b6100b3610224565b6040518082815260200191505060405180910390f35b34156100d457600080fd5b6100dc61022a565b604051808273ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16815260200191505060405180910390f35b341561012957600080fd5b61013f600480803590602001909190505061024f565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff163373ffffffffffffffffffffffffffffffffffffffff161415610220578190508073ffffffffffffffffffffffffffffffffffffffff1663fdacd5766001546040518263ffffffff167c010000000000000000000000000000000000000000000000000000000002815260040180828152602001915050600060405180830381600087803b151561020b57600080fd5b6102c65a03f1151561021c57600080fd5b5050505b5050565b60015481565b6000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1681565b6000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff163373ffffffffffffffffffffffffffffffffffffffff1614156102ac57806001819055505b505600a165627a7a72305820a9c628775efbfbc17477a472413c01ee9b33881f550c59d21bee9928835c854b0029", - ...> nonce: 0, - ...> r: 0xAD3733DF250C87556335FFE46C23E34DBAFFDE93097EF92F52C88632A40F0C75, - ...> s: 0x72caddc0371451a58de2ca6ab64e0f586ccdb9465ff54e1c82564940e89291e3, - ...> status: :ok, - ...> v: 0x8d, - ...> value: 0 - ...> } - ...> ) - iex> changeset.valid? - false - iex> Keyword.get_values(changeset.errors, :block_number) - [{"can't be set when the transaction is pending", []}] - iex> Keyword.get_values(changeset.errors, :cumulative_gas_used) - [{"can't be set when the transaction is pending", []}] - iex> Keyword.get_values(changeset.errors, :gas_used) - [{"can't be set when the transaction is pending", []}] - iex> Keyword.get_values(changeset.errors, :index) - [{"can't be set when the transaction is pending", []}] - iex> Keyword.get_values(changeset.errors, :status) - [{"can't be set when the transaction is pending", []}] - A collated transaction MUST have an `index` so its position in the `block` is known and the `cumulative_gas_used` ane `gas_used` to know its fees. @@ -359,35 +323,6 @@ defmodule Explorer.Chain.Transaction do iex> changeset.valid? true - Once the `internal_transactions_indexed_at` is set, both pre- and post-Byzantium transactions will be able to know - their status, so if `internal_transaction_indexed_at` is set, `status` is required. - - iex> changeset = Explorer.Chain.Transaction.changeset( - ...> %Transaction{}, - ...> %{ - ...> block_hash: "0xe52d77084cab13a4e724162bcd8c6028e5ecfaa04d091ee476e96b9958ed6b47", - ...> block_number: 34, - ...> cumulative_gas_used: 0, - ...> from_address_hash: "0xe8ddc5c7a2d2f0d7a9798459c0104fdf5e987aca", - ...> gas: 4700000, - ...> gas_price: 100000000000, - ...> gas_used: 4600000, - ...> hash: "0x3a3eb134e6792ce9403ea4188e5e79693de9e4c94e499db132be086400da79e6", - ...> index: 0, - ...> input: "0x6060604052341561000f57600080fd5b336000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055506102db8061005e6000396000f300606060405260043610610062576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff1680630900f01014610067578063445df0ac146100a05780638da5cb5b146100c9578063fdacd5761461011e575b600080fd5b341561007257600080fd5b61009e600480803573ffffffffffffffffffffffffffffffffffffffff16906020019091905050610141565b005b34156100ab57600080fd5b6100b3610224565b6040518082815260200191505060405180910390f35b34156100d457600080fd5b6100dc61022a565b604051808273ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16815260200191505060405180910390f35b341561012957600080fd5b61013f600480803590602001909190505061024f565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff163373ffffffffffffffffffffffffffffffffffffffff161415610220578190508073ffffffffffffffffffffffffffffffffffffffff1663fdacd5766001546040518263ffffffff167c010000000000000000000000000000000000000000000000000000000002815260040180828152602001915050600060405180830381600087803b151561020b57600080fd5b6102c65a03f1151561021c57600080fd5b5050505b5050565b60015481565b6000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1681565b6000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff163373ffffffffffffffffffffffffffffffffffffffff1614156102ac57806001819055505b505600a165627a7a72305820a9c628775efbfbc17477a472413c01ee9b33881f550c59d21bee9928835c854b0029", - ...> internal_transactions_indexed_at: DateTime.utc_now(), - ...> nonce: 0, - ...> r: 0xAD3733DF250C87556335FFE46C23E34DBAFFDE93097EF92F52C88632A40F0C75, - ...> s: 0x72caddc0371451a58de2ca6ab64e0f586ccdb9465ff54e1c82564940e89291e3, - ...> v: 0x8d, - ...> value: 0 - ...> } - ...> ) - iex> changeset.valid? - false - iex> Keyword.get_values(changeset.errors, :status) - [{"can't be blank when the internal transactions have been fetched", []}] - The `error` can only be set with a specific error message when `status` is `:error` iex> changeset = Explorer.Chain.Transaction.changeset( @@ -445,10 +380,9 @@ defmodule Explorer.Chain.Transaction do transaction |> cast(attrs, @required_attrs ++ @optional_attrs) |> validate_required(@required_attrs) - |> validate_collated_or_pending() + |> validate_collated() |> validate_error() |> validate_status() - |> check_pending() |> check_collated() |> check_error() |> check_status() @@ -592,29 +526,17 @@ defmodule Explorer.Chain.Transaction do {collated_field, :"collated_#{collated_field}}"} end) - @pending_fields_with_check @collated_fields - @pending_fields_with_validation @collated_fields ++ ~w(internal_transaction_indexed_at status)a - @pending_message "can't be set when the transaction is pending" - @pending_field_to_check Enum.into(@pending_fields_with_check, %{}, fn pending_field -> - {pending_field, :"pending_#{pending_field}}"} - end) - defp check_collated(%Changeset{} = changeset) do check_constraints(changeset, @collated_field_to_check, @collated_message) end - defp check_pending(%Changeset{} = changeset) do - check_constraints(changeset, @pending_field_to_check, @pending_message) - end - @error_message "can't be set when status is not :error" defp check_error(%Changeset{} = changeset) do check_constraint(changeset, :error, message: @error_message, name: :error) - changeset end - @status_message "can't be blank when the internal transactions have been fetched" + @status_message "can't be set when the block_hash is unknown" defp check_status(%Changeset{} = changeset) do check_constraint(changeset, :status, message: @status_message, name: :status) @@ -632,22 +554,10 @@ defmodule Explorer.Chain.Transaction do end) end - defp validate_collated_or_pending(%Changeset{} = changeset) do + defp validate_collated(%Changeset{} = changeset) do case Changeset.get_field(changeset, :block_hash) do - nil -> validate_collated_or_pending(changeset, @pending_fields_with_validation, &validate_pending/2) - %Hash{} -> validate_collated_or_pending(changeset, @collated_fields, &validate_collated/2) - end - end - - defp validate_collated_or_pending(%Changeset{} = changeset, fields, field_validator) - when is_list(fields) and is_function(field_validator, 2) do - Enum.reduce(fields, changeset, field_validator) - end - - defp validate_pending(field, %Changeset{} = changeset) when is_atom(field) do - case Changeset.get_field(changeset, field) do + %Hash{} -> Enum.reduce(@collated_fields, changeset, &validate_collated/2) nil -> changeset - _ -> Changeset.add_error(changeset, field, @pending_message) end end @@ -667,9 +577,8 @@ defmodule Explorer.Chain.Transaction do end defp validate_status(%Changeset{} = changeset) do - # all other errors on status are handled by validate_pending - if Changeset.get_field(changeset, :internal_transactions_indexed_at) != nil and - Changeset.get_field(changeset, :status) == nil do + if Changeset.get_field(changeset, :block_hash) == nil and + Changeset.get_field(changeset, :status) != nil do Changeset.add_error(changeset, :status, @status_message) else changeset diff --git a/apps/explorer/priv/repo/migrations/20190619154943_reduce_transaction_status_constraint.exs b/apps/explorer/priv/repo/migrations/20190619154943_reduce_transaction_status_constraint.exs new file mode 100644 index 0000000000..4964bc4250 --- /dev/null +++ b/apps/explorer/priv/repo/migrations/20190619154943_reduce_transaction_status_constraint.exs @@ -0,0 +1,132 @@ +defmodule Explorer.Repo.Migrations.ReduceTransactionStatusConstraint do + use Ecto.Migration + + def up do + drop( + constraint( + :transactions, + :status + ) + ) + + create( + constraint( + :transactions, + :status, + # NOTE: all checks on status are lifted except those regarding block_hash + # This is because of block invalidation, that causes transactions to be + # refetched while previous internal transactions still exist + check: """ + (block_hash IS NULL AND status IS NULL) OR + (block_hash IS NOT NULL) OR + (status = 0 and error = 'dropped/replaced') + """ + ) + ) + + drop( + constraint( + :transactions, + :error + ) + ) + + create( + constraint( + :transactions, + :error, + # NOTE: all checks on error are lifted except when status is not 0, for + # the same reasons as above + check: """ + (status = 0) OR + (status != 0 AND error IS NULL) + """ + ) + ) + end + + def down do + drop( + constraint( + :transactions, + :status + ) + ) + + create( + constraint( + :transactions, + :status, + # 0 - NULL + # 1 - NOT NULL + # + # | block_hash | internal_transactions_indexed_at | status | OK | description + # |------------|----------------------------------|--------|----|------------ + # | 0 | 0 | 0 | 1 | pending + # | 0 | 0 | 1 | 0 | pending with status + # | 0 | 1 | 0 | 0 | pending with internal transactions + # | 0 | 1 | 1 | 0 | pending with internal transactions and status + # | 1 | 0 | 0 | 1 | pre-byzantium collated transaction without internal transactions + # | 1 | 0 | 1 | 1 | post-byzantium collated transaction without internal transactions + # | 1 | 1 | 0 | 0 | pre-byzantium collated transaction with internal transaction without status + # | 1 | 1 | 1 | 1 | pre- or post-byzantium collated transaction with internal transactions and status + # + # [Karnaugh map](https://en.wikipedia.org/wiki/Karnaugh_map) + # b \ is | 00 | 01 | 11 | 10 | + # -------|----|----|----|----| + # 0 | 1 | 0 | 0 | 0 | + # 1 | 1 | 1 | 1 | 0 | + # + # Simplification: ¬i·¬s + b·¬i + b·s + check: """ + (internal_transactions_indexed_at IS NULL AND status IS NULL) OR + (block_hash IS NOT NULL AND internal_transactions_indexed_at IS NULL) OR + (block_hash IS NOT NULL AND status IS NOT NULL) OR + (status = 0 and error = 'dropped/replaced') + """ + ) + ) + + drop( + constraint( + :transactions, + :error + ) + ) + + create( + constraint( + :transactions, + :error, + # | status | internal_transactions_indexed_at | error | OK | description + # |--------|----------------------------------|----------|------------|------------ + # | NULL | NULL | NULL | TRUE | pending or pre-byzantium collated + # | NULL | NULL | NOT NULL | FALSE | error cannot be known before internal transactions are indexed + # | NULL | NOT NULL | NULL | DON'T CARE | handled by `status` check + # | NULL | NOT NULL | NOT NULL | FALSE | error cannot be set unless status is known to be error (`0`) + # | 0 | NULL | NULL | TRUE | post-byzantium before internal transactions indexed + # | 0 | NULL | NOT NULL | FALSE | error cannot be set unless internal transactions are indexed + # | 0 | NOT NULL | NULL | FALSE | error MUST be set when status is error + # | 0 | NOT NULL | NOT NULL | TRUE | error is set when status is error + # | 1 | NULL | NULL | TRUE | post-byzantium before internal transactions indexed + # | 1 | NULL | NOT NULL | FALSE | error cannot be set when status is ok + # | 1 | NOT NULL | NULL | TRUE | error is not set when status is ok + # | 1 | NOT NULL | NOT NULL | FALSE | error cannot be set when status is ok + # + # Karnaugh map + # s \ ie | NULL, NULL | NULL, NOT NULL | NOT NULL, NOT NULL | NOT NULL, NULL | + # -------|------------|----------------|--------------------|----------------| + # NULL | TRUE | FALSE | FALSE | DON'T CARE | + # 0 | TRUE | FALSE | TRUE | FALSE | + # 1 | TRUE | FALSE | FALSE | TRUE | + # + check: """ + (internal_transactions_indexed_at IS NULL AND error IS NULL) OR + (status = 0 AND internal_transactions_indexed_at IS NOT NULL AND error IS NOT NULL) OR + (status != 0 AND internal_transactions_indexed_at IS NOT NULL AND error IS NULL) OR + (status = 0 and error = 'dropped/replaced') + """ + ) + ) + end +end diff --git a/apps/explorer/test/explorer/chain/import_test.exs b/apps/explorer/test/explorer/chain/import_test.exs index 5bff700dcb..dd3bbf1aec 100644 --- a/apps/explorer/test/explorer/chain/import_test.exs +++ b/apps/explorer/test/explorer/chain/import_test.exs @@ -573,7 +573,7 @@ defmodule Explorer.Chain.ImportTest do transaction = :transaction - |> insert(error: nil, internal_transactions_indexed_at: nil, status: nil, from_address: from_address, status: 0) + |> insert(error: nil, internal_transactions_indexed_at: nil, status: nil, from_address: from_address) |> with_block(block, status: :error) internal_transacton =