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

[1.0.3] Review: Alternative implementation of handling forking #968

Conversation

heifner
Copy link
Member

@heifner heifner commented Oct 22, 2024

I was concerned about the performance of block_has_trx.

Review comments for #966

@heifner heifner requested a review from linh2931 October 22, 2024 20:12
Copy link
Member

@linh2931 linh2931 left a comment

Choose a reason for hiding this comment

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

Thanks! It does not need an extra look up the block's transaction list. Please go ahead to merge it into my branch. I will remove block_has_trx() and add additional tests.

} else if (std::holds_alternative<lib_entry_v0>(entry)) {
auto lib = std::get<lib_entry_v0>(entry).lib;
if (trx_entries > 0 && lib >= trx_block_num) {
return trx_block_num;
if (!trx_block_nums.empty() && lib >= *(--trx_block_nums.end())) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment saying *(--trx_block_nums.end()) is the highest block number as trx_block_nums is a set of uint32_t.

Copy link
Member

Choose a reason for hiding this comment

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

Or I can add the comments myself after you merge.


// initial block forks out
sp.append(forked_block_trace);
sp.append(forked_block_trace); // block 1
sp.append_trx_ids(forked_block_trxs_entry); // block 1
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this. I missed it.

@heifner heifner marked this pull request as ready for review October 22, 2024 20:38
@heifner heifner merged commit da532e0 into fix_trace_api_get_trx_block_number Oct 22, 2024
35 checks passed
@heifner heifner deleted the fix_trace_api_get_trx_block_number-alt branch October 22, 2024 20:38
@heifner heifner added the OCI Work exclusive to OCI team label Oct 24, 2024
@ericpassmore
Copy link
Contributor

Note:start
category: System Stability
component: Internal
summary: Alternative implementation of handling forking.
Note:end

@heifner heifner changed the title Review: Alternative implementation of handling forking [1.0.3] Review: Alternative implementation of handling forking Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants