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

Check for parent of first ready block being on chain #1812

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Check for parent of first ready block being on chain #1812

merged 7 commits into from
Oct 10, 2023

Conversation

rahulksnv
Copy link
Contributor

@rahulksnv rahulksnv commented Oct 7, 2023 •
edited by dmitry-markin
Loading

When retrieving the ready blocks, verify that the parent of the first ready block is on chain. If the parent is not on chain, we are downloading from a fork. In this case, keep downloading until we have a parent on chain (common ancestor

Resolves #493.

@rahulksnv
Check for parent being on chain
578a99d 
When retrieveing the ready blocks, verify that the parent
of the first ready block is on chain. If the parent is not
on chain, we are downloading from a fork. In this case,
keep downloading until we have a parent on chain (common
ancestor)
@rahulksnv rahulksnv mentioned this pull request Oct 7, 2023
Closed
2 tasks
@dmitry-markin dmitry-markin 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 7, 2023
dmitry-markin
dmitry-markin approved these changes Oct 7, 2023
View reviewed changes
substrate/client/network/sync/src/blocks.rs Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
@dmitry-markin
Copy link
Contributor

@rahulksnv looks really nice!

@dmitry-markin dmitry-markin mentioned this pull request Oct 7, 2023
Closed
altonen
altonen approved these changes Oct 7, 2023
View reviewed changes
substrate/client/network/sync/src/blocks.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
@dmitry-markin
Copy link
Contributor

@rahulksnv Some tests are failing, could you look into that please? The important ones are in continuous-integration/gitlab-test-linux-stable, others are likely just a consequence of these failures.

@rahulksnv
Copy link
Contributor Author

@rahulksnv Some tests are failing, could you look into that please? The important ones are in continuous-integration/gitlab-test-linux-stable, others are likely just a consequence of these failures.

Looks like the first ready block is 1, and the parent (genesis block) is in InChainPruned state which was causing the test failure. Updated the check to include this state as well, which helps the test pass. But wanted to verify if the pruned state should indeed be included on the check?

@dmitry-markin
Copy link
Contributor

@rahulksnv Some tests are failing, could you look into that please? The important ones are in continuous-integration/gitlab-test-linux-stable, others are likely just a consequence of these failures.

Looks like the first ready block is 1, and the parent (genesis block) is in InChainPruned state which was causing the test failure. Updated the check to include this state as well, which helps the test pass. But wanted to verify if the pruned state should indeed be included on the check?

I think it's correct to include InChainPruned into the check, it just means that we don't store the state for that block, but the essential part is that the block is imported.

@rahulksnv
Copy link
Contributor Author

Please merge if all looks good

@dmitry-markin dmitry-markin merged commit 2b4b33d into paritytech:master Oct 10, 2023
9 of 10 checks passed
@dmitry-markin
Copy link
Contributor

Please merge if all looks good

All is done. Thanks for fixing this!

nazar-pc pushed a commit to autonomys/polkadot-sdk that referenced this pull request Oct 10, 2023
@rahulksnv @nazar-pc
Check for parent of first ready block being on chain (paritytech#1812)
e1f129a 
When retrieving the ready blocks, verify that the parent of the first
ready block is on chain. If the parent is not on chain, we are
downloading from a fork. In this case, keep downloading until we have a
parent on chain (common ancestor).

Resolves paritytech#493.

---------

Co-authored-by: Aaro Altonen <48052676+altonen@users.noreply.github.com>
(cherry picked from commit 2b4b33d)
@nazar-pc nazar-pc mentioned this pull request Oct 10, 2023
Merged
ordian added a commit that referenced this pull request Oct 12, 2023
@ordian
Merge branch 'master' into ao-disabling-statement-distribution
e333fa8 
* master: (33 commits)
  ci: set CI_IMAGE back to (now updated) .ci-unified (#1854)
  ci: bump ci image to rust 1.73.0 (#1830)
  Refactor Identity to benchmark v2 (#1838)
  PVF worker: bump landlock, update ABI docs (#1850)
  Xcm emulator nits (#1649)
  Fixes path issue in derive-impl (#1823)
  upgrade to macro_magic 0.4.3 (#1832)
  Use safe math when pruning statuses (#1835)
  remote-ext: fix state download stall on slow connections and reduce memory usage (#1295)
  Update testnet bootnode dns name (#1712)
  [FRAME] Warn on unchecked weight witness (#1818)
  [xcm] Use `Weight::MAX` for `reserve_asset_deposited`, `receive_teleported_asset` benchmarks (#1726)
  Update bridges subtree (#1803)
  Check for parent of first ready block being on chain (#1812)
  Make CheckNonce refuse transactions signed by accounts with no providers (#1578)
  Fix Asset Hub collator crashing when starting from genesis (#1788)
  Mixnet integration (#1346)
  [xcm-emulator] Decouple the `AccountId` type from `AccountId32` (#1458)
  Treasury spends various asset kinds (#1333)
  chore: bump zombienter version (#1806)
  ...
@rahulksnv rahulksnv mentioned this pull request Oct 19, 2023
Closed
@nazar-pc nazar-pc mentioned this pull request Oct 20, 2023
Open
nazar-pc added a commit to autonomys/polkadot-sdk that referenced this pull request Oct 20, 2023
@nazar-pc
Revert "Check for parent of first ready block being on chain (parityt…
892bf8e 
…ech#1812)"

This reverts commit e1f129a.
@nazar-pc nazar-pc mentioned this pull request Oct 20, 2023
Merged
1 task
dmitry-markin added a commit that referenced this pull request Oct 20, 2023
@dmitry-markin
Revert "Check for parent of first ready block being on chain (#1812)"
8a77665 
This reverts commit 2b4b33d.
@dmitry-markin dmitry-markin mentioned this pull request Oct 20, 2023
Merged
bkchr pushed a commit that referenced this pull request Oct 20, 2023
@dmitry-markin
Revert "Check for parent of first ready block being on chain (#1812)" (…
69c986f 
…#1950)

This reverts #1812 until
we know why it causes syncing issues reported in
autonomys/subspace#2122.
@rahulksnv rahulksnv mentioned this pull request Oct 24, 2023
Merged
rahulksnv added a commit to autonomys/polkadot-sdk that referenced this pull request Oct 24, 2023
@rahulksnv
Revert "Revert "Check for parent of first ready block being on chain (p…
89268c3 
…aritytech#1812)""

This reverts commit 892bf8e.
dmitry-markin added a commit that referenced this pull request Oct 31, 2023
@rahulksnv @dmitry-markin
Add test to demonstrate the failure scenario (#1999)
d85c1d9 
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)

---------

Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@kiltbot kiltbot mentioned this pull request Nov 7, 2023
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@rahulksnv @altonen
Check for parent of first ready block being on chain (paritytech#1812)
2151bf6 
When retrieving the ready blocks, verify that the parent of the first
ready block is on chain. If the parent is not on chain, we are
downloading from a fork. In this case, keep downloading until we have a
parent on chain (common ancestor).

Resolves paritytech#493.

---------

Co-authored-by: Aaro Altonen <48052676+altonen@users.noreply.github.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@dmitry-markin
Revert "Check for parent of first ready block being on chain (parityt…
e07841a 
…ech#1812)" (paritytech#1950)

This reverts paritytech#1812 until
we know why it causes syncing issues reported in
autonomys/subspace#2122.
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>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
@serban300 @bkchr
Add tool for auto generating runtime code from metadata (#1812)
b283257 
* Create CLI tool for generating indirect runtimes code

* Use the generated runtime for rialto parachain

* Avoid autogenerated files when executing cargo spellcheck

* Fix clippy warning
This was referenced Jun 5, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altonen altonen altonen approved these changes

@dmitry-markin dmitry-markin dmitry-markin 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.

Node can't sync with the network
3 participants
@rahulksnv @dmitry-markin @altonen