Skip to content

DHT bootnodes: get randomness from the next epoch descriptor#8405

Merged
dmitry-markin merged 6 commits into
masterfrom
dm-dht-bootnodes-runtime-optimization
May 6, 2025
Merged

DHT bootnodes: get randomness from the next epoch descriptor#8405
dmitry-markin merged 6 commits into
masterfrom
dm-dht-bootnodes-runtime-optimization

Conversation

@dmitry-markin

Copy link
Copy Markdown
Contributor

Use runtime calls to get epoch randomness only on startup, and later get it from the next epoch descriptor at the first block of every epoch.

Resolves #8377.

@dmitry-markin dmitry-markin added R0-no-crate-publish-required The change does not require any crates to be re-published. T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus. labels May 2, 2025
@dmitry-markin dmitry-markin requested review from a team and lexnv May 2, 2025 10:10
Comment thread cumulus/client/bootnodes/src/advertisement.rs Outdated
@@ -207,9 +207,20 @@ impl BootnodeAdvertisement {
async fn handle_import_notification(&mut self, header: RelayHeader) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some edge case that is not covered is when the notification stream skipped blocks (because of major sync). Then you would maybe advertise too long with an invalid key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, stop_providing is a local-only action only (Kademlia DHT doesn't have a way to tell target nodes we are not providing the key anymore), so remote nodes will advertise every key until it expires in 48 hours anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was not what I wanted to say :) The problem I raised there is when you for example sync across an epoch change, but the node was doing this in "major sync mode". Then you would not get a notification for these blocks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation, this makes sense. How common is such situation except during the initial sync?

@dmitry-markin dmitry-markin enabled auto-merge May 5, 2025 09:35
@paritytech-workflow-stopper

Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/14833176530
Failed job name: test-linux-stable

@lexnv

lexnv commented May 5, 2025

Copy link
Copy Markdown
Contributor

It looks like an unrelated test to these changes is failing (since we have only modified cumulus/client/bootnodes/src/advertisement.rs). Might be worth rerunning and raising an issue about it? 🤔

──── TRY 1 STDERR:       sc-service-test client::returns_status_for_pruned_blocks
thread 'client::returns_status_for_pruned_blocks' panicked at substrate/client/service/test/src/client/mod.rs:1845:5:
assertion `left == right` failed
  left: InChainWithState
 right: InChainPruned

@skunert

skunert commented May 5, 2025

Copy link
Copy Markdown
Contributor

It looks like an unrelated test to these changes is failing (since we have only modified cumulus/client/bootnodes/src/advertisement.rs). Might be worth rerunning and raising an issue about it? 🤔

──── TRY 1 STDERR:       sc-service-test client::returns_status_for_pruned_blocks
thread 'client::returns_status_for_pruned_blocks' panicked at substrate/client/service/test/src/client/mod.rs:1845:5:
assertion `left == right` failed
  left: InChainWithState
 right: InChainPruned

Yes we can rerun, seems to be known flaky :(

#[test]
// FIXME: https://github.com/paritytech/polkadot-sdk/issues/48
fn returns_status_for_pruned_blocks() {

@dmitry-markin dmitry-markin added this pull request to the merge queue May 6, 2025
Merged via the queue into master with commit f2835be May 6, 2025
238 of 249 checks passed
@dmitry-markin dmitry-markin deleted the dm-dht-bootnodes-runtime-optimization branch May 6, 2025 13:53
@github-project-automation github-project-automation Bot moved this to Blocked ⛔️ in Networking May 6, 2025
castillax pushed a commit that referenced this pull request May 12, 2025
Use runtime calls to get epoch randomness only on startup, and later get
it from the next epoch descriptor at the first block of every epoch.

Resolves #8377.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
Use runtime calls to get epoch randomness only on startup, and later get
it from the next epoch descriptor at the first block of every epoch.

Resolves #8377.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus.

Projects

Status: Blocked ⛔️

Development

Successfully merging this pull request may close these issues.

Eliminate extra runtime calls used for DHT bootnode advertisement

4 participants