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

Compact block cache pt3 #170

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

idky137
Copy link
Contributor

@idky137 idky137 commented Jan 27, 2025

Final integration of the Compact Block Cache.

  • Adds BlockCache to FetchService and uses BlockCache block fetch methods.
  • Updates DB to use big-endian bytes for height instead of Height(u32) (zebra-chain). This is required to search LMDB height keys by order.
  • Moves startup sync from FetchService::Spawn to BlockCache::Spawn.

@idky137 idky137 added the ZGM2 Issues that need to be resolved for the completion of the Zaino dev grant milestone 2 label Jan 29, 2025
@idky137 idky137 marked this pull request as ready for review January 29, 2025 14:17
@idky137 idky137 requested review from zancas and AloeareV January 29, 2025 14:17
@idky137 idky137 marked this pull request as draft January 30, 2025 13:26
@idky137 idky137 marked this pull request as ready for review January 30, 2025 13:42
@idky137 idky137 mentioned this pull request Jan 30, 2025
Copy link
Contributor

@pacu pacu left a comment

Choose a reason for hiding this comment

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

Left some remarks and requests! Pretty exciting step forward!

@@ -54,8 +54,40 @@ impl NonFinalisedState {
config,
};

// If no_db is active wait for server to sync with p2p network.
if non_finalised_state.config.no_db
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be factored out to a function of non_finalized_state that has meaningful name? It seems a pretty specific condition that is being described here.

{
break;
} else {
println!(" - Validator syncing with network. Validator chain height: {}, Estimated Network chain height: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a log entry instead?

@@ -286,8 +274,11 @@ impl NonFinalisedState {
};
}

// Refill from reorg_height[+1].
for block_height in (reorg_height.0 + 1)..self.fetcher.get_blockchain_info().await?.blocks.0
// Refill from max(reorg_height[+1], sapling_activation_height).
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you refill the cache from reorg_height - 1?

let validator_height = self.fetcher.get_blockchain_info().await?.blocks.0;
for block_height in ((reorg_height.0 + 1)
.max(self.config.network.sapling_activation_height().0))
..=validator_height
{
// Either pop the reorged block or pop the oldest block in non-finalised state.
Copy link
Contributor

Choose a reason for hiding this comment

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

Document Reorg Logic outside code as well.

{
self.status.store(StatusType::CriticalError.into());
return Err(NonFinalisedStateError::Critical(
"Critical error in database. Closing NonFinalisedState"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the error be more self explanatory given that the conditions of the failure are well-known?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZGM2 Issues that need to be resolved for the completion of the Zaino dev grant milestone 2
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants