Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: handle "transaction underpriced" and other unknown server error responses #1617

Merged
merged 6 commits into from
Jul 8, 2020
Merged

fix: handle "transaction underpriced" and other unknown server error responses #1617

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
09080da
fix: handle tx underpriced response
Jul 1, 2020
adda76a
feat: add handling for unknown server errors
Jul 1, 2020
a47c784
style: mix formatting
Jul 1, 2020
2afe0d1
style: mix credo
Jul 1, 2020
d7b5d12
feat: log block submission response as info
Jul 2, 2020
4ae8fb1
Merge branch 'master' into unnawut/block-queue-log
unnawut Jul 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions apps/omg_child_chain/lib/omg_child_chain/block_queue/core.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,17 @@ defmodule OMG.ChildChain.BlockQueue.Core do
:ok
end

# https://github.com/ethereum/go-ethereum/blob/v1.9.15/core/tx_pool.go#L62-L64
def process_submit_result(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure why do we even bother "handling" results. We don't do anything with them, we're just spending effort defining all function headers and then we ... log and return :ok?
I've never understood the purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same. It turns about the block queue uses these :ok, {:error, ...} returns to determine if the submission was successful or if it needs to retry the submission :(

I don't like it too but it's not a small refactor. I'm kind of unbundling this in #1575 though

submission,
{:error, %{"code" => -32_000, "message" => "transaction underpriced"}},
_newest_mined_blknum
) do
log_tx_underpriced(submission)
:ok
end

# https://github.com/ethereum/go-ethereum/blob/v1.9.15/core/tx_pool.go#L66-L68
def process_submit_result(
submission,
{:error, %{"code" => -32_000, "message" => "replacement transaction underpriced"}},
Expand Down Expand Up @@ -340,6 +351,15 @@ defmodule OMG.ChildChain.BlockQueue.Core do
:ok
end

# Fallback for unknown server errors: https://eth.wiki/json-rpc/json-rpc-error-codes-improvement-proposal
# Only server errors are handled as they are the only set of errors that we have no control of.
# Returns `:ok` so that BlockQueue can continue and do a retry, similar to a low replacement price error.
def process_submit_result(submission, {:error, %{"code" => code} = error}, _newest_mined_blknum)
when code >= -32_099 and code <= -32_000 do
_ = Logger.error("Submission #{inspect(submission)} resulted in an unknown server error: #{inspect(error)}.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we alarm on all error log. If not let's make sure we add alarm on this, either via Datadog parsing the error message or do the direct alarm (?) from code?

Copy link
Contributor Author

@unnawut unnawut Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I think we can do with an app alarm when a block submission is pending for more than 2 blocks (since we're aiming for next-block inclusion, but I'll make number configurable in case of too many false panics). Then Datadog can consume this app alarm similar to :ethereum_stalled_sync and friends.

Since we're losing this visibility because this PR is suppressing the exception, I'll add :block_submission_stalled alarm to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're losing this visibility because this PR is suppressing the exception, I'll add :block_submission_stalled alarm to this PR.

Queue observability size. Hell yeah!

:ok
end

#
# Private functions
#
Expand Down Expand Up @@ -612,12 +632,17 @@ defmodule OMG.ChildChain.BlockQueue.Core do
end

defp log_known_tx(submission) do
_ = Logger.debug("Submission #{inspect(submission)} is known transaction - ignored")
_ = Logger.info("Submission #{inspect(submission)} is known transaction - ignored")
:ok
end

defp log_tx_underpriced(submission) do
_ = Logger.info("Submission #{inspect(submission)} got rejected due to low gas price - ignored")
:ok
end

defp log_low_replacement_price(submission) do
_ = Logger.debug("Submission #{inspect(submission)} is known, but with higher price - ignored")
_ = Logger.info("Submission #{inspect(submission)} is known, but with higher price - ignored")
:ok
end

Expand Down
23 changes: 23 additions & 0 deletions apps/omg_child_chain/test/omg_child_chain/block_queue/core_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ defmodule OMG.ChildChain.BlockQueue.CoreTest do

# responses from geth to simulate what we're getting from geth in `BlockQueue`
@known_transaction_response {:error, %{"code" => -32_000, "message" => "known transaction tx"}}
@transaction_underpriced_response {:error, %{"code" => -32_000, "message" => "transaction underpriced"}}
@replacement_transaction_response {:error, %{"code" => -32_000, "message" => "replacement transaction underpriced"}}
@nonce_too_low_response {:error, %{"code" => -32_000, "message" => "nonce too low"}}
@account_locked_response {:error, %{"code" => -32_000, "message" => "authentication needed: password or unlock"}}
Expand Down Expand Up @@ -712,6 +713,8 @@ defmodule OMG.ChildChain.BlockQueue.CoreTest do
# no change in mined blknum
assert :ok = Core.process_submit_result(submission, @known_transaction_response, 1000)

assert :ok = Core.process_submit_result(submission, @transaction_underpriced_response, 1000)

assert :ok = Core.process_submit_result(submission, @replacement_transaction_response, 1000)
end

Expand All @@ -738,5 +741,25 @@ defmodule OMG.ChildChain.BlockQueue.CoreTest do
assert {:error, :account_locked} = Core.process_submit_result(submission, @account_locked_response, 0)
end) =~ "[error]"
end

test "logs unknown server error response", %{submission: submission} do
assert capture_log(fn ->
assert :ok =
Core.process_submit_result(
submission,
{:error, %{"code" => -32_000, "message" => "foo error"}},
1000
)
end) =~ "unknown server error: %{\"code\" => -32000, \"message\" => \"foo error\"}"

assert capture_log(fn ->
assert :ok =
Core.process_submit_result(
submission,
{:error, %{"code" => -32_070, "message" => "bar error"}},
2000
)
end) =~ "unknown server error: %{\"code\" => -32070, \"message\" => \"bar error\"}"
end
end
end