feat: poisson regression and gas price feature flag #1575
Conversation
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| defmodule OMG.ChildChain.BlockQueue.BlockSubmission do |
There was a problem hiding this comment.
This is an untouched extraction from OMG.ChildChain.BlockQueue.Core
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| defmodule OMG.ChildChain.GasPrice.LegacyGasStrategy do |
There was a problem hiding this comment.
A heavy refactor of the gas estimation formally OMG.ChildChain.BlockQueue.Core.adjust_gas_price() and OMG.ChildChain.BlockQueue.Core.adjust_gas_price(). All states in BlockQueue.Core that were used solely for gas price estimation also got moved here.
I'm expecting this to be 1:1 parity in behaviour with the current gas price estimation
apps/omg_child_chain/lib/omg_child_chain/block_queue/block_submission.ex
Outdated
Show resolved
Hide resolved
| @impl GenServer | ||
| def handle_continue(:start_recalculate, state) do | ||
| _ = send(self(), :recalculate) | ||
| {:ok, _} = :timer.send_interval(@recalculate_interval_ms, self(), :recalculate) |
There was a problem hiding this comment.
Was going to use the handle_info(:timeout, ... approach that @mederic-p shared with me, but this genserver will be accepting :get_price call regularly and so I think it will interfere with the timeout approach.
|
|
||
| # The ets table is not initialized with `:read_concurrency` because we are expecting interleaving | ||
| # reads and writes. See http://erlang.org/doc/man/ets.html | ||
| history_ets = :ets.new(@history_table, [:ordered_set, :protected, :named_table]) |
There was a problem hiding this comment.
[discussion/design]
When I was reading some let it crash related video, I saw a design recommendation that to isolate the owning process of ets, so the ets could still survive when the business logic handling process failed. In the case here, it is fetching data from network so potentially it could fail.
Okay, but I see you're already retrying there in Fetcher 😛 (I am hearing Ino coughing) so probably it would not bring too much benefit on that case. However, testing wise, it might still be very convenient to be able to inject a ets table.
There was a problem hiding this comment.
isolate the owning process of ets, so the ets could still survive when the business logic handling process failed
Hmm I did not realize that this process will take owner ship of this ets, and couldn't find any mention in the docs? Do you have a link to something I can read? If not I'll try experiment it through iex.
However, testing wise, it might still be very convenient to be able to inject a ets table.
Yeah you are right on this. Will update.
There was a problem hiding this comment.
probably this shows the ownership of the ets table: https://dev.to/strech/where-is-my-ets-table-3ff1 (from my random googling :p I am not 100% sure too actually after reading your comment)
There was a problem hiding this comment.
And you are right!
iex(5)> task = Task.async(fn ->
...(5)> table = :ets.new(:some_history_table, [:ordered_set, :protected, :named_table])
...(5)> IO.inspect("#{inspect(self())}: ets table created: #{inspect(table)}")
...(5)>
...(5)> record = {:foo, "bar"}
...(5)> true = :ets.insert(table, record)
...(5)> IO.inspect("#{inspect(self())}: inserted #{inspect(record)} to #{inspect(table)}")
...(5)> end)
"#PID<0.128.0>: ets table created: :some_history_table"
"#PID<0.128.0>: inserted {:foo, \"bar\"} to :some_history_table"
%Task{
owner: #PID<0.105.0>,
pid: #PID<0.128.0>,
ref: #Reference<0.575725472.2248409092.246859>
}
iex(6)>
nil
iex(7)> _ = Task.await(task)
"#PID<0.128.0>: inserted {:foo, \"bar\"} to :some_history_table"
iex(8)>
nil
iex(9)> IO.inspect("#{inspect(self())}: The async task is: #{inspect(Process.info(task.pid))}")
"#PID<0.105.0>: The async task is: nil"
"#PID<0.105.0>: The async task is: nil"
iex(10)> IO.inspect("#{inspect(self())}: :some_history_table has #{inspect(:ets.tab2list(:some_history_table))}")
** (ArgumentError) argument error
(stdlib 3.12) :ets.match_object(:some_history_table, :_)
(stdlib 3.12) ets.erl:763: :ets.tab2list/1
There was a problem hiding this comment.
Good points raised by both of you.
It's not uncommon to create the ETS table in the application process (top level supervisor) if the purpose of the ETS is to NOT crash it with the child process. Or, you put a process next to Server, let's call it Manager and you set the ETS configuration so that heir is the Manager process. In case Server restarts, it'll hand off the data to Manager. And implement the logic in the Manager that will give it back to the Server once it's restarted.
Don't forget, ETS is a shared state entity, so you need to be super careful. Because you might put something in there that will crash other processes (the reason why Erlang/Elixir are not pure functional languages is because they have primitives like ETS).
But what you've done here in Server and later in the Fetcher is that you're trying to handle everything that might happen to the process and prevent it from dying. Not everything (you could have trapped exits!).
The problem is... you can not be 100% in your approach. Remember the :hackney bug that uncontrolably sends a ssl_closed message? benoitc/hackney#464
This would crash your Server and the ETS with it.
apps/omg_child_chain/lib/omg_child_chain/gas_price/history/server.ex
Outdated
Show resolved
Hide resolved
apps/omg_child_chain/lib/omg_child_chain/gas_price/strategy/block_percentile_gas_strategy.ex
Outdated
Show resolved
Hide resolved
apps/omg_child_chain/lib/omg_child_chain/gas_price/strategy/legacy_gas_strategy.ex
Outdated
Show resolved
Hide resolved
| ] | ||
|
|
||
| _ = Logger.info("Starting #{__MODULE__}") | ||
| Supervisor.init(children, strategy: :one_for_one) |
There was a problem hiding this comment.
I think if History dies, all process need to restart to re-subscribe? Probably cannot use one_for_one?
There was a problem hiding this comment.
Wow epic catch! 💯 💯 💯 💯 💯
Bouncing idea: Would keeping :one_for_one as is, and instead Process.link/1 the history and the strategy during History.subscribe/1 be more optimal?
Otherwise if we change to :all_for_one, we also kill LegacyGasStrategy which does not depend on History and unnecessarily destroys its state. And :rest_for_one depends on the children sequence which I think it a bit too implicit/mistake-prone.
There was a problem hiding this comment.
cc: @InoMurko for opinion
There was a problem hiding this comment.
Sounds like a good idea! 🔗🔗🔗🔗🔗🔗
There was a problem hiding this comment.
I see what you wanted to do, but designing a proper pub/sub ... this might be a little bit too simplistic. For example, you're not tracking dead pids in your subscription list. Perhaps it would be easier to simply use OMG.Bus and define a new topic?
|
|
||
| # The ets table is not initialized with `:read_concurrency` because we are expecting interleaving | ||
| # reads and writes. See http://erlang.org/doc/man/ets.html | ||
| history_ets = :ets.new(@history_table, [:ordered_set, :protected, :named_table]) |
There was a problem hiding this comment.
Good points raised by both of you.
It's not uncommon to create the ETS table in the application process (top level supervisor) if the purpose of the ETS is to NOT crash it with the child process. Or, you put a process next to Server, let's call it Manager and you set the ETS configuration so that heir is the Manager process. In case Server restarts, it'll hand off the data to Manager. And implement the logic in the Manager that will give it back to the Server once it's restarted.
Don't forget, ETS is a shared state entity, so you need to be super careful. Because you might put something in there that will crash other processes (the reason why Erlang/Elixir are not pure functional languages is because they have primitives like ETS).
But what you've done here in Server and later in the Fetcher is that you're trying to handle everything that might happen to the process and prevent it from dying. Not everything (you could have trapped exits!).
The problem is... you can not be 100% in your approach. Remember the :hackney bug that uncontrolably sends a ssl_closed message? benoitc/hackney#464
This would crash your Server and the ETS with it.
| _ = Logger.info("#{__MODULE__} available gas prices from Eth heights: #{from_height} - #{to_height}.") | ||
|
|
||
| # Inform all subscribers that the history has been updated. | ||
| _ = Enum.each(state.subscribers, fn subscriber -> send(subscriber, {History, :updated}) end) |
There was a problem hiding this comment.
how come didn't you utilise OMG.Bus for this?
| prices = Enum.map(block["transactions"], fn tx -> Encoding.int_from_hex(tx["gasPrice"]) end) | ||
| timestamp = Encoding.int_from_hex(block["timestamp"]) | ||
|
|
||
| true = :ets.insert(history_ets, {height, prices, timestamp}) |
There was a problem hiding this comment.
It's already streaming per item though (because of the processing needed on each item). Not sure if it's useful to bulk it the stream up before inserting... 🤔
apps/omg_child_chain/lib/omg_child_chain/gas_price/history/server.ex
Outdated
Show resolved
Hide resolved
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| defmodule OMG.ChildChain.GasPrice.History.Fetcher do |
There was a problem hiding this comment.
I like that you found the correct border - the logic that separates the integration point and your internal algorithm. Noice!
I would add another option that allows you to test this (wink, I noticed you didn't explicitly test this file).
I found it greatly simplifies testing if you use Ethereumex second/third Keyword paramter: url: url.
That allows you to define your own server and simplifies exunit testing considerably! Lemme know if you're interested how I've done it.
| @impl GenServer | ||
| def handle_cast({:subscribe, subscriber}, state) do | ||
| subscribers = Enum.uniq([subscriber | state.subscribers]) | ||
| {:noreply, %{state | subscribers: subscribers}} |
There was a problem hiding this comment.
this is an ever growing list of subscribers? how do you remove dead pids?
| ] | ||
|
|
||
| _ = Logger.info("Starting #{__MODULE__}") | ||
| Supervisor.init(children, strategy: :one_for_one) |
There was a problem hiding this comment.
I see what you wanted to do, but designing a proper pub/sub ... this might be a little bit too simplistic. For example, you're not tracking dead pids in your subscription list. Perhaps it would be easier to simply use OMG.Bus and define a new topic?
| # Internal implementations | ||
| # | ||
|
|
||
| defp do_recalculate() do |
There was a problem hiding this comment.
perhaps a quick desc in what it does.
I believe History.all() returns all stored blocks (like 200) and you find the median price?
What happens if you don't have enough blocks to properly estimate, could it even happen?
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| defmodule OMG.ChildChain.GasPrice.Strategy.LegacyGasStrategy do |
There was a problem hiding this comment.
I believe this didn't change right? I don't have to look at this? he he he
apps/omg_child_chain/lib/omg_child_chain/release_tasks/set_block_submit_gas_price_strategy.ex
Outdated
Show resolved
Hide resolved
|
Deprioritized for now. Will pick back up maybe during the cleanup period |
|
Let the genie out! |
TODOs:
|
# Conflicts: # apps/omg_child_chain/lib/omg_child_chain/block_queue/core.ex
# Conflicts: # apps/omg_child_chain/lib/omg_child_chain/configuration.ex # apps/omg_child_chain/lib/omg_child_chain/sync_supervisor.ex # config/config.exs # config/releases.exs # docs/deployment_configuration.md
|
Will reopen in https://github.com/omgnetwork/omg-childchain-v1 |
Relates to #1268
Overview
Adds a new gas estimation strategy using poisson regression (also used by ETH Gas Station).
With dual strategies, a feature flag
BLOCK_SUBMIT_GAS_PRICE_STRATEGYis also introduced.Changes
OMG.ChildChain.BlockQueue.CoreintoOMG.ChildChain.GasPriceandOMG.ChildChain.GasPrice.LegacyGasStrategyOMG.ChildChain.GasPrice.PoissonGasStrategyBLOCK_SUBMIT_GAS_PRICE_STRATEGYflag (options:LEGACYandPOISSON, defaults toLEGACY).LegacyGasStrategyandPoissonGasStrategywill still compute and log their results even with the flag. The flag is used specifically to dictate which strategy to use for actual block submissionTesting
Gas price estimations from both
LegacyGasStrategyandPoissonGasStrategyshould appear in log.