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

Feature: digest tracker #1370

Merged
merged 19 commits into from
Nov 10, 2022
Merged

Feature: digest tracker #1370

merged 19 commits into from
Nov 10, 2022

Conversation

xDimon
Copy link
Member

@xDimon xDimon commented Oct 18, 2022

Referenced issues

Resolves #1363

Description of the Change

Observe digest and call special observer for each digest type
Move epoch descriptor mechanism to babe config repository
Move babe util implementation to babe config repository
Mode dispatch of finalisation event from block tree to babe

Benefits

Handling next epoch data
Conditional (by babe state) using runtime

@xDimon xDimon force-pushed the feature/digest_tracker branch from fd7d08f to fde8929 Compare October 19, 2022 03:43
@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #1370 (0f59fd6) into master (ed531b0) will decrease coverage by 0.41%.
The diff coverage is 14.40%.

❗ Current head 0f59fd6 differs from pull request most recent head dac2c10. Consider uploading reports for the commit dac2c10 to get more accurate results

@@            Coverage Diff             @@
##           master    #1370      +/-   ##
==========================================
- Coverage   24.85%   24.43%   -0.42%     
==========================================
  Files         629      633       +4     
  Lines       23794    24035     +241     
  Branches    12314    12470     +156     
==========================================
- Hits         5915     5874      -41     
- Misses      12603    12906     +303     
+ Partials     5276     5255      -21     
Impacted Files Coverage Δ
core/blockchain/block_tree.hpp 100.00% <ø> (ø)
core/blockchain/impl/block_tree_impl.cpp 19.48% <0.00%> (-1.72%) ⬇️
core/blockchain/impl/block_tree_impl.hpp 33.33% <ø> (ø)
core/blockchain/impl/cached_tree.hpp 75.00% <ø> (ø)
core/consensus/authority/impl/schedule_node.hpp 37.50% <0.00%> (+4.16%) ⬆️
core/consensus/babe/babe_config_repository.hpp 100.00% <ø> (ø)
core/consensus/babe/babe_error.cpp 0.00% <0.00%> (ø)
core/consensus/babe/babe_util.hpp 100.00% <ø> (ø)
core/consensus/babe/impl/babe_config_node.hpp 0.00% <0.00%> (ø)
core/consensus/babe/impl/babe_impl.hpp 100.00% <ø> (ø)
... and 34 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xDimon xDimon force-pushed the feature/digest_tracker branch from 903134e to bec874a Compare October 20, 2022 10:44
test/core/host_api/offchain_extension_test.cpp Outdated Show resolved Hide resolved
core/host_api/impl/offchain_extension.cpp Outdated Show resolved Hide resolved
core/runtime/common/runtime_instances_pool.hpp Outdated Show resolved Hide resolved
@xDimon xDimon force-pushed the feature/digest_tracker branch 5 times, most recently from a4e1a7b to 8a7be9f Compare November 3, 2022 09:39
@xDimon xDimon requested review from iceseer and kamilsa November 3, 2022 09:40
@xDimon xDimon marked this pull request as ready for review November 3, 2022 09:45
@xDimon xDimon force-pushed the feature/digest_tracker branch 15 times, most recently from 2ed3ee0 to 2e5a0f0 Compare November 8, 2022 09:10
@xDimon xDimon requested a review from turuslan November 8, 2022 11:21
core/blockchain/impl/digest_tracker_impl.cpp Outdated Show resolved Hide resolved
core/blockchain/impl/digest_tracker_impl.cpp Outdated Show resolved Hide resolved
core/blockchain/impl/digest_tracker_impl.cpp Outdated Show resolved Hide resolved
core/consensus/babe/impl/babe_config_node.cpp Outdated Show resolved Hide resolved
core/consensus/babe/impl/babe_config_repository_impl.cpp Outdated Show resolved Hide resolved
@xDimon xDimon force-pushed the feature/digest_tracker branch from 2e5a0f0 to a5cd511 Compare November 8, 2022 12:51
xDimon added 18 commits November 9, 2022 04:34
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
refactor: epoch descriptor storage moved to babe config repository
refactor: babe util implementation moved to babe config repository

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
…error

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
…not in fast-sync)

Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: Dmitriy Khaustov aka xDimon <khaustov.dm@gmail.com>
Signed-off-by: safinsaf <safinsaft@gmail.com>
@xDimon xDimon force-pushed the feature/digest_tracker branch from c40ced6 to a641045 Compare November 8, 2022 20:34
@xDimon xDimon enabled auto-merge (squash) November 9, 2022 10:10

prune(block_info);

if (block_info.number % (kSavepointEachSuchBlock / 10) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

А почему в 10 раз чаще?
и тогда нужно поставить static_assert(kSavepointEachSuchBlock >= 10).

ходим с периодом kSavepointEachSuchBlock в цикле выше

// Save state on finalized part of blockchain
if (need_to_save) {
if (auto save_res = save(); save_res.has_error()) {
SL_WARN(logger_, "Can't re-save state: {}", save_res.error());
Copy link
Contributor

Choose a reason for hiding this comment

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

Мы должны продолжить работу если стейт не сохранился?

Copy link
Contributor

Choose a reason for hiding this comment

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

Насколько понимаю, мы будем продолжать пытаться сохранить стейт в этот чекпоинт пока находимся внутри этого периода, но как только перешагнем через границу kSavepointEachSuchBlock, будем сохранять уже следующий...насколько критичен пропуск если не удалось сделать сейвпоинт в пределах одного периода?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it not problem. It can be succeed next time. Anyway at the next start it can be recovered from last savepoint

Comment on lines +371 to +381
BOOST_ASSERT(last_saved_state_block_ <= finalized_block.number);

auto saving_state_node = getNode(finalized_block);
BOOST_ASSERT_MSG(saving_state_node != nullptr,
"Finalized block must have associated node");
const auto saving_state_block = saving_state_node->block;

// Does not need to save
if (last_saved_state_block_ >= saving_state_block.number) {
return outcome::success();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Мне кажется Ассерт и выражение под if противоречат друг другу. Если ситуация (last_saved_state_block_ > finalized_block.number) является нормальной и обрабатывается, то возможно ассерт лишний.

Comment on lines +383 to +385
const auto last_savepoint =
(last_saved_state_block_ / kSavepointEachSuchBlock)
* kSavepointEachSuchBlock;
Copy link
Contributor

Choose a reason for hiding this comment

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

округление до кратного периоду встречается несколько раз, возможно имеет смысл вынести в отдельный метод.

@xDimon xDimon merged commit 79ae107 into master Nov 10, 2022
@xDimon xDimon deleted the feature/digest_tracker branch November 10, 2022 08:19
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.

Digest tracker
3 participants