Skip to content

introduce cluster slots constructor / new default_for_tests api #7349#7489

Merged
steviez merged 8 commits intoanza-xyz:masterfrom
puhtaytow:core-7349-0
Sep 8, 2025
Merged

introduce cluster slots constructor / new default_for_tests api #7349#7489
steviez merged 8 commits intoanza-xyz:masterfrom
puhtaytow:core-7349-0

Conversation

@puhtaytow
Copy link
Copy Markdown

@puhtaytow puhtaytow commented Aug 13, 2025 •
edited
Loading

Problem

#7314 addressed an issue where ClusterSlots is initialized and used before the a root bank has been set. This stems from the struct being built via a Default trait implementation #7349

Summary of Changes

  • Validator constructor now pass root bank and cluster info as params to cluster slot constructor instead of calling default.
  • ClusterSlots now has constructor which returns properly initialized instance
  • There is new default_for_tests api and private default implementation
  • Tests been updated accordingly, core and validator

@mergify mergify Bot requested a review from a team August 13, 2025 07:57
@puhtaytow puhtaytow marked this pull request as draft August 13, 2025 08:05
alexpyattaev
alexpyattaev reviewed Aug 13, 2025
View reviewed changes
Comment thread core/src/cluster_slots_service/cluster_slots.rs Outdated
@alexpyattaev
Copy link
Copy Markdown

alexpyattaev commented Aug 13, 2025

You will also probably need to effectively call

 cluster_slots.update(&root_bank, &cluster_info);

to fully initialize the service.

@puhtaytow puhtaytow force-pushed the core-7349-0 branch 4 times, most recently from c15e434 to 562ce87 Compare August 13, 2025 14:52
@puhtaytow puhtaytow marked this pull request as ready for review August 13, 2025 14:53
@puhtaytow
Copy link
Copy Markdown
Author

puhtaytow commented Aug 13, 2025

Hey @steviez if you could have a look when you have spare few minutes 🙏 you created the issue at the first point so it seem appropriate to tag you.

@puhtaytow puhtaytow changed the title introduce cluster slots constructor with epoch param #7349 introduce cluster slots constructor with root bank param #7349 Aug 13, 2025
steviez
steviez reviewed Sep 4, 2025
View reviewed changes
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

My writeup wasn't super thorough before as I was intending it to be something I'd come back to myself, but I have updated #7349 with a more concrete plan if you want to continue

Comment thread core/src/cluster_slots_service/cluster_slots.rs Outdated
@puhtaytow
Copy link
Copy Markdown
Author

puhtaytow commented Sep 4, 2025

My writeup wasn't super thorough before as I was intending it to be something I'd come back to myself, but I have updated #7349 with a more concrete plan if you want to continue

Surely. Let me read it thru and adjust the PR accordingly.

@puhtaytow puhtaytow force-pushed the core-7349-0 branch 2 times, most recently from c2d9c95 to b610368 Compare September 6, 2025 16:24
@puhtaytow puhtaytow marked this pull request as draft September 6, 2025 16:30
@puhtaytow puhtaytow force-pushed the core-7349-0 branch 3 times, most recently from 023cff6 to 0cbf0ca Compare September 7, 2025 13:11
@puhtaytow puhtaytow marked this pull request as ready for review September 7, 2025 13:14
@puhtaytow puhtaytow requested a review from a team as a code owner September 7, 2025 13:14
@puhtaytow
Copy link
Copy Markdown
Author

puhtaytow commented Sep 7, 2025

@steviez if you could have a look whenever you find time 🙏

@alexpyattaev alexpyattaev added the CI Pull Request is ready to enter CI label Sep 7, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Sep 7, 2025
@puhtaytow puhtaytow changed the title introduce cluster slots constructor with root bank param #7349 introduce cluster slots constructor / new default_for_tests api #7349 Sep 7, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 7, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.1%. Comparing base (1d17220) to head (bfe9293).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff            @@
##           master    #7489     +/-   ##
=========================================
- Coverage    83.1%    83.1%   -0.1%     
=========================================
  Files         811      811             
  Lines      357501   357526     +25     
