-
Notifications
You must be signed in to change notification settings - Fork 14
Implement proper finalization state repository saving and loading (leveldb) #793
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
Conversation
aea5cc3
to
bf18809
Compare
0338333
to
16cfaf9
Compare
@@ -93,6 +93,7 @@ Parameters Parameters::TestNet() noexcept { | |||
p.genesis_block = GenesisBlock(GenesisBlockBuilder().Add(TestnetFunds()).Build(p)); | |||
|
|||
p.default_settings.p2p_port = 17182; | |||
p.data_dir_suffix = "testnet3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name testnet3
comes from chainparams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we probably want to change that but not in this PR.
16cfaf9
to
d6380b7
Compare
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
14f8e6a
to
fb743fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 6378a21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 6378a21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 6378a21
#793 introduced comments which produce warnings in gcc as they use the blackslash character at the end of a line. The backslash character continues a line which means that: ```C++ int x = 3; // comment \ x = 4; std::cout << x << std::endl; ``` would adjoin the line `x = 4` to the comment before. The snippet above will print `3`, clearly not what was intended. The syntax highlight here in github shows this nicely too. @kostyantyn and I were discussing to use a unicode character instead, but unfortunately having unicode characters in the codebase screws up some lint scripts :-( So I am going for the alternative proposed by @kostyantyn , which is to use ``` | > ``` instead for the ascii graphic. Signed-off-by: Julian Fleischer <julian@thirdhash.com>
Currently unit tests can not be run in parallel. This prevents us from speeding up ci builds (see bitcoin/bitcoin#12926, #865) and won't work with the updated build definitions from bitcoin 0.17 (#860). This is a regression which was introduced in #793. This pull request fixes #861. The problem is that the `StateDB` component does something actively when being initialized (not something a component should do), which is to access disk and so on, which can fail. Since there is a `UnitEInjector` for every `BasicTestingSetup` running the unit tests in parallel creates a database _in the same directory_ per unit tests suite which clashes and breaks and throws and aborts and dies. This can be prevented by using an in-memory database, but the problem shifts: How to get that configuration in? I did not want to expose this as a user-definable setting and also not as a chain parameter, so these two configuration options are not available. Which is why I created `UnitEInjectorConfiguration`. This is something which should not be necessary as usually you would not expose the same module as in production in unit tests but use a subset or just the mocked version, but since the rest of bitcoin is not in well-defined components and accesses them using `GetComponent` a global injector has to be available in some tests. The change ultimately does not affect the prod version of things, but the tests, which inject a different `UnitEInjectorConfiguration`. I made this a `struct` rather then just a flag `unit_tests` such that it is extensible in case we have other cases like this. In order to make the injector be able to access fields from it's own instance in the definition of a component I altered the definition of `UNMANAGED_COMPONENT` a bit. I would like to point out that these initialization issues would also have happened with an Application class (#723), as that is exactly what `UnitEInjector` is, just you would inject things manually. Signed-off-by: Julian Fleischer <julian@thirdhash.com>
Introduces finalization::StateDB component which is basically an interface to load and save repository. It provides two ways to restore repository:
StateDB::Load
loads every entry from the database to the repository.StateDB::LoadStatesHigherThan
loads states for the blocks which height is greater than given height.The second one is preferable as we keep only a limited set of states in memory. In other words, even when we load everything from the database, we trim the repository on the next step.
Also implements recovery for cases when finalization state database is partially broken or out-of-sync with the block-index database.
Adds
BlockIndexMap
component which is a mockable abstraction formapBlockIndex
.Fixes #432.