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.

Sign up for GitHub

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

Jump to bottom

Add test to demonstrate the failure scenario #1999

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Add test to demonstrate the failure scenario #1999

merged 5 commits into from
Oct 31, 2023

Conversation

rahulksnv
Copy link
Contributor

The change adds a test to show the failure scenario that caused #1812 to be rolled back (more context: #493 (comment))

Summary of the scenario:

  1. Node has finished downloading up to block 1000 from the peers, from the canonical chain.
  2. Peers are undergoing re-org around this time. One of the peers has switched to a non-canonical chain, announces block 1001 from that chain
  3. Node downloads 1001 from the peer, and tries to import which would fail (as we don't have the parent block 1000 from the other chain)

@rahulksnv rahulksnv mentioned this pull request Oct 24, 2023
Closed
2 tasks
@bkchr bkchr requested a review from a team October 24, 2023 08:29
dmitry-markin
dmitry-markin reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Is it possible to modify the test so that it's testing not the failure scenario but the expected behavior? E.g., if we remove [should_panic] and the test passes, that means that we fixed the bug?

@rahulksnv
Copy link
Contributor Author

Is it possible to modify the test so that it's testing not the failure scenario but the expected behavior? E.g., if we remove [should_panic] and the test passes, that means that we fixed the bug?

yes, if the issue is resolved, two changes would be needed:

  1. [should_panic] removed
  2. Minor change to the last part would be needed. The last part should result in no requests being sent out, test should check for that instead

dmitry-markin
dmitry-markin approved these changes Oct 25, 2023
View reviewed changes
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
@rahulksnv
Copy link
Contributor Author

@dmitry-markin Addressed comments, please merge if looks good, thanks

@dmitry-markin
Copy link
Contributor

dmitry-markin commented Oct 26, 2023 •
edited
Loading

@dmitry-markin Addressed comments, please merge if looks good, thanks

Looks good to me, but still need one more approving review to be able to merge.

@altonen altonen added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Oct 30, 2023
@dmitry-markin dmitry-markin mentioned this pull request Oct 31, 2023
Open
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4140199

@dmitry-markin
Copy link
Contributor

bot help

@command-bot
Copy link

command-bot bot commented Oct 31, 2023

Here's a link to docs

@dmitry-markin dmitry-markin merged commit d85c1d9 into paritytech:master Oct 31, 2023
109 of 110 checks passed
@github-actions github-actions bot mentioned this pull request Feb 19, 2024
Closed
@github-actions github-actions bot mentioned this pull request Mar 13, 2024
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@rahulksnv @dmitry-markin
Add test to demonstrate the failure scenario (paritytech#1999)
90c3334 
The change adds a test to show the failure scenario that caused paritytech#1812 to
be rolled back (more context:
paritytech#493 (comment))

Summary of the scenario:
1. Node has finished downloading up to block 1000 from the peers, from
the canonical chain.
2. Peers are undergoing re-org around this time. One of the peers has
switched to a non-canonical chain, announces block 1001 from that chain
3. Node downloads 1001 from the peer, and tries to import which would
fail (as we don't have the parent block 1000 from the other chain)

---------

Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@altonen altonen altonen approved these changes

Assignees
No one assigned
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
Networking
Status: Done ✅
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rahulksnv @dmitry-markin @paritytech-cicd-pr @altonen