refactor: multiple kernel-0 preparations#7299
Conversation
…dex dependency for linking
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 0bcb345) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces CNode-based peer context with string peer identifiers across governance processing (ProcessObject, AddGovernanceObjectInternal, AddGovernanceObject, ProcessVote) and updates call sites and relays accordingly. Moves CDeterministicMN::ToJson() out of evo/deterministicmns.cpp and adds an implementation in rpc/evo_util.cpp. Refactors ChainLockSig: type and serialization moved into chainlock/chainlock.{h,cpp}, VerifyChainLock relocated/rewritten, and related headers/forward-declarations updated. Miscellaneous include/forward-declaration adjustments across LLMQ/DKG/observer, mempool null-checks in chainstate setup, a lint circular-dependency exception, and small logging/format tweaks. Sequence Diagram(s)sequenceDiagram
participant Peer as Peer (CNode / peer_str)
participant Net as NetGovernance (ProcessMessage)
participant GMan as CGovernanceManager
participant Relay as Network Relay
Note over Peer,Net: Incoming governance messages
Peer->>Net: send governance object / vote
alt governance object
Net->>GMan: ProcessObject(peer.GetLogString(), hash, govobj)
GMan->>GMan: Validate object (syntax, sigs)
alt valid
GMan->>GMan: AddGovernanceObject(govobj, peer_str)
GMan->>GMan: AddGovernanceObjectInternal(insert_obj, peer_str)
GMan->>Relay: Relay object to peers
else invalid
GMan-->>Net: reject / increase ban score
end
else vote
Net->>GMan: ProcessVote(vote, exception, hashToRequest)
GMan->>GMan: Validate vote and link to object
alt vote accepted
GMan->>Relay: Relay vote
else missing object / ignore
GMan-->>Net: request object / ignore
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/node/chainstate.cpp`:
- Around line 251-257: The mempool teardown call is happening after llmq_ctx is
reset which can leave stale pointers; move the mempool->DisconnectManagers()
call so it runs before destroying llmq_ctx (and chain_helper) to ensure managers
are disconnected while llmq_ctx is still valid. Concretely, call
mempool->DisconnectManagers() (guarded by if (mempool)) prior to resetting
llmq_ctx (and chain_helper if relevant), then proceed to reset dmnman and the
other unique_ptrs.
In `@src/rpc/evo_util.cpp`:
- Around line 151-159: The code assumes collateralOutpoint.n is a valid index
into collateralTx->vout after g_txindex->FindTx; add a bounds check to guard
against out-of-range access: after confirming collateralTx is non-null, verify
collateralOutpoint.n < collateralTx->vout.size() before using
collateralTx->vout[collateralOutpoint.n]; if the index is invalid, skip
extracting the destination (or optionally log/warn) and do not call
ExtractDestination or obj.pushKV to avoid undefined behavior. Ensure the check
is applied around the existing ExtractDestination and obj.pushKV calls
referencing collateralTx and collateralOutpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b247bb85-790b-4d2a-855c-26acc58fef78
📒 Files selected for processing (15)
src/evo/deterministicmns.cppsrc/evo/dmnstate.cppsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/governance/object.cppsrc/governance/signing.cppsrc/governance/signing.hsrc/llmq/dkgsessionmgr.cppsrc/llmq/dkgsessionmgr.hsrc/llmq/observer.cppsrc/node/chainstate.cppsrc/qt/masternodemodel.cppsrc/rpc/evo_util.cpptest/lint/lint-circular-dependencies.py
knst
force-pushed
the
kernel-0-preps
branch
2 times, most recently
from
daa1cda to
45d24b1
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Low-risk header dependency refactor PR (kernel-0 prep). No blocking issues found. Codex's two 'blocking' findings on InitializeCurrentBlockTip are false positives — they reference src/validationinterface.cpp and src/init.cpp, neither of which this PR modifies (those changes live in earlier develop commits dc8c6cf / 863ee6b). Claude's findings about the new "nullptr" magic-string sentinel for peer_str are real and worth addressing; the rest are nits.
Reviewed commit: 45d24b1
🟡 1 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
💬 nitpick: Templated InitializeHandlers keeps chainparams.h/util/helpers.h pinned in the header
src/llmq/dkgsessionmgr.h (lines 106-117)
The new // TODO: move it to cpp file and drop chainparams.h and helpers.h acknowledges that the header still pulls chainparams.h and util/helpers.h only because InitializeHandlers is defined inline. Since this PR is specifically about untangling header dependencies, it would have been a natural place to finish the job — HandlerFn is only instantiated from one cpp (LLMQ context setup), so converting the body to a non-template helper or providing an explicit instantiation in the cpp would let those includes drop now. Not blocking; the TODO will likely outlive this PR.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/governance/governance.h`:
- [SUGGESTION] line 324: Use std::optional<std::string> instead of the literal "nullptr" sentinel for peer_str
The decoupling from `const CNode*` to `const std::string& peer_str` is a sensible step, but encoding "no peer" as the literal string `"nullptr"` propagates a code smell through the codebase. The default parameter (`= "nullptr"`) and explicit call sites in `governance.cpp:834` and `signing.cpp:166` mean a forgotten or self-originated call silently logs `received from peer nullptr`, which reads like C jargon leaking into operator-facing logs. `std::optional<std::string>` (or `std::string_view` with empty meaning "unknown") would express intent properly and let the log formatter render the absent case meaningfully (e.g. `"<local>"` or `"<postponed>"`). It also avoids subtle bugs if a future caller passes the literal `"None"` / empty string and breaks log grep patterns. This finding subsumes Claude's separate nit on the log line at governance.cpp:834.
| class CBlockIndex; | ||
| class CSporkManager; | ||
| class uint256; |
There was a problem hiding this comment.
💬 Nitpick: Redundant forward declaration of uint256
#include <uint256.h> at line 12 already brings in the full type, so class uint256; at line 16 is redundant. uint256 is also used as a complete type later in the file (member blockHash, return values, equality comparisons), so the forward declaration is misleading about the include dependency. Drop it.
💡 Suggested change
| class CBlockIndex; | |
| class CSporkManager; | |
| class uint256; | |
| class CBlockIndex; | |
| class CSporkManager; |
source: ['claude']
| @@ -248,7 +250,9 @@ void DashChainstateSetupClose(std::unique_ptr<CChainstateHelper>& chain_helper, | |||
| { | |||
| chain_helper.reset(); | |||
| llmq_ctx.reset(); | |||
| mempool->DisconnectManagers(); | |||
| if (mempool) { | |||
| mempool->DisconnectManagers(); | |||
| } | |||
| dmnman.reset(); | |||
There was a problem hiding this comment.
💬 Nitpick: Document that mempool may be nullptr in DashChainstateSetup{,Close}
The added if (mempool) guards make DashChainstateSetup and DashChainstateSetupClose internally consistent (these are the only mempool-> derefs in the functions). However, the function signatures and headers don't communicate that nullptr is a supported argument, so future callers may not realize the chainstate-only construction path is intended. A one-line comment in the header (e.g. // mempool may be nullptr for chainstate-only setup) would make the contract obvious and reduce the risk of future regressions reintroducing unguarded derefs.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
No blockers from me on the current head; remaining comments are advisory suggestions/nits.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Low-risk dependency-untangling refactor for kernel-0 prep: moving CDeterministicMN::ToJson, VerifyChainLock, and ChainLockSig between TUs, swapping CNode* for std::string in governance log paths, and adding null-mempool guards. Core logic is preserved. The only valid concerns are minor readability/log-quality items. The codex 'blocking' finding about dust protection is out of scope — that code is not in this PR's diff (codex compared against a stale local develop).
Reviewed commit: 45d24b1
Fresh dispatcher rerun for this queue item. Same-SHA automated reviews already existed, so I am posting this as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: Default `peer_str = "nullptr"` is a magic string and lies in logs
src/governance/governance.h (line 324)
After this refactor, AddGovernanceObject defaults peer_str to the literal string "nullptr" (also in signing.h:44, with callers governance.cpp:834 and signing.cpp:166 explicitly passing "nullptr"). The previous const CNode* pfrom = nullptr produced pfrom ? pfrom->GetLogString() : "nullptr" at the log site, but for genuinely local-origin objects the log now reports received from peer nullptr, which is misleading. Default to an empty string and let the log format choose local/self when empty: LogPrint(..., peer_str.empty() ? "local" : peer_str). Same applies to the two call sites that hard-code "nullptr". Behavior is unchanged; this is a readability/log-quality fix.
🟡 suggestion: `ScriptToAsmStr` → `HexStr` in collateral logs degrades operator debuggability
src/governance/object.cpp (lines 491-499)
The two LogPrint calls in IsCollateralValid previously printed disassembled scripts (e.g. OP_RETURN <hash>), which is the format operators see when triaging "invalid collateral" reports. Switching to HexStr emits raw hex bytes that require manual decoding. The motivation (drop the core_io.h dependency from governance/object) is sound, but the regression is real for support workflows. Options: (a) keep ScriptToAsmStr and find a different way to break the dependency, (b) only switch the lengthy output.scriptPubKey line to hex and leave the short findScript disassembled, or (c) accept the regression with a release note.
💬 nitpick: Stale breadcrumb: `CDeterministicMN::ToJson()` is no longer in `evo/deterministicmns.cpp`
src/evo/core_write.cpp (line 180)
After this PR, CDeterministicMN::ToJson() was moved to src/rpc/evo_util.cpp (see rpc/evo_util.cpp:142), but the breadcrumb comment here still points at the old location.
💡 Suggested change
// CDeterministicMN::ToJson() defined in rpc/evo_util.cpp
💬 nitpick: `if (mempool)` guards in DashChainstateSetup{,Close} are untested defensive scaffolding
src/node/chainstate.cpp (lines 236-253)
Today both dashd and the test framework always pass a non-null mempool, so the new guarded branch is unreachable on the regular node path. The change is fine as kernel-0 prep, but a one-line comment near the guard explaining why it exists ("kernel-0 will invoke this with a null mempool") would prevent future readers from assuming it's hot code.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/governance/governance.h`:
- [SUGGESTION] line 324: Default `peer_str = "nullptr"` is a magic string and lies in logs
After this refactor, `AddGovernanceObject` defaults `peer_str` to the literal string `"nullptr"` (also in `signing.h:44`, with callers `governance.cpp:834` and `signing.cpp:166` explicitly passing `"nullptr"`). The previous `const CNode* pfrom = nullptr` produced `pfrom ? pfrom->GetLogString() : "nullptr"` at the log site, but for genuinely local-origin objects the log now reports `received from peer nullptr`, which is misleading. Default to an empty string and let the log format choose `local`/`self` when empty: `LogPrint(..., peer_str.empty() ? "local" : peer_str)`. Same applies to the two call sites that hard-code `"nullptr"`. Behavior is unchanged; this is a readability/log-quality fix.
In `src/governance/object.cpp`:
- [SUGGESTION] lines 491-499: `ScriptToAsmStr` → `HexStr` in collateral logs degrades operator debuggability
The two `LogPrint` calls in `IsCollateralValid` previously printed disassembled scripts (e.g. `OP_RETURN <hash>`), which is the format operators see when triaging "invalid collateral" reports. Switching to `HexStr` emits raw hex bytes that require manual decoding. The motivation (drop the `core_io.h` dependency from `governance/object`) is sound, but the regression is real for support workflows. Options: (a) keep `ScriptToAsmStr` and find a different way to break the dependency, (b) only switch the lengthy `output.scriptPubKey` line to hex and leave the short `findScript` disassembled, or (c) accept the regression with a release note.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Mechanical refactor PR to move ChainLockSig and VerifyChainLock between TUs and replace CNode* with std::string in the governance signing API. No consensus impact. Three minor cleanup findings: the string "nullptr" used as a sentinel leaks into call sites and logs; the default argument is inconsistent between base and override; one redundant forward declaration. Codex found nothing actionable in scope.
Reviewed commit: 45d24b1
Fresh dispatcher rerun for this queue item. Same-SHA automated reviews already existed, so I am posting this as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🟡 suggestion: Literal "nullptr" leaks into the API and log output as a no-peer sentinel
src/governance/governance.h (line 324)
Before this PR, the no-peer case was modelled as CNode* pfrom = nullptr and only formatted as "nullptr" at log time (pfrom ? pfrom->GetLogString() : "nullptr"). The refactor pushes that placeholder string up into the API: callers now pass the literal "nullptr" (governance.cpp:834 in CheckPostponedObjects, signing.cpp:166 in CreateGovernanceTrigger), and the default arg in governance.h:324 hardcodes the same string. The log line ... received from peer nullptr becomes ambiguous — a reader cannot tell it from a peer that legitimately self-identifies that way. Prefer modelling absence explicitly: std::optional<std::string> (formatting "nullptr" only inside the log call), or a named static constexpr std::string_view kNoPeer = "nullptr" so the intent is documented at every call site.
💬 nitpick: Default argument mismatch between virtual base and override
src/governance/signing.h (line 44)
GovernanceSignerParent::AddGovernanceObject declares peer_str without a default, while the concrete CGovernanceManager::AddGovernanceObject (governance.h:324) defaults it to "nullptr". Default arguments are bound to the static type of the call expression, not inherited, so a caller through a GovernanceSignerParent&/* is forced to pass an explicit string while a caller through CGovernanceManager&/* is not. All callers today use the concrete type, so it works, but the asymmetry is a footgun the next time someone refactors against the interface. Either add the default to the base or remove it from the override.
💡 Suggested change
virtual void AddGovernanceObject(CGovernanceObject& govobj, const std::string& peer_str = "nullptr") = 0;
💬 nitpick: Redundant forward declaration of `uint256`
src/chainlock/chainlock.h (lines 9-16)
The same hunk that adds class uint256; also includes <uint256.h>, which already provides the complete definition. The forward declaration is dead. The include is genuinely needed (IsNull() constructs a uint256() and the SERIALIZE_METHODS block reads/writes a uint256 member), so just drop the forward declaration rather than the include.
💡 Suggested change
class CBlockIndex;
class CSporkManager;
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/governance/governance.h`:
- [SUGGESTION] line 324: Literal "nullptr" leaks into the API and log output as a no-peer sentinel
Before this PR, the no-peer case was modelled as `CNode* pfrom = nullptr` and only formatted as `"nullptr"` at log time (`pfrom ? pfrom->GetLogString() : "nullptr"`). The refactor pushes that placeholder string up into the API: callers now pass the literal `"nullptr"` (governance.cpp:834 in `CheckPostponedObjects`, signing.cpp:166 in `CreateGovernanceTrigger`), and the default arg in governance.h:324 hardcodes the same string. The log line `... received from peer nullptr` becomes ambiguous — a reader cannot tell it from a peer that legitimately self-identifies that way. Prefer modelling absence explicitly: `std::optional<std::string>` (formatting `"nullptr"` only inside the log call), or a named `static constexpr std::string_view kNoPeer = "nullptr"` so the intent is documented at every call site.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/governance/governance.cpp (1)
750-759: ⚡ Quick winDrop the stale
CConnman¶meter fromProcessVoteAndRelay().After
ProcessVote()stopped taking peer state, this helper no longer usesconnmanor performs any network-side recovery. Keeping the parameter here leaves an unnecessary net-facing dependency in the API and makes the method look more capable than it is.♻️ Suggested cleanup
-bool CGovernanceManager::ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman) +bool CGovernanceManager::ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception) { AssertLockNotHeld(cs_store); AssertLockNotHeld(cs_relay); - uint256 hashToRequest; // Ignored for local votes (no peer to request from) - bool fOK = ProcessVote(vote, exception, hashToRequest); + uint256 ignored_hash_to_request; + bool fOK = ProcessVote(vote, exception, ignored_hash_to_request); if (fOK) { RelayVote(vote); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/governance/governance.cpp` around lines 750 - 759, Remove the unused network dependency by deleting the CConnman& connman parameter from CGovernanceManager::ProcessVoteAndRelay: update the function signature in the class declaration and its definition to take only (const CGovernanceVote& vote, CGovernanceException& exception), remove any references to connman inside the body (there are none), and update all call sites to stop passing a CConnman argument; also adjust any forward declarations/includes that only existed to support that parameter. Ensure matching prototype changes in headers and recompilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/governance/governance.cpp`:
- Around line 750-759: Remove the unused network dependency by deleting the
CConnman& connman parameter from CGovernanceManager::ProcessVoteAndRelay: update
the function signature in the class declaration and its definition to take only
(const CGovernanceVote& vote, CGovernanceException& exception), remove any
references to connman inside the body (there are none), and update all call
sites to stop passing a CConnman argument; also adjust any forward
declarations/includes that only existed to support that parameter. Ensure
matching prototype changes in headers and recompilation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1a69f0d3-1f60-44bc-b4f3-a046af037868
📒 Files selected for processing (17)
src/chainlock/chainlock.cppsrc/chainlock/chainlock.hsrc/chainlock/clsig.cppsrc/chainlock/clsig.hsrc/chainlock/handler.cppsrc/chainlock/handler.hsrc/chainlock/signing.hsrc/governance/governance.cppsrc/governance/signing.cppsrc/llmq/dkgsessionmgr.cppsrc/llmq/dkgsessionmgr.hsrc/llmq/observer.cppsrc/rpc/quorums.cppsrc/test/llmq_chainlock_tests.cppsrc/test/util/llmq_tests.hsrc/zmq/zmqpublishnotifier.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (2)
- src/chainlock/signing.h
- src/chainlock/handler.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/governance/signing.cpp
- src/zmq/zmqpublishnotifier.cpp
NOTE: the circular dependency over dkgsessionhandler <-> net_processing is not new,
it's pre-existing one.
Dependency of llmq/observer is also pre-existing but has been marked by recursive including over msg_result.h
"llmq/dkgsessionhandler -> net_processing -> llmq/dkgsessionmgr -> llmq/dkgsessionhandler",
"llmq/dkgsessionhandler -> net_processing -> llmq/observer -> llmq/dkgsessionhandler",
…tangle chain-state dependencies chain-state should be available to validate chainlocks without using chainlock/handlers.cpp which is tightly connected to network code. On other hand, VerifyChainLock uses quorumsman, so it can't be just moved to chainlock/clsig or something like that because it will create new circular dependency over quorumsman. Though, chainlock::Chainlock and chainlock::ChainLockSig are tightly connected and there is no usages of ChainLockSig withdout Chainlock except regression tests; moving ChainLockSig to chainlock.h is fine then.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/lint/lint-circular-dependencies.py (1)
47-47: ⚖️ Poor tradeoffCorrectly documents the cycle, but it is newly introduced (not pre-existing).
The PR description refers to this as a "pre-existing" circular dependency, but the path through
observeronly exists becauseobserver.cppnow includesdkgsessionhandler.hfor the first time in this PR. The previously tracked cycle at line 46 goes throughdkgsessionmgr, notobserver. Adding this entry to the allowlist is the right call per project conventions, but the cycle itself is new technical debt that should be tracked for resolution (e.g., by eliminatingnet_processing.hfromdkgsessionhandler.hor using a factory abstraction inobserver.cpp).Based on learnings: "when refactoring changes module dependencies, update the EXPECTED_CIRCULAR_DEPENDENCIES entries to reflect the new dependency paths rather than removing them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lint/lint-circular-dependencies.py` at line 47, The new circular dependency through observer is newly introduced and must be recorded and tracked: update the EXPECTED_CIRCULAR_DEPENDENCIES list in test/lint/lint-circular-dependencies.py to include the exact new cycle "llmq/dkgsessionhandler -> net_processing -> llmq/observer -> llmq/dkgsessionhandler" (do not delete or collapse other entries like the dkgsessionmgr path), and add a short follow-up TODO to track remediation; in parallel, plan a code fix by removing the direct net_processing.h dependency from dkgsessionhandler.h or refactoring observer.cpp to use a factory/abstraction instead of including dkgsessionhandler.h so the cycle can be eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/lint/lint-circular-dependencies.py`:
- Line 47: The new circular dependency through observer is newly introduced and
must be recorded and tracked: update the EXPECTED_CIRCULAR_DEPENDENCIES list in
test/lint/lint-circular-dependencies.py to include the exact new cycle
"llmq/dkgsessionhandler -> net_processing -> llmq/observer ->
llmq/dkgsessionhandler" (do not delete or collapse other entries like the
dkgsessionmgr path), and add a short follow-up TODO to track remediation; in
parallel, plan a code fix by removing the direct net_processing.h dependency
from dkgsessionhandler.h or refactoring observer.cpp to use a
factory/abstraction instead of including dkgsessionhandler.h so the cycle can be
eliminated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 187f3397-2648-4537-9d62-a8d2f09e59a2
📒 Files selected for processing (20)
src/chainlock/chainlock.cppsrc/chainlock/chainlock.hsrc/chainlock/clsig.cppsrc/chainlock/clsig.hsrc/chainlock/handler.cppsrc/chainlock/handler.hsrc/chainlock/signing.hsrc/governance/governance.cppsrc/governance/governance.hsrc/governance/signing.cppsrc/llmq/dkgsessionmgr.cppsrc/llmq/dkgsessionmgr.hsrc/llmq/observer.cppsrc/node/interfaces.cppsrc/rpc/governance.cppsrc/rpc/quorums.cppsrc/test/llmq_chainlock_tests.cppsrc/test/util/llmq_tests.hsrc/zmq/zmqpublishnotifier.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (2)
- src/chainlock/signing.h
- src/chainlock/handler.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified findings against the worktree at 0bcb345. Two prior findings (peer-name sentinel via 'nullptr' string, AddGovernanceObject default-argument mismatch) are resolved. The only remaining finding is a style-only redundant forward declaration of uint256 in chainlock.h. Codex returned no findings; no new correctness or consensus issues identified.
Reviewed commit: 0bcb345
💬 1 nitpick(s)
| class CBlockIndex; | ||
| class CSporkManager; | ||
| class uint256; |
There was a problem hiding this comment.
💬 Nitpick: Redundant forward declaration of uint256
Line 12 #include <uint256.h> already provides the complete definition of uint256, so the class uint256; forward declaration on line 16 is dead. The include itself is necessary because IsNull() value-initializes a uint256 and SERIALIZE_METHODS reads/writes one — drop only the forward declaration.
💡 Suggested change
| class CBlockIndex; | |
| class CSporkManager; | |
| class uint256; | |
| class CBlockIndex; | |
| class CSporkManager; |
source: ['claude']
2 participants
Issue being fixed or feature implemented
That's some final preparation with various refactors for kernel-0 project, splitted out from #7234
What was done?
NOTE: the circular dependency over
dkgsessionhandler <-> net_processingis not new, it's pre-existing one but discovered by replacing removingllmq/dkgsessionhandler.hfromllmq/dkgsessionmgr.hand direct adding this include to llmq/observerHow Has This Been Tested?
Run unit & functional tests; proof-of-concept of final PR is #7234
Compilation time is not affected by this PR: removing msg_result.h from the header have too small positive impact; replacing chainlock/clsig.h to chainlock/chainlock.h doesn't give any visible change either.
Breaking Changes
N/A
Checklist: