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

IF: Support load of fork database after instant-finality is enabled #2113

Merged
merged 12 commits into from
Jan 21, 2024

Conversation

heifner
Copy link
Member

@heifner heifner commented Jan 19, 2024

  • Supports writing/reading of fork_database in legacy and after instant-finality is activated.
  • Moves mutex out of fork database implementation and into fork_database to protect transition of impl from legacy to instant finality.
  • Includes some refactoring:
    • Moves controller::head which was in controller block_data_gen_t::head to fork_database_t::chain_head
    • Removes block_data_gen_t and block_data_t

Resolves #2048
Resolves #2083

…ant-finality.

GH-2083 Moved mutex out of fork database impl so it can protect transition from legacy to instant-finality.
@heifner heifner added the OCI Work exclusive to OCI team label Jan 19, 2024
* as well as switching from legacy to instant-finality.
*/
class fork_database {
mutable std::recursive_mutex m;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of recursive mutexes. However, currently controller uses nested calls of fork_database.apply. We can look at refactoring those so that no apply calls functions which also calls apply but that will take some work.

Copy link
Contributor

@greg7mdp greg7mdp left a comment

Choose a reason for hiding this comment

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

Nice change. I'm really glad we got rid of block_data, this is much nicer.

libraries/chain/fork_database.cpp Outdated Show resolved Hide resolved
libraries/chain/fork_database.cpp Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
}}, v);
}

// if we every support more than one version then need to save min/max in fork_database_t
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment doesn't read right. Maybe this

Suggested change
// if we every support more than one version then need to save min/max in fork_database_t
// if we ever support more than one version, then we need to save min/max in fork_database_t

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.

Like the restructuring.

@@ -7,6 +7,11 @@

namespace eosio::chain {

// moved this warning out of header so it only uses once
#warning TDDO https://github.com/AntelopeIO/leap/issues/2080
Copy link
Member

Choose a reason for hiding this comment

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

Now it occurs to me, if we already have an issue, we don't need a warning, because the issue will remind us.

libraries/chain/fork_database.cpp Outdated Show resolved Hide resolved
@ericpassmore
Copy link
Contributor

Note:start
group: IF
category: INTERNALS
summary: Supports writing/reading of fork_database in legacy and after instant-finality is activated. Specific to Faster Finality Includes work to persist fork_database on shutdown.
Note: end

@heifner heifner merged commit 7284414 into hotstuff_integration Jan 21, 2024
26 checks passed
@heifner heifner deleted the GH-2048-persist-if-fork-db branch January 21, 2024 02:18
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.

IF: fork database transition not thread safe IF: Persist post-IF fork database to disk on shutdown
4 participants