Skip to content

Conversation

frolosofsky
Copy link
Member

@frolosofsky frolosofsky commented Mar 15, 2019

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 for mapBlockIndex.

Fixes #432.

@thothd thothd changed the title Save state repository to disk (leveldb) Save finalization state repository to disk (leveldb) Mar 15, 2019
@thothd thothd requested a review from kostyantyn March 16, 2019 15:01
@frolosofsky frolosofsky force-pushed the save-state-repo branch 2 times, most recently from aea5cc3 to bf18809 Compare March 21, 2019 08:44
@frolosofsky frolosofsky changed the title Save finalization state repository to disk (leveldb) Implement proper finalization state repository saving and loading (leveldb) Mar 21, 2019
@frolosofsky frolosofsky self-assigned this Mar 21, 2019
@frolosofsky frolosofsky requested review from scravy, Gnappuraz, cornelius and a team March 21, 2019 09:27
@frolosofsky frolosofsky force-pushed the save-state-repo branch 2 times, most recently from 0338333 to 16cfaf9 Compare March 21, 2019 09:59
@@ -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";
Copy link
Member Author

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.

Copy link
Member

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.

@frolosofsky frolosofsky marked this pull request as ready for review March 21, 2019 10:00
@frolosofsky frolosofsky added this to the 0.1 milestone Mar 25, 2019
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Copy link
Member

@thothd thothd left a comment

Choose a reason for hiding this comment

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

utACK 6378a21

Copy link
Member

@Gnappuraz Gnappuraz left a comment

Choose a reason for hiding this comment

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

utACK 6378a21

Copy link
Member

@nzmdn nzmdn left a comment

Choose a reason for hiding this comment

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

utACK 6378a21

@frolosofsky frolosofsky merged commit 557f08c into dtr-org:master Mar 26, 2019
scravy added a commit that referenced this pull request Mar 28, 2019
#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>
@frolosofsky frolosofsky deleted the save-state-repo branch April 1, 2019 04:29
scravy added a commit that referenced this pull request Apr 1, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants