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

babe indexer #1661

Merged
merged 17 commits into from
Jul 19, 2023
Merged

babe indexer #1661

merged 17 commits into from
Jul 19, 2023

Conversation

turuslan
Copy link
Contributor

@turuslan turuslan commented Jun 16, 2023

Referenced issues

Description of the Change

  • (most of application_injector.cpp changes are clang-format, view with "ignore whitespace changes").
  • add Indexer.
    • Lazily compute some optional value for each chain in block from it's parents.
    • Does not miss digests, because it indexes each block between requested and last indexed.
    • Descent remembers unindexed blocks info.
    • Store unfinalized and cached values in memory.
    • Store finalized values in db.
    • Indexed::inherit=true means block is indexed and value is in block Indexed::prev.
  • change BabeConfigRepository to use Indexer.
    • If there are more than 10000 unindexed finalized blocks (empty or old db), find last digest and call runtime.
    • Use BabeIndexedValue::state from runtime for genesis and warp sync'ed blocks.
    • Store latest BabeIndexedValue::config, because digest changing it is rare.
    • Compute BabeIndexedValue::next_state from BabeIndexedValue::config and digests.
  • change BabeConfigRepository::readFromState(BlockInfo) to BabeConfigRepository::warp(BlockInfo).
  • add BabeConfigRepository toWarpSync.
  • remove BabeDigestObserver from DigestTracker.
  • add SCALE_TIE_ONLY(field1, field2, ...) macro.
  • add InMemoryCursor.
  • add MapCursor::seekReverse(prefix).
  • change getNextEpochDigest to HasAuthoritySetChange.
  • add BlockTree::isFinalized(BlockInfo).
  • add BlockTree::hasDirectChain(BlockInfo, BlockInfo) stub.
  • add BlockHeader::parentInfo().

Benefits

  • don't miss babe digests

Possible Drawbacks

  • BUG: warp sync finalizes blocks, but headers are missing, so need to put marker (there is state, so it must be different from "warp sync ended", to avoid recovering from state on that block) on last block when starting warp sync too (first WarpSync::applyInner call), or will miss digests for old blocks. (or special struct for warp sync blocks/gaps)

Signed-off-by: turuslan <[email protected]>
Signed-off-by: turuslan <[email protected]>
@turuslan turuslan requested review from Harrm and iceseer June 19, 2023 05:54
Copy link
Contributor

@Harrm Harrm left a comment

Choose a reason for hiding this comment

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

Sorry for so many naming comments, but I comment when I genuinely get confused while reading.

core/consensus/babe/impl/babe_config_repository_impl.hpp Outdated Show resolved Hide resolved
core/consensus/babe/impl/block_appender_base.cpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Show resolved Hide resolved
core/consensus/babe/has_authority_set_change.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
core/blockchain/indexer.hpp Outdated Show resolved Hide resolved
@turuslan turuslan requested a review from Harrm June 22, 2023 17:56
Signed-off-by: turuslan <[email protected]>
@turuslan turuslan requested a review from Harrm June 23, 2023 03:46
Signed-off-by: turuslan <[email protected]>
Signed-off-by: turuslan <[email protected]>
}

std::shared_ptr<blockchain::BlockTree> block_tree_;
mutable std::vector<primitives::BlockInfo> path_;
Copy link
Contributor

Choose a reason for hiding this comment

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

кажется, что std::deque тут будет лучше

Copy link
Contributor

Choose a reason for hiding this comment

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

либо резервировать по высоте |to.num - start.num|

Copy link
Contributor

Choose a reason for hiding this comment

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

Возможно дистанция может быть очень большой, имеет смысл ограничить глубину?

core/blockchain/indexer.hpp Show resolved Hide resolved
std::shared_ptr<storage::BufferStorage> db_;
std::shared_ptr<blockchain::BlockTree> block_tree_;
primitives::BlockInfo last_finalized_indexed_;
std::map<primitives::BlockInfo, Indexed<T>> map_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Возможно тут map<> не нужен, у нас в список элементы кладутся уже сортированными и правая часть(до финализации) непрерывкная. Можно организовать поиск за O(1) по индексу заместо O(log N) по мапе.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. проверяем по индексу за O(1), если лежит то что искали, то ок - выходим. Лучший случай
  2. если не то что надо, то за O(log N) через lower_bound. Худший случай.

Signed-off-by: turuslan <[email protected]>

# Conflicts:
#	core/consensus/babe/impl/babe_config_repository_impl.cpp
#	core/consensus/babe/impl/babe_impl.cpp
#	core/injector/application_injector.cpp
#	test/core/network/state_protocol_observer_test.cpp
Signed-off-by: turuslan <[email protected]>
@turuslan turuslan enabled auto-merge (squash) July 19, 2023 14:13
@turuslan turuslan merged commit f0132ae into master Jul 19, 2023
@turuslan turuslan deleted the babe/indexer branch July 19, 2023 15:26
@turuslan turuslan mentioned this pull request Jul 26, 2023
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.

5 participants