-
Notifications
You must be signed in to change notification settings - Fork 927
Add --semi-supernode
support
#8254
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.
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
Conversation
Co-authored-by: pawanjay176 <[email protected]>
0ffed11
to
a8580d0
Compare
a8580d0
to
0932f36
Compare
Squashed commit of the following: commit 7767a2e Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:47:40 2025 +1100 More test fixes and update help text. commit 0932f36 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:14:46 2025 +1100 Add tests for restoring custody context from persisted. commit 5cc3186 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:07:47 2025 +1100 Implement semi-supernode. Co-authored-by: pawanjay176 <[email protected]>
Squashed commit of the following: commit 7767a2e Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:47:40 2025 +1100 More test fixes and update help text. commit 0932f36 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:14:46 2025 +1100 Add tests for restoring custody context from persisted. commit 5cc3186 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:07:47 2025 +1100 Implement semi-supernode. Co-authored-by: pawanjay176 <[email protected]>
… validator should have `validator_custody_at_head` set to 0.
let is_semi_supernode = parse_flag(cli_args, "semi-supernode"); | ||
|
||
client_config.chain.node_custody_type = if is_supernode { | ||
client_config.network.subscribe_all_data_column_subnets = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed subscribe_all_data_column_subnets
is redundant in network config, because NetworkGlobals
already has the sampling columns when initialised, but I can do it in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty straight forward to remove: jimmygchen@e623eb6
Squashed commit of the following: commit 45a4315 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 23:15:58 2025 +1100 Add tests for flags commit 15569bc Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 22:40:49 2025 +1100 Revert some changes to preserve existing behaviour. Full node with no validator should have `validator_custody_at_head` set to 0. commit 7767a2e Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:47:40 2025 +1100 More test fixes and update help text. commit 0932f36 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:14:46 2025 +1100 Add tests for restoring custody context from persisted. commit 5cc3186 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:07:47 2025 +1100 Implement semi-supernode. Co-authored-by: pawanjay176 <[email protected]>
Squashed commit of the following: commit 45a4315 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 23:15:58 2025 +1100 Add tests for flags commit 15569bc Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 22:40:49 2025 +1100 Revert some changes to preserve existing behaviour. Full node with no validator should have `validator_custody_at_head` set to 0. commit 7767a2e Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:47:40 2025 +1100 More test fixes and update help text. commit 0932f36 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:14:46 2025 +1100 Add tests for restoring custody context from persisted. commit 5cc3186 Author: Jimmy Chen <[email protected]> Date: Tue Oct 21 16:07:47 2025 +1100 Implement semi-supernode. Co-authored-by: pawanjay176 <[email protected]>
Some devnet-3 testing:
|
This comment was marked as outdated.
This comment was marked as outdated.
Some required checks have failed. Could you please take a look @jimmygchen? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a neat idea. Its minimal changes so we can be confident that it doesn't mess with anything before the release. I have also tested this on devnet 3 and am convinved it works as advertised.
Just a few questions
spec: &ChainSpec, | ||
) -> Self { | ||
let cgc_override = node_custody_type.get_custody_count_override(spec); | ||
if let Some(cgc_from_cli) = cgc_override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just log a warn or explicitly refuse to start here and ask the user to resync if they want to change the mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed earlier, we'll trigger backfill in this case for better UX. Mind if I do it in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
ah I gotta pull unstable - the test are failing because of some new change from unstable. |
This pull request has been removed from the queue for the following reason: The pull request conflicts with at least one pull request ahead in queue. There is nothing you can do for now. If the pull request ahead in the queue is merged, this pull request will become conflicting and you'll have to update it. |
Are we waiting for custody backfill to merge before merging this? Don't we want to add the feature to start custody backfill when the node changes to a semi-supernode/supernode? |
if a node's cgc increases on start up, we just need two things for custody backfill to do its thing
|
@mergify requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Oh yeah I've discussed with Pawan and going to make a follow up PR so it's bit easier to review |
Open PRs to include for the release - #7907 - #8247 - #8251 - #8253 - #8254 - #8265 - #8269 - #8266 Co-Authored-By: Jimmy Chen <[email protected]> Co-Authored-By: Jimmy Chen <[email protected]>
Issue Addressed
Addresses #8218
A simplified version of #8241 for the initial release.
I've tried to minimise the logic change in this PR, although introducing the
NodeCustodyType
enum still result in quite a bit a of diff, but the actual logic change inCustodyContext
is quite small.The main changes are in the
CustdoyContext
structcombiningvalidator_custody_count
andcurrent_is_supernode
fields into a singlecustody_group_count_at_head
field. We persist the cgc of the initial cli values into thecustody_group_count_at_head
field and only allow for increase (same behaviour as before).validator_custody_count
value if either flag--supernode
or--semi-supernode
is used; otherwise leave it as the existing default0
. Most other logic remains unchanged.All existing validator custody unit tests are still all passing, and I've added additional tests to cover semi-supernode, and restoring
CustodyContext
from disk.Note: I've added a
WARN
if the user attempts to switch to a--semi-supernode
or--supernode
- this currently has no effect, but once @eserilev column backfill is merged, we should be able to support this quite easily.Things to test