=========================================
- Hits       297234   297226      -8     
- Misses      60267    60300     +33
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alexpyattaev
alexpyattaev previously approved these changes Sep 7, 2025
View reviewed changes
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

Lgtm, wdyt @steviez ?

Comment thread core/src/cluster_slots_service/cluster_slots.rs

// Intentionally private default function to disallow uninitialized construction
#[inline]
fn default() -> Self {
Copy link
Copy Markdown

@alexpyattaev alexpyattaev Sep 7, 2025

Choose a reason for hiding this comment

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

This is a neat trick!

steviez
steviez requested changes Sep 8, 2025
View reviewed changes
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

This hits what I was thinking in the writeup from #7349; think we can merge after addressing the two minor comments I raised

Comment thread core/src/cluster_slots_service/cluster_slots.rs Outdated
}

impl ClusterSlots {
#[inline]
Copy link
Copy Markdown

@steviez steviez Sep 8, 2025

Choose a reason for hiding this comment

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

nit: Let's leave inline off of these 3 new functions and let the compiler do what it wants; these are constructors and only called once at startup so not like these are getting called in a hot path or anything

Comment thread core/src/cluster_slots_service/cluster_slots.rs Outdated
}
}

/// Setup cluster slots with default values only for tests.
Copy link
Copy Markdown

@steviez steviez Sep 8, 2025

Choose a reason for hiding this comment

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

nit: I think we can drop this comment; it being DCOU + for_tests being in the name is fairly self-explanatory IMO

puhtaytow and others added 8 commits September 8, 2025 12:02
@puhtaytow @steviez
- introduce constructor of ClusterSlots with root bank and cluster info
e53390b 
- remove defaults derivation for ClusterSlots
- introduce private default function, to prevent uninitialized construction
- introduce new api / default_for_tests as replacement to previously used default

Co-authored-by: steviez <steven@anza.xyz>
@puhtaytow
migrate replay_stage tests to new ClusterSlots::default_for_tests()
34b91f1
@puhtaytow
migrate tvu tests to new ClusterSlots::default_for_tests()
3377382
@puhtaytow
migrate repair/ancestor_hashes_service tests to new ClusterSlots::def…
a486fda 
…ault_for_tests
@puhtaytow
migrate repair/repair_service tests to new ClusterSlots::default_for_…
bc7d5f0 
…tests()
@puhtaytow
migrate repair/serve_repair tests to new ClusterSlots::default_for_te…
168f832 
…sts()
@puhtaytow
migrate vote_simulator::simulate_vote to ClusterSlots::default_for_te…
27b99a5 
…sts()
@puhtaytow
migrate validator/admin_rpc_service tests to new ClusterSlots::defaul…
bfe9293 
…t_for_tests()
alexpyattaev
alexpyattaev approved these changes Sep 8, 2025
View reviewed changes
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

LGTM

@alexpyattaev alexpyattaev added the CI Pull Request is ready to enter CI label Sep 8, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Sep 8, 2025
steviez
steviez approved these changes Sep 8, 2025
View reviewed changes
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Thanks for doing this

@steviez steviez merged commit eb62c2e into anza-xyz:master Sep 8, 2025
54 checks passed
@steviez steviez linked an issue Sep 8, 2025 that may be closed by this pull request
Update ClusterSlots to initialize itself in constructor #7349
Closed
@puhtaytow
Copy link
Copy Markdown
Author

puhtaytow commented Sep 8, 2025

Thank you guys! :))

@puhtaytow puhtaytow deleted the core-7349-0 branch September 8, 2025 15:48
@steviez steviez mentioned this pull request Sep 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steviez steviez steviez approved these changes

@alexpyattaev alexpyattaev alexpyattaev approved these changes

Assignees

No one assigned

Labels

community need:merge-assist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update ClusterSlots to initialize itself in constructor

5 participants

@puhtaytow @alexpyattaev @codecov-commenter @steviez @anza-team