Skip to content

Conversation

@jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Nov 10, 2025

Issue Addressed

Take 2 of #8390.

Fixes the race condition properly instead of propagating the error. I think this is a better alternative, and doesn't seem to look that bad.

Proposed Changes

  • Lift node id loading or generation from NetworkService startup to the ClientBuilder, so that it can be used to compute custody columns for the beacon chain without waiting for Network bootstrap.

I've considered and implemented a few alternatives:

  1. passing node_id to beacon chain builder and compute columns when creating CustodyContext. This approach isn't good for separation of concerns and isn't great for testability
  2. passing ordered_custody_groups to beacon chain. CustodyContext only uses this to compute ordered custody columns, so we might as well lift this logic out, so we don't have to do error handling in CustodyContext construction. Less tests to update;.

@jimmygchen jimmygchen force-pushed the fix-custody-context-race branch from 8d310a5 to 770e1e5 Compare November 10, 2025 07:43
@michaelsproul michaelsproul changed the title Fix custdoy context initialization race condition that caused panic Fix custody context initialization race condition that caused panic Nov 10, 2025
@jimmygchen jimmygchen force-pushed the fix-custody-context-race branch 2 times, most recently from 60e06f8 to 0e80706 Compare November 12, 2025 05:03
@jimmygchen jimmygchen marked this pull request as ready for review November 12, 2025 05:36
@jimmygchen jimmygchen requested a review from jxs as a code owner November 12, 2025 05:36
@jimmygchen jimmygchen added bug Something isn't working ready-for-review The code is ready for review v8.0.1 Cheeky patch release for Fulu labels Nov 12, 2025
@jimmygchen jimmygchen force-pushed the fix-custody-context-race branch from 28c2073 to 935db19 Compare November 12, 2025 05:57
@jimmygchen jimmygchen changed the base branch from unstable to release-v8.0 November 12, 2025 05:57
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks a lot nicer without the expects, and the extra plumbing for initialisation doesn't seem too bad. Looks good!

@michaelsproul michaelsproul added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Nov 16, 2025
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed under-review A reviewer has only partially completed a review. labels Nov 17, 2025
@michaelsproul michaelsproul removed the ready-for-review The code is ready for review label Nov 17, 2025
@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Nov 17, 2025
@mergify mergify bot added the queued label Nov 17, 2025
mergify bot added a commit that referenced this pull request Nov 17, 2025
@mergify mergify bot merged commit af1d9b9 into sigp:release-v8.0 Nov 17, 2025
36 checks passed
@mergify mergify bot removed the queued label Nov 17, 2025
mergify bot pushed a commit that referenced this pull request Nov 20, 2025
This hot fix release includes the following fixes:
* #8388
* #8406
* #8391
* #8413


  


Co-Authored-By: Jimmy Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready-for-merge This PR is ready to merge. v8.0.1 Cheeky patch release for Fulu

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants