refactor: remove spork21 (is all members connected) and related code. #6968
Conversation
|
WalkthroughThis pull request removes all references to SPORK_21_QUORUM_ALL_CONNECTED ("all members connected" spork) and eliminates spork manager dependencies from the LLMQ quorum subsystem. Changes include: renaming the spork to deprecated status, removing the Sequence Diagram(s)sequenceDiagram
actor Before as Before (with SPORK_21)
participant QM as CQuorumManager
participant Utils as utils::GetQuorumConnections
participant Spork as CSporkManager
Before->>QM: new(..., sporkman)
QM->>Utils: GetQuorumConnections(..., sporkman)
Utils->>Spork: Check IsAllMembersConnectedEnabled()
alt All Connected Enabled
Spork-->>Utils: true
Utils-->>QM: Return all members
else All Connected Disabled
Spork-->>Utils: false
Utils-->>QM: Return filtered members
end
sequenceDiagram
actor After as After (SPORK_21 removed)
participant QM as CQuorumManager
participant Utils as utils::GetQuorumConnections
After->>QM: new(...) [no sporkman]
QM->>Utils: GetQuorumConnections(...) [no sporkman]
Utils-->>QM: Return all members (deterministic)
Note over After,Utils: Unconditional behavior, no spork dependency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (3)
test/functional/feature_llmq_is_retroactive.py (1)
68-68: Consistent mocktime bumpsUsing 10-second bumps at synchronization points looks fine and reduces wall time. Consider using a small helper (e.g., bump_and_sync) if this pattern spreads further, but not required here.
Also applies to: 89-89, 99-99, 104-104, 110-110, 131-131, 136-136
test/functional/test_framework/test_framework.py (2)
1933-1939: Fix bare except in wait_for_instantlockUse explicit exceptions to satisfy lint and avoid hiding bugs.
- def check_instantlock(): - try: - self.bump_mocktime(1) - return node.getrawtransaction(txid, True)["instantlock"] - except: - return False + def check_instantlock(): + try: + self.bump_mocktime(1) + return node.getrawtransaction(txid, True)["instantlock"] + except JSONRPCException: + return False + except KeyError: + return False
1943-1951: Fix bare except in wait_for_chainlocked_blockMirror the explicit handling and keep the 1s step bump.
- def check_chainlocked_block(): - try: - self.bump_mocktime(1) - block = node.getblock(block_hash) - return block["confirmations"] > 0 and block["chainlock"] - except: - return False + def check_chainlocked_block(): + try: + self.bump_mocktime(1) + block = node.getblock(block_hash) + return block["confirmations"] > 0 and block["chainlock"] + except JSONRPCException: + return False + except KeyError: + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/llmq/context.cpp(1 hunks)src/llmq/dkgsession.cpp(1 hunks)src/llmq/dkgsessionhandler.cpp(1 hunks)src/llmq/options.cpp(0 hunks)src/llmq/options.h(0 hunks)src/llmq/quorums.cpp(3 hunks)src/llmq/quorums.h(1 hunks)src/llmq/signing_shares.cpp(4 hunks)src/llmq/signing_shares.h(2 hunks)src/llmq/utils.cpp(3 hunks)src/llmq/utils.h(2 hunks)src/masternode/active/context.cpp(1 hunks)src/net.cpp(0 hunks)src/net.h(0 hunks)src/rpc/quorums.cpp(1 hunks)src/spork.cpp(1 hunks)src/spork.h(2 hunks)test/functional/feature_llmq_connections.py(0 hunks)test/functional/feature_llmq_data_recovery.py(0 hunks)test/functional/feature_llmq_is_retroactive.py(7 hunks)test/functional/feature_llmq_signing.py(3 hunks)test/functional/feature_llmq_simplepose.py(0 hunks)test/functional/test_framework/test_framework.py(3 hunks)test/functional/test_runner.py(0 hunks)
💤 Files with no reviewable changes (8)
- src/llmq/options.cpp
- src/llmq/options.h
- src/net.h
- test/functional/feature_llmq_data_recovery.py
- test/functional/feature_llmq_connections.py
- test/functional/test_runner.py
- test/functional/feature_llmq_simplepose.py
- src/net.cpp
🧰 Additional context used
📓 Path-based instructions (3)
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/llmq/dkgsession.cppsrc/llmq/context.cppsrc/llmq/utils.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/dkgsessionhandler.cppsrc/masternode/active/context.cppsrc/spork.hsrc/llmq/utils.cppsrc/spork.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.h
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_llmq_signing.pytest/functional/feature_llmq_is_retroactive.pytest/functional/test_framework/test_framework.py
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/masternode/active/context.cpp
🧠 Learnings (14)
📓 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: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
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: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/llmq/dkgsession.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/feature_llmq_signing.pysrc/llmq/utils.hsrc/llmq/dkgsessionhandler.cpptest/functional/test_framework/test_framework.pysrc/llmq/utils.cppsrc/rpc/quorums.cpp
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.cppsrc/llmq/quorums.cppsrc/llmq/quorums.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.cppsrc/llmq/utils.hsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/dkgsessionhandler.cppsrc/llmq/utils.cppsrc/rpc/quorums.cppsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.cpp
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
Applied to files:
src/llmq/utils.hsrc/llmq/utils.cppsrc/llmq/signing_shares.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/llmq/quorums.hsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.h
📚 Learning: 2025-08-19T18:53:02.863Z
Learnt from: knst
Repo: dashpay/dash PR: 6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
Learning: The `check_no_is` method in `test/functional/feature_llmq_is_retroactive.py` uses assertions for validation and doesn't return any value - it either succeeds silently or raises an AssertionError when the InstantLock assertion fails.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/masternode/active/context.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/llmq/signing_shares.cpp
📚 Learning: 2025-07-15T14:53:04.819Z
Learnt from: knst
Repo: dashpay/dash PR: 6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Applied to files:
src/llmq/signing_shares.cppsrc/llmq/signing_shares.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/llmq/signing_shares.h
🧬 Code graph analysis (7)
test/functional/feature_llmq_signing.py (6)
test/functional/test_framework/test_framework.py (7)
get_node(1219-1224)MasternodeInfo(1145-1461)get_mninfo(2355-2359)wait_until(907-908)connect_nodes(702-742)connect_nodes(1488-1495)bump_mocktime(880-897)test/functional/test_framework/util.py (2)
assert_raises_rpc_error(132-148)assert_equal(69-74)test/functional/feature_llmq_singlenode.py (2)
assert_sigs_nochange(80-81)wait_for_sigs(77-78)test/functional/test_framework/test_node.py (2)
add_p2p_connection(651-708)disconnect_p2ps(778-795)test/functional/test_framework/p2p.py (2)
P2PInterface(492-770)send_message(432-440)test/functional/test_framework/messages.py (2)
msg_qsigshare(2417-2433)uint256_to_string(140-141)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (2)
EnsureQuorumConnections(833-891)EnsureQuorumConnections(833-837)
test/functional/feature_llmq_is_retroactive.py (1)
test/functional/test_framework/test_framework.py (3)
bump_mocktime(880-897)reconnect_isolated_node(784-786)wait_for_mnauth(2393-2401)
src/llmq/dkgsessionhandler.cpp (1)
src/llmq/utils.cpp (2)
EnsureQuorumConnections(833-891)EnsureQuorumConnections(833-837)
src/rpc/quorums.cpp (1)
src/llmq/utils.cpp (2)
GetQuorumConnections(732-753)GetQuorumConnections(732-735)
src/llmq/signing_shares.cpp (1)
src/spork.h (1)
cs(225-340)
src/llmq/signing_shares.h (1)
src/llmq/params.h (1)
LLMQType(14-125)
🪛 Flake8 (7.3.0)
test/functional/test_framework/test_framework.py
[error] 1934-1934: do not use bare 'except'
(E722)
[error] 1946-1946: do not use bare 'except'
(E722)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/context.cpp
[error] 1-1: Clang format differences found in src/llmq/context.cpp. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/llmq/utils.h
[error] 1-1: Clang format differences found in src/llmq/utils.h. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/llmq/quorums.cpp
[error] 1-1: Clang format differences found in src/llmq/quorums.cpp. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/llmq/quorums.h
[error] 1-1: Clang format differences found in src/llmq/quorums.h. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/spork.h
[error] 1-1: Clang format differences found in src/spork.h. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/llmq/utils.cpp
[error] 1-1: Clang format differences found in src/llmq/utils.cpp. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found in src/rpc/quorums.cpp. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
src/llmq/signing_shares.h
[error] 1-1: Clang format differences found in src/llmq/signing_shares.h. Run the clang-format-diff.py step again or apply formatting changes to fix the diffs.
🪛 Ruff (0.14.4)
test/functional/test_framework/test_framework.py
1934-1934: Do not use bare except
(E722)
1946-1946: Do not use bare except
(E722)
⏰ 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). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (11)
test/functional/feature_llmq_is_retroactive.py (2)
32-34: Deterministic wait is goodLooping sleep + bump_mocktime improves stability and speed. No issues.
163-163: Session-timeout timing bumpsThese targeted bumps align with Cleanup()/timeout windows; looks correct.
Also applies to: 173-173, 178-178, 181-181
src/llmq/context.cpp (1)
31-33: qman ctor updatedLLMQContext now constructs CQuorumManager without sporkman; consistent with header change. Re-run clang-format to fix CI.
test/functional/test_framework/test_framework.py (2)
2109-2111: expected_connections defaultUsing llmq_size - 1 unconditionally (no spork21 branch) is consistent with the PR.
2201-2202: expected_connections for DIP0024Same here: llmq_size_dip0024 - 1 looks right.
src/rpc/quorums.cpp (1)
366-371: GetQuorumConnections signature update verified—all call sites consistentThe function signature has been successfully updated to remove the sporkman parameter. All three call sites (two in src/rpc/quorums.cpp and one in src/llmq/utils.cpp) are consistent with the new signature and properly pass six parameters in the correct order. The code changes shown in the review snippet are correct.
src/llmq/dkgsessionhandler.cpp (1)
566-567: Verification confirmed: Spork removal complete and correctly implementedThe code changes are correct:
EnsureQuorumConnections(lines 566-567) correctly does not takesporkmanAddQuorumProbeConnections(line 569) correctly still takesm_sporkmanto gate PoSe logic viaSPORK_23_QUORUM_POSE(confirmed insrc/llmq/options.cpp:34-37)- No active
spork21checks remain; only historical comments exist insrc/llmq/signing_shares.cppsrc/llmq/quorums.h (1)
270-270: <!-- [scratchpad_start]
[task verify_constructor_change completed]
[observations]
- Header signature (src/llmq/quorums.h:266-270): correct, no sporkman
- Implementation signature (src/llmq/quorums.cpp:210-214): correct, no sporkman
- Call site (src/llmq/context.cpp:30-32): passes 10 args, matches new signature
- Git history confirms sporkman removal: member variable deleted, parameter removed from signature
- Formatting appears consistent across header and implementation
[analysis]
- All three locations (header, implementation, call site) are synchronized
- Constructor decoupling is complete: sporkman successfully removed
- No inconsistencies or mismatches found
- Code formatting aligns with Dash C++ style
[done]
Verification complete. Constructor signature change properly applied across codebase. No issues detected.
[scratchpad_end]Constructor signature change verified: sporkman parameter successfully removed
Verification confirms all definitions and call sites have been correctly updated:
- Header declaration (src/llmq/quorums.h:266-270): removed sporkman parameter
- Implementation (src/llmq/quorums.cpp:210-214): matches header signature
- Call site (src/llmq/context.cpp:30-32): passes 10 arguments matching the new signature
The sporkman decoupling is complete and consistent across the codebase.
src/llmq/dkgsession.cpp (1)
486-487: Change verified: badConnection unconditionally marks missing peersThe modification correctly removes the spork21 gate from badConnection marking when peers are missing from protoMap (lines 486-487). Recent commit "drop leftover usages of IsAllMembersConnectedEnabled and unused references to spork manager" confirms this is intentional cleanup following spork21 removal.
Test coverage is adequate. Tests exercise partial connectivity scenarios with offline nodes (feature_llmq_simplepose.py) and verify expected complaint counts during phase 3. The mine_quorum helper checks spork23_active and validates expected_complaints, ensuring PoSe behavior remains properly gated by SPORK_23. While badConnection internals aren't directly tested, the behavioral chain—missing peers → badConnection → complaints → PoSe banning—is verified through expected_complaints assertions.
src/spork.cpp (1)
265-265: Ignore this review comment; the hardening is intentional and integral to the PR's scope.The git history and code comments confirm that hardening ALL sporks on mainnet is not scope creep—it is the refactoring's core goal. The commit series explicitly removes code paths that only execute when spork21 is disabled (e.g., "remove functions related to disabled spork21"). The comment at src/llmq/signing_shares.cpp:114 states: "this message is not expected to be received once spork21 is hardened as active," confirming that activating all sporks is the intended design.
Returning 0 from GetSporkValue() for all sporks ensures the codebase only executes logic that assumes sporks are active, eliminating dead code and runtime overhead. Limiting the change to SPORK_21_DEPRECATED alone would defeat the refactoring's purpose and leave disabled-spork code paths in place.
Likely an incorrect or invalid review comment.
src/llmq/utils.cpp (1)
732-752: Fix outbound quorum connection selection
DeterministicOutboundConnection()returns the proTxHash that is supposed to initiate the link. By comparing the result todmn->proTxHashwe now keep the peers that should wait for inbound traffic and skip the ones we are actually responsible for dialing. As soon as every quorum member runs this build, each pair of nodes waits for the other side to connect, which breaks the full-mesh topology—especially for NATed operators that cannot accept unsolicited inbound sessions. Please restore the comparison against our own proTxHash.Apply this diff:
- if (!onlyOutbound || deterministicOutbound == dmn->proTxHash) { + if (!onlyOutbound || deterministicOutbound == forMember) {⛔ Skipped due to learnings
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.
… failure This commit is a partial revert of 71ef10a
Downside of having spork21: - llmq code run in 2 different modes depending it is active or not active. It complicates implementation while we not actually need a case of "inactive" - it increase scope of test coverage; functional test to test it takes 2 minutes to run and it increase cost of CI, making CI slower, making local run of functional tests noticeable slower See previuos commit: functional test feature_llmq_is_retroactive.py actually had been broken for awhile, but disabled spork21 masked it
…hods from llmq/signing_shares Careful reviewing implementation is showing that there's no relevant code. Logic which prepares & creates data to be sent by QGETSIGSHARES is `CollectSigSharesToRequest` in the loop over nodeState.sessions and it does nothing if spork21 is enabled
…ds from llmq/signing_shares Careful reviewing implementation is showing that there's no relevant code after removing spork21 Logic which prepares & creates data to be sent by QBSIGSHARES is `CollectSigSharesToSend` in the loop over nodeState.sessions and it does nothing if spork21 is enabled
… spork21 is disabled
…n spork21 is active
… could be send only if spork21 is disabled
…used references to spork manager
src/llmq/options.cpp
Outdated
| { | ||
| return EvalSpork(llmqType, sporkman.GetSporkValue(SPORK_21_QUORUM_ALL_CONNECTED)); | ||
| } | ||
| bool IsAllMembersConnectedEnabled(const Consensus::LLMQType llmqType, const CSporkManager& sporkman) { return true; } |
There was a problem hiding this comment.
c96395b: You can't do that. Mainnet is running with SPORK_21_QUORUM_ALL_CONNECTED: 1 meaning that it's true for small quorums only. For large quorums it's false for a reason - connecting each node of a large quorum is too resource hungry. Also, simply dropping the old quorum connection logic means that nodes that update to this version will try to use "wrong" quorum connection logic (from the point of view of non-updated nodes) and new nodes could be pose-banned or quorum creation could fail, or both.
There was a problem hiding this comment.
All you can do here is replace sporkman.GetSporkValue(SPORK_21_QUORUM_ALL_CONNECTED) with 1 and drop const CSporkManager& sporkman from params. And that's pretty much it imo.
There was a problem hiding this comment.
ah, that's terrible! 😡 I just though I found a short-cut to make 6984 much simpler for signing-shares.
Anyway, nothing to do then.
I miss the fact that EvalSpork actually do something magic
if (spork_value == 1 && llmqType != Consensus::LLMQType::LLMQ_100_67 && llmqType != Consensus::LLMQType::LLMQ_400_60 && llmqType != Consensus::LLMQType::LLMQ_400_85) {
2 participants
Issue being fixed or feature implemented
llmq code for signing runs in 2 different modes depending on spork21 value. It complicates implementation signinficantly; while spork21 is active and hardened on mainnet and there's no plans to change a behavior, no reason to keep this implementation. This refactoring is also a blocker for refactor: drop dependency on net_processing for multiple consensus related objects #6934 because separation of network code is over complicated for llmq/signing_share module.
Having spork21 increase scope for functional tests; The mechanism of recovered signatures is very different if spork21 is active or not. The functional test feature_llmq_signing indeed runs in 2 modes to ensure that signing mechanism works correctly with and without spork21, but I just have found a bug in a functional test
feature_llmq_is_retroactive.pywhich assumed spork21 is disabled. See commits.What was done?
QSIGSESANN,QSIGSHARESINV,QGETSIGSHARES) inllmq/signing_sharesand related codefeature_llmq_is_retroactive.pywith spork21 activeHow Has This Been Tested?
Run unit & functional tests
Breaking Changes
There's no actually breaking changes; because spork21 is hardened on mainnet and no practical usage of spork21 on devnets or testnet.
Checklist: