Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix some problems with prove_warp_sync #8037

Merged
4 commits merged into from
Feb 5, 2021
Merged

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Feb 3, 2021

No description provided.

@expenses expenses added A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 3, 2021
@@ -289,7 +289,7 @@ pub fn prove_warp_sync<Block: BlockT, B: BlockchainBackend<Block>>(

let mut index = *header.number();
while index <= end_number {
if max_fragment_limit.map(|limit| result.len() <= limit).unwrap_or(false) {
if max_fragment_limit.map(|limit| result.len() >= limit).unwrap_or(false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 , sorry.

client/finality-grandpa/src/finality_proof.rs Show resolved Hide resolved
if result.last().as_ref().map(|head| head.header.number()) != Some(&end_number) {
let at_limit = max_fragment_limit.map(|limit| result.len() >= limit).unwrap_or(false);

if !at_limit && result.last().as_ref().map(|head| head.header.number()) != Some(&end_number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 when at_limit we probably had twice the fragment
Actually realize this condition was not really simple to read, the point was just add the last finalized block if not already added, so I added a code suggestion bellow.

client/finality-grandpa/src/finality_proof.rs Show resolved Hide resolved
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

LGTM

expenses and others added 3 commits February 4, 2021 12:02
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
…tytech/substrate into ashley-fix-prove-warp-sync-problems
@expenses
Copy link
Contributor Author

expenses commented Feb 5, 2021

bot merge

@ghost
Copy link

ghost commented Feb 5, 2021

Trying merge.

@ghost ghost merged commit cc71cca into master Feb 5, 2021
@ghost ghost deleted the ashley-fix-prove-warp-sync-problems branch February 5, 2021 11:53
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants