fix: correct BLS scheme setting in MigrateLegacyDiffs() when nVersion is present

[UdjinM6]

Issue being fixed or feature implemented

BLS public key legacy scheme is only being set when nVersion field is missing and needed to be auto-filled, instead of being set whenever a pubKeyOperator field is present.

What was done?

Moved the SetLegacy() call outside the nVersion check.

How Has This Been Tested?

Migrate on mainnet at block 2367886

Breaking Changes

n/a

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

A closing brace is repositioned in src/evo/deterministicmns.cpp, moving it to occur before a subsequent Field_pubKeyOperator check rather than within the earlier conditional block. This structural adjustment does not modify any logic, control flow, or error handling—the behavior of the code remains identical.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Single-file change involving only brace repositioning
• No logic modifications, control-flow changes, or semantic alterations
• Straightforward structural adjustment with no behavioral impact

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: correcting BLS scheme setting in MigrateLegacyDiffs() when nVersion is present, which directly relates to the code repositioning fix described in the changeset.
Description check	✅ Passed	The description is directly related to the changeset, explaining the issue being fixed (BLS scheme not set when pubKeyOperator is present), the solution (moving SetLegacy() outside nVersion check), and testing performed (mainnet migration at block 2367886).

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 368eebb and 65c33ea.

📒 Files selected for processing (1)

• src/evo/deterministicmns.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/evo/deterministicmns.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

• src/evo/deterministicmns.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.

Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.

📚 Learning: 2025-01-07T18:50:44.838Z

Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.

Applied to files:

• src/evo/deterministicmns.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64_nowallet-build / Build source

🔇 Additional comments (1)

src/evo/deterministicmns.cpp (1)

1493-1503: Fix correctly ensures BLS legacy scheme is set whenever pubKeyOperator is present.

The change correctly moves the SetLegacy() call outside the nVersion check. Previously, the legacy scheme was only set when nVersion was missing and auto-filled. Now it's set whenever pubKeyOperator is present in the diff, which is the correct behavior since the BLS key needs its legacy flag set regardless of whether nVersion was already in the diff or was just added.

Note: The AI-generated summary incorrectly states "behavior remains identical" when this is actually a bug fix that changes behavior—SetLegacy() now executes in additional cases where nVersion was already present in the diff.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[UdjinM6]

This structural adjustment does not modify any logic, control flow, or error handling—the behavior of the code remains identical.

This can't be true... Are you sure @coderabbitai?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the diff for the changed file to see the actual changes
gh pr diff 6961 --repo dashpay/dash

Length of output: 1000

---

@UdjinM6 You're absolutely right, and I apologize for the incorrect AI summary. Looking at the diff, this is a behavior change:

Before: The pubKeyOperator.SetLegacy() call was nested inside the outer conditional block, meaning it would only execute when that outer condition was satisfied. Based on your PR description, this meant SetLegacy() was only called when the nVersion field was missing and being auto-filled.

After: The code is unindented and moved outside that conditional block. Now SetLegacy() will be called whenever pubKeyOperator field is present in the stateDiff.fields, regardless of whether the nVersion field existed or was auto-filled.

This is exactly as you described in the PR summary: a fix that ensures the BLS legacy scheme is set correctly for all cases where pubKeyOperator is present, not just in the narrow case where nVersion was missing.

The AI-generated summary incorrectly characterized this as having "identical behavior"—that statement should be disregarded. This is a genuine bug fix that changes the conditions under which SetLegacy() is invoked.

---

🧠 Learnings used

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

---

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

---

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.

---

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

---

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.

[knst]

LGTM 65c33ea

[kwvg]

utACK 65c33ea