Skip to content

Hoist determine_block_hash out of asyncio.gather#3365

Open
Yupsecous wants to merge 2 commits into
latent-to:stagingfrom
Yupsecous:fix/yupsecous/3129-determine-block-hash-hoist
Open

Hoist determine_block_hash out of asyncio.gather#3365
Yupsecous wants to merge 2 commits into
latent-to:stagingfrom
Yupsecous:fix/yupsecous/3129-determine-block-hash-hoist

Conversation

@Yupsecous
Copy link
Copy Markdown

Resolve the block hash once before three asyncio.gather sites (get_all_subnets_info, AsyncMetagraph._process_root_weights and _apply_extra_info) and fan out on block_hash, removing one redundant concurrent get_block_hash RPC per site. Behavior unchanged; adds regression tests asserting get_block_hash is awaited once.

Refs #3129

Requirements for Contributing a Performance Improvement

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must only affect performance of existing functionality
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution.

Description of the Change

Quantitative Performance Benefits

Possible Drawbacks

Verification Process

Applicable Issues

Release Notes

Branch Acknowledgement

[ ] I am acknowledging that I am opening this branch against staging

Resolve the block hash once before three asyncio.gather sites
(get_all_subnets_info, AsyncMetagraph._process_root_weights and
_apply_extra_info) and fan out on block_hash, removing one redundant
concurrent get_block_hash RPC per site. Behavior unchanged; adds
regression tests asserting get_block_hash is awaited once.

Refs latent-to#3129
Copy link
Copy Markdown
Contributor

@thewhaleking thewhaleking left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@thewhaleking
Copy link
Copy Markdown
Contributor

thewhaleking commented Jun 1, 2026

Flaky test flaking. Should be fixed by #3367

Copy link
Copy Markdown
Collaborator

@basfroman basfroman left a comment

Choose a reason for hiding this comment

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

Hey, looked at this closer - AsyncSubstrateInterface.get_block_hash already goes through CachedFetcher which deduplicates concurrent calls via an inflight Future dict (_inflight). Meaning when two coroutines in a gather both call get_block_hash(123), only one RPC actually fires, the second one awaits the first's Future.

So the scenario this PR prevents (duplicate chain_getBlockHash RPCs from a single gather) can't actually happen at the transport layer. The hoisted determine_block_hash resolves to the same single RPC either way.

The change isn't harmful and arguably reads better, but it's not a performance fix - it's cosmetic. Wanted to flag there's no measurable gain to claim here.

If we still want it as a readability refactor - totally fine, just worth reframing the commit message and removing the "Refs #3129" perf issue link (or closing that issue as already-resolved by the caching layer).

@thewhaleking
Copy link
Copy Markdown
Contributor

Hey, looked at this closer - AsyncSubstrateInterface.get_block_hash already goes through CachedFetcher which deduplicates concurrent calls via an inflight Future dict (_inflight). Meaning when two coroutines in a gather both call get_block_hash(123), only one RPC actually fires, the second one awaits the first's Future.

So the scenario this PR prevents (duplicate chain_getBlockHash RPCs from a single gather) can't actually happen at the transport layer. The hoisted determine_block_hash resolves to the same single RPC either way.

The change isn't harmful and arguably reads better, but it's not a performance fix - it's cosmetic. Wanted to flag there's no measurable gain to claim here.

If we still want it as a readability refactor - totally fine, just worth reframing the commit message and removing the "Refs #3129" perf issue link (or closing that issue as already-resolved by the caching layer).

I think it makes more sense to drop this PR, and just use ASI 2.1.0+, as it accomplishes the same goal while being more readable (e.g. keeping the current code in staging)

@Yupsecous
Copy link
Copy Markdown
Author

Yupsecous commented Jun 2, 2026

Thanks both — you're right. I'd missed that get_block_hash already collapses concurrent same-block calls through CachedFetcher's in-flight future map, so the duplicate never reaches the transport layer and this hoist is cosmetic rather than a perf win. Agreed that keeping the current code in staging (and relying on ASI 2.1.0+) is cleaner. Closing this out — thanks for the careful review. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants