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.

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

feat: add /v2/micro-blocks/:hash endpoint #898

Merged
merged 5 commits into from
Sep 13, 2022
Merged

feat: add /v2/micro-blocks/:hash endpoint #898

merged 5 commits into from
Sep 13, 2022

Conversation

sborrazas
Copy link
Contributor

@sborrazas sborrazas commented Sep 9, 2022

refs #888

@sborrazas sborrazas self-assigned this Sep 9, 2022
Copy link
Collaborator

@thepiwo thepiwo left a comment

Choose a reason for hiding this comment

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

again docs seem missing

Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

No operational findings, just some enhancements.

Comment on lines 74 to 75
hash
|> Db.get_micro_blocks()
Copy link
Member

Choose a reason for hiding this comment

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

Nice trick using the mb hash instead of next kb hash 👏 Depending on how much use the Explorer gonna make of this endpoint, maybe the AeMdw.Node.Db could have a similar micro block walker function that only uses :aec_blocks.prev_hash(block). It might be (or not) an early optimization but since it's a simple code I would go this way instead of profiling and monitoring the use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. :aec_blocks.prev_hash(block) returns a hash, so I can't do :aec_blocks.prev_hash(:aec_blocks.prev_hash(hash)). I need to use :aec_db.get_block(hash) or :aec_db.get_header(hash) too to get the prev_hash from it

Copy link
Member

Choose a reason for hiding this comment

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

It's because the current micro_block_walker defines the unfolding function expected by unfold(acc(), (acc() -> {element(), acc()} | nil)) in a way that the element accumulated is the block:

{block, :aec_blocks.prev_hash(block)}

On the other hand with a simple recursion the accumulator could be the index counter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference between using a number accumulated and having this stream reduce blocks is that it will keep 1 block in-memory, while the other alternative will keep 1 number in-memory. TBH, I don't think it's worth a separate implementation since the block is quite tiny in size

Copy link
Member

Choose a reason for hiding this comment

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

What caught my attention on the first moment was Enum.count being used on the blocks. The unfold keeps all previous microblocks, including its transactions in memory accumulating them to create a list. So one related factor that popped up when I saw it was the comment on the amount of microblocks, that there might thousands in a single generation (decided by the leader limited only by the delay of 3 seconds). On top of this taking a more objective number, thinking only on a 100 TPS and supposing all transactions are spend_tx (a small kind of transaction with ~ 1KB) it would allocate and reallocate up to 3 * 60 * 100 * 1000 = 18MB per request. In order to handle 1000 simultaneous requests it would require 18GB only for this function call. As the goal of these new endpoints is to provide lighter ones, I think it's a good opportunity to save some resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not how streams work, streams are lazy, which is why there will be at most 1 micro block in memory at any given time. Please note that at no point the stream is turned into a list

stream
|> Enum.to_list()
|> Enum.count()

is very different from

stream
|> Enum.count()

The former will contain all the elements of the Enum in memory, while the latter will contain at most 1 element of the Enum at any given time

Copy link
Member

Choose a reason for hiding this comment

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

While Stream.map operates on each element Enum.map depends on the whole list. Almost sure that Enum.count would behave the same but I am checking it.

Copy link
Member

Choose a reason for hiding this comment

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

It's true when there is a Enum.reverse() like the micro_block_walker:

iex(aeternity@localhost)1> Task.async(fn -> Stream.unfold(1_000_000, fn 0 -> nil; n -> {n, n-1} end) |> Enum.reverse() |> Enum.count(); :erlang.process_info(self(), :memory) end) |> Task.await()
{:memory, 22445712}

] do
assert %{
"height" => ^kbi,
"transactions_count" => 6
Copy link
Member

Choose a reason for hiding this comment

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

Could we have also the "micro_block_index"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sborrazas sborrazas force-pushed the micro-blocks-v2 branch 2 times, most recently from f960765 to fba3ab6 Compare September 12, 2022 13:20
Base automatically changed from micro-blocks-v2 to master September 12, 2022 13:33
@sborrazas sborrazas merged commit 2c16e47 into master Sep 13, 2022
@sborrazas sborrazas deleted the micro-block-v2 branch September 13, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants