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

Finality votes cache #116

Merged
merged 19 commits into from
Jun 10, 2020
Merged

Finality votes cache #116

merged 19 commits into from
Jun 10, 2020

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Jun 1, 2020

attempt to solve (1) from #78 (comment)
on top of #105

This PR adds persistent finality votes cache. It doesn't change computational complexity of headers finalization algorithm, but instead significantly decreases current implementation overhead. I.e. if number of unfinalized headers is N, then we still need to cycle over N headers, but instead of making N storage reads, we only need to perform limited number of reads (though at least one of these reads may read larger number of bytes than regular header - see below).

We still need to introduce 'spanned finalization' (mentioned in #78 (comment)), but I think for now (I mean for initial start) the cache should be enough. On the same 1926 unfinalized Kovan headers from #78, I'm now getting time(import(header#1926)) / time(import(header#1)) ~ 6.5 instead of previous ~46.5.

The cache now works as follows:

  1. when header N is inserted, we're building struct FinalityVotes, that includes info about all unfinalized ancestors of this header (note that it isn't header itself, but some shortened version);
  2. if N % FinalityVotesCachingInterval == 0 then we're inserting this structure into finality cache;
  3. when next header N+1 is inserted, we will only need to read header N and this struct;
  4. if nothing has been finalized yet, then at N+2 we'll read two headers (N+1, N) and the same struct;
  5. ...
  6. at header N + FinalityVotesCachingInterval we'll insert another entry into finality cache;
  7. at header N + FinalityVotesCachingInterval + 1 we'll again need to read only one header (N + FinalityVotesCachingInterval and that struct).

The cache itself may be improved later, e.g. by not inserting entries if finalization is regular (i.e. we do not have large delays) or by making bridge guess FinalityVotesCachingInterval on its own (i.e. we know number of validators and we know we need votes of 2/3 validators => so having 2/3*len(validators) + regular_finalization_delay unfinalized blocks may be a good criteria for enabling cache and initial caching interval).

The cached struct has approximate size of NUM_VOTED_VALIDATORS*28 + NUM_UNFINALIZED_HEADERS*81. E.g. if 5 validators have authored 1000 unfinalized blocks, then the size would be ~80K. This is rather large storage entry, but: (1) it is much better than reading 1000 headers and (2) I'm not sure if we'll be able to get rid of this struct even in 'spanned finalization', because we'll need to cache computation results somewhere between continue_finalization() calls.

Performance comparison on initial Kovan headers: before this PR vs after this PR (native execution, debug build).

@tomusdrw
Copy link
Contributor

tomusdrw commented Jun 1, 2020

Why is it on top of #105 ? Is there a specific thing required from that PR? Maybe split it into two then or ping us to review #105 :)

Reviewing "on top" PRs is really super hard (read: impossible) and after we squash-merge #105 there's going to be a mess with commits which also makes it a bit harder to grasp your intentions (by looking at individual commits).

@svyatonik
Copy link
Contributor Author

svyatonik commented Jun 1, 2020

  1. It actually uses stuff from Use runtime storage in PoA -> Substrate module tests #103 - will rebase on top of it instead;
  2. I'm not willing to push/ping anyone for review - I feel uneasy about it;
  3. the simplest solution is not to squash, no? Right now this PR has only one commit - if I'll add any other significant changes, I'll left a comment here.

@svyatonik
Copy link
Contributor Author

It should be ready now. This PR only adds commits >= finality cache.

One last thing that I wanted to mention is that now PoA headers are mostly referenced by HeaderId { hash, number } both when in storage and in memory. This may now seem redundant almost everywhere, except for StoredHeader::last_signal_block. But there (and maybe later in other places) it helps to resolve header.number by header.hash without additional storage reads.

@svyatonik svyatonik changed the title [WIP] Finality votes cache Finality votes cache Jun 3, 2020
@tomusdrw tomusdrw added this to Pull Requests in Rialto Release via automation Jun 5, 2020
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

/// Finality votes for given block.
#[derive(RuntimeDebug, Decode, Encode)]
#[cfg_attr(test, derive(Clone, Default, PartialEq))]
pub struct FinalityVotes<Submitter> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a custom Default implementation to simplify creation.

@svyatonik svyatonik merged commit 0c4af28 into master Jun 10, 2020
Rialto Release automation moved this from Pull Requests to Done Jun 10, 2020
@svyatonik svyatonik deleted the finality-cache branch June 10, 2020 07:56
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* removeInMemoryStorage + extract Kovan stuff to runtime

* removed comment from the future

* remove redundant conversions

* remove redundant `u8 as usize`

* remove redundant `u8 as usize`

* Update modules/ethereum/src/mock.rs

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* use hex-literal in kovan config

* cargo fmt --all

* extracted insert_header

* cargo fmt --all

* finality cache

* cargo fmt --all

* cargo fmt --all

* impl Default for FinalityVotes

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <castano.ha@gmail.com>
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* removeInMemoryStorage + extract Kovan stuff to runtime

* removed comment from the future

* remove redundant conversions

* remove redundant `u8 as usize`

* remove redundant `u8 as usize`

* Update modules/ethereum/src/mock.rs

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

* use hex-literal in kovan config

* cargo fmt --all

* extracted insert_header

* cargo fmt --all

* finality cache

* cargo fmt --all

* cargo fmt --all

* impl Default for FinalityVotes

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Co-authored-by: Hernando Castano <castano.ha@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants