Skip to content

feat: add error log when D_P < n_P (#1505)#1506

Merged
gilescope merged 4 commits into
mainfrom
ozgb-d-param-error
May 14, 2026
Merged

feat: add error log when D_P < n_P (#1505)#1506
gilescope merged 4 commits into
mainfrom
ozgb-d-param-error

Conversation

@ozgb

@ozgb ozgb commented May 13, 2026

Copy link
Copy Markdown
Contributor

Overview

Fixes #1505

For context, see discussion in #1481

When the D parameter is lower that the number of permissioned candidates, no candidate has a guaranteed seat in the session committee. This can lead to liveness issues if a small number of nodes are repeatedly selected for multiple seats in the committee - if one of those nodes goes offline, so does the chain.

We should add error logging when D_P < n_P:

  • At startup
  • On session change

This will alert network operators that there is a configuration issue that needs addressing.

🗹 TODO before merging

  • Ready

📌 Submission Checklist

  • All commits are signed off (git commit -s) for the DCO
  • Changes are backward-compatible (or flagged if breaking)
  • Pull request description explains why the change is needed
  • Self-reviewed the diff
  • I have included a change file, or skipped for this reason:
  • If the changes introduce a new feature, I have bumped the node minor version
  • Update documentation (if relevant)
  • Updated AGENTS.md if build commands, architecture, or workflows changed
  • No new todos introduced

🧪 Testing Evidence

Please describe any additional testing aside from CI:

  • Additional tests are provided (if possible)

🔱 Fork Strategy

  • Node Runtime Update
  • Node Client Update
  • Other:
  • N/A

Links

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb ozgb requested a review from a team as a code owner May 13, 2026 10:39
ozgb added 2 commits May 13, 2026 11:39
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb ozgb mentioned this pull request May 13, 2026
Comment thread changes/runtime/added/d-param-permissioned-candidates-check.md

@gilescope gilescope left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bin the start up check. Its a lot of code and when starting from genesis d-param / authorities for the session are from the chainspec so I'm not sure it would be comparing the right thing. The per session error is the stonger of the two checks anyway.

Comment thread node/src/d_param_check.rs Outdated
@LGLO

LGLO commented May 13, 2026

Copy link
Copy Markdown
Contributor

I have a feeling that the problems could be solved by modification to selection algorithm:

  • if R = 0, then ignore P, give one slot to each candidate (shuffled).

Today I see this PR. Yesterday I was reviewing few pages of documentation by Jon. Selection algorithm is under our control and can solve both.

Lots of code for little improvement over just the session change check

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb

ozgb commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

I have a feeling that the problems could be solved by modification to selection algorithm:

* if R = 0, then ignore P, give one slot to each candidate (shuffled).

Today I see this PR. Yesterday I was reviewing few pages of documentation by Jon. Selection algorithm is under our control and can solve both.

Agreed on this - the fact that adding a permissioned candidate impacts liveness is not intuitive

Suggest we keep this error print for now - and propose a new PR with those selection changes, or alternative changes that fix the D < P liveness issues

@gilescope

Copy link
Copy Markdown
Contributor

Agreed - two separate PRs seems best (that way we have a rollback path)

@gilescope gilescope left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@gilescope gilescope enabled auto-merge May 14, 2026 08:24
@gilescope gilescope added this pull request to the merge queue May 14, 2026
Merged via the queue into main with commit 837e39b May 14, 2026
53 of 55 checks passed
@gilescope gilescope deleted the ozgb-d-param-error branch May 14, 2026 14:30
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.

Add error logging with D_P < n_P (liveness issue)

4 participants