test: better checks for trigger removal in feature_governance_cl.py #7051
Conversation
Check for the actual "CleanAndRemoveTriggers -- Removing trigger object" log message instead of the generic "UpdateCachesAndClean" message. Exclude isolated node 5 from debug log assertions since it doesn't receive governance updates during this test phase.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe test changes in test/functional/feature_governance_cl.py add a governance_tip_updated waiter that reads the last 100 KiB of each node's debug log to detect UpdatedBlockTip on nodes 0–4. The per-delta mocktime advancement and log assertions are restricted to self.nodes[0:5]; node 5 is advanced and scheduled separately via setmocktime and mockscheduler. The 5-minute delta expected cleanup log is changed to "CleanAndRemoveTriggers -- Removing trigger object". A synchronization wait for the governance tip update is inserted before the cleanup waits. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-12-09T20:40:38.414ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-10-28T08:54:00.392ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
📚 Learning: 2025-08-01T07:46:37.840ZApplied to files:
📚 Learning: 2025-11-04T18:24:27.241ZApplied to files:
📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-07-29T14:33:01.040ZApplied to files:
🧬 Code graph analysis (1)test/functional/feature_governance_cl.py (1)
⏰ 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). (2)
🔇 Additional comments (3)
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 (1)
test/functional/feature_governance_cl.py (1)
145-156: Log expectation change and isolated-node handling look correct; consider clarifying comment.Using
['CleanAndRemoveTriggers -- Removing trigger object']for the 5-minute delta aligns better with the intent of “mark old triggers for deletion”, and restrictingassert_debug_logtoself.nodes[0:5]while still callingsetmocktime/mockscheduleronself.nodes[5]keeps the scheduler-based cleanup running on the isolated node without assuming it has the same governance state or logs. This matches the design that mockscheduler runs regardless of network isolation, while avoiding flaky expectations on node 5’s debug log.To make the special handling of node 5 obvious to future readers, you could add a brief comment:
- self.nodes[5].setmocktime(self.mocktime) - self.nodes[5].mockscheduler(delta) + # Isolated node: still advance scheduler so governance cleanup runs, + # but its state/logs differ, so we don't assert on its debug log here. + self.nodes[5].setmocktime(self.mocktime) + self.nodes[5].mockscheduler(delta)Based on learnings, this preserves scheduler behavior on isolated nodes while tightening the log checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_governance_cl.py
🧠 Learnings (2)
📓 Common learnings
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.
📚 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_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (1)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)
⏰ 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). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-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: Lint / Run linters
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/functional/feature_governance_cl.py (2)
146-156: Refinegovernance_tip_updatedto avoid magic height and closure on loop variableThis helper works, but there are two concerns:
- It hard‑codes block height
281in the expected log line, which makes the test brittle if the setup ever changes the tip height at this point.- It closes over
node, which is only bound by the subsequentfor node in self.nodes[0:5]loop. While this is functionally OK given the sequentialwait_untilcalls, it’s non-obvious and is exactly what Ruff’s B007 is warning about.You can make this both clearer and more robust by capturing the current tip height once and passing
nodeexplicitly:- self.log.info("Wait for governance module to catch up with block updates") - def governance_tip_updated(): - expected_msg = 'CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: 281' - with open(node.debug_log_path, encoding='utf-8') as dl: - debug_log = dl.read() - if re.search(re.escape(expected_msg), debug_log, flags=re.MULTILINE) is None: - return False - return True - for node in self.nodes[0:5]: - self.wait_until(governance_tip_updated, sleep=0.5, timeout=15) + self.log.info("Wait for governance module to catch up with block updates") + tip_height = self.nodes[0].getblockcount() + expected_msg = f'CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: {tip_height}' + + def governance_tip_updated(node): + with open(node.debug_log_path, encoding='utf-8') as dl: + debug_log = dl.read() + return re.search(re.escape(expected_msg), debug_log, flags=re.MULTILINE) is not None + + for node in self.nodes[0:5]: + self.wait_until(lambda node=node: governance_tip_updated(node), sleep=0.5, timeout=15)This removes the magic constant, satisfies B007, and keeps the intent (“all non‑isolated nodes have processed the latest UpdatedBlockTip”) explicit.
159-160: More specific cleanup log expectation looks good, but may still be timing‑sensitiveSwitching the 5‑minute delta expectation to
'CleanAndRemoveTriggers -- Removing trigger object'gives a clearer signal that the trigger was actually marked for removal, instead of matching any genericUpdateCachesAndCleanline. That aligns with the PR goal of tightening the assertion.Given previous intermittent failures around this phase, it’s worth double‑checking (locally or in CI) that this specific line reliably appears for all non‑isolated nodes after the prior governance-tip wait, as the cleanup behavior is quite timing/height dependent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 58d89b6 and 53afb68e0a649242ce2bb45a832cf4ff0f1ae904.
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_governance_cl.py
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
test/functional/feature_governance_cl.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_governance_cl.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
test/functional/feature_governance_cl.py
📚 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_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (1)
test/functional/test_framework/test_node.py (2)
debug_log_path(435-436)assert_debug_log(444-472)
🪛 Ruff (0.14.8)
test/functional/feature_governance_cl.py
154-154: Loop control variable node not used within loop body
(B007)
⏰ 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). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (2)
test/functional/feature_governance_cl.py (2)
8-8: Import ofreis appropriate hereThe new
reimport is needed for the debug-log polling helper below and is scoped correctly in this test module. No issues.
163-168: Restricting log assertions to non‑isolated nodes while still running the scheduler on the isolated node is correctChanging the loop to:
- Run
assert_debug_logand scheduler only onself.nodes[0:5](non‑isolated nodes), and- Separately advancing
mocktimeandmockscheduleronself.nodes[5]without asserting its logs,fits the intended model where the isolated node continues to run scheduler-based governance cleanup but may legitimately have different governance state and log output. This matches the prior learning that mockscheduler is independent of network connectivity while avoiding false negatives from node 5’s differing view.
UdjinM6
force-pushed
the
test_gov_cl_trig_rem_check
branch
from
53afb68 to
2fce68d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_governance_cl.py (1)
146-156: Consider optimizing log polling to track file position.The custom
governance_tip_updatedfunction reads the entire debug log on each iteration, which is less efficient than the existingassert_debug_loginfrastructure that tracks the previous file position (see test_framework/test_node.py:443-471). For consistency and performance, consider tracking the last read position.Additionally, based on the past review comment, the test still fails with "NOT marked for removal" even though the UpdatedBlockTip synchronization appears to succeed. This suggests the synchronization may be necessary but not sufficient to resolve the intermittent failures. You may need to investigate whether additional state needs to be synchronized or if there's a timing issue in the governance cleanup logic itself.
Optional refactor to match assert_debug_log efficiency:
- self.log.info("Wait for governance module to catch up with block updates") - tip_height = self.nodes[0].getblockcount() - expected_msg = f'CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: {tip_height}' - - def governance_tip_updated(node): - with open(node.debug_log_path, encoding='utf-8') as dl: - debug_log = dl.read() - return re.search(re.escape(expected_msg), debug_log, flags=re.MULTILINE) is not None - - for node in self.nodes[0:5]: - self.wait_until(lambda node=node: governance_tip_updated(node), sleep=0.5, timeout=15) + self.log.info("Wait for governance module to catch up with block updates") + tip_height = self.nodes[0].getblockcount() + expected_msg = f'CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight: {tip_height}' + + # Wait for governance tip update on non-isolated nodes + for node in self.nodes[0:5]: + self.wait_until(lambda node=node: node.wait_for_debug_log([expected_msg], timeout=0), sleep=0.5, timeout=15)Note: You would need to add a non-blocking
wait_for_debug_logvariant to the test framework, or use a different approach with position tracking similar toassert_debug_log.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 53afb68e0a649242ce2bb45a832cf4ff0f1ae904 and 2fce68dfd0774ffc88044d14de06929e2048b6ba.
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_governance_cl.py
🧠 Learnings (5)
📓 Common learnings
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: 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: 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.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
test/functional/feature_governance_cl.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_governance_cl.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
test/functional/feature_governance_cl.py
📚 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_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (1)
test/functional/test_framework/test_node.py (2)
debug_log_path(435-436)assert_debug_log(444-472)
⏰ 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). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
🔇 Additional comments (3)
test/functional/feature_governance_cl.py (3)
8-8: LGTM - necessary import for log polling.The regex module is correctly imported for the new
governance_tip_updatedhelper function.
164-169: Correct exclusion of isolated node from log assertions.The change correctly limits log assertions to nodes 0-4 while still advancing mocktime and running the scheduler for the isolated node 5. This aligns with the established pattern that the scheduler operates independently of network state, but isolated nodes have different governance state (missing votes and triggers), so the cleanup logs won't match the expected messages.
Based on learnings, this is the right approach for handling isolated nodes in governance tests.
160-160: Unable to verify - codebase access unavailableI attempted to verify the review comment by examining the test file
test/functional/feature_governance_cl.py, but cannot access the repository. Without direct access to the actual code, I cannot confirm:
- Whether line 160 contains the exact assertion shown
- If the log messages ("UpdateCachesAndClean", "CleanAndRemoveTriggers -- Removing trigger object", "CleanAndRemoveTriggers -- NOT marked for removal") are actually generated
- The current test state (passing or failing)
- The trigger expiration and removal logic
- Whether the claims about the "past review" reflect the current codebase state
The original review comment makes specific claims about test failures and trigger removal behavior that require direct code inspection to verify.
…eature_governance_cl.py`
UdjinM6
force-pushed
the
test_gov_cl_trig_rem_check
branch
from
2fce68d to
27ac9f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/feature_governance_cl.py (1)
145-158:governance_tip_updatedlogic is sound; consider simplifying file access (optional).The new wait for
CGovernanceManager::UpdatedBlockTipon nodes 0–4 matches the intent to let governance catch up before asserting cleanup logs, and correctly excludes the isolated node.If you want to shave a bit of complexity/IO, you can avoid opening
debug.logtwice by using a single handle for both size and tail, e.g.:- def governance_tip_updated(node): - with open(node.debug_log_path, encoding='utf-8') as dl: - seek_pos = node.debug_log_bytes() - 100 * 1024 # read the last 100 KiB only - dl.seek(seek_pos if seek_pos > 0 else 0) - debug_log_part = dl.read() - return expected_msg in debug_log_part + def governance_tip_updated(node): + with open(node.debug_log_path, encoding='utf-8') as dl: + dl.seek(0, 2) + end_pos = dl.tell() + dl.seek(max(0, end_pos - 100 * 1024)) # read the last 100 KiB only + debug_log_part = dl.read() + return expected_msg in debug_log_partThis keeps behavior the same while avoiding an extra open/size pass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2fce68dfd0774ffc88044d14de06929e2048b6ba and 27ac9f2.
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/feature_governance_cl.py
🧠 Learnings (5)
📓 Common learnings
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: 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.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
Applied to files:
test/functional/feature_governance_cl.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_governance_cl.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
test/functional/feature_governance_cl.py
📚 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_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (1)
test/functional/test_framework/test_node.py (3)
debug_log_path(435-436)debug_log_bytes(438-441)assert_debug_log(444-472)
⏰ 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). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: Lint / Run linters
| self.log.info("Bump mocktime to trigger governance cleanup") | ||
| for delta, expected in ( | ||
| (5 * 60, ['UpdateCachesAndClean -- Governance Objects:']), # mark old triggers for deletion | ||
| (5 * 60, ['CleanAndRemoveTriggers -- Removing trigger object']), # mark old triggers for deletion | ||
| (10 * 60, ['UpdateCachesAndClean -- Governance Objects: 0']), # deletion after delay | ||
| ): | ||
| self.mocktime += delta | ||
| for node in self.nodes: | ||
| for node in self.nodes[0:5]: | ||
| with node.assert_debug_log(expected_msgs=expected): | ||
| node.setmocktime(self.mocktime) | ||
| node.mockscheduler(delta) | ||
| self.nodes[5].setmocktime(self.mocktime) | ||
| self.nodes[5].mockscheduler(delta) |
There was a problem hiding this comment.
5‑minute expectation on "CleanAndRemoveTriggers -- Removing trigger object" is too strict and appears flaky.
The new assertion:
(5 * 60, ['CleanAndRemoveTriggers -- Removing trigger object']), # mark old triggers for deletionassumes that the first cleanup pass after the additional block + 5‑minute mocktime jump will always log "Removing trigger object". The combined log in the earlier review, however, shows a valid run where at this point CleanAndRemoveTriggers logs:
CSuperblock::IsExpired -- nBlockHeight = 260, nExpirationBlock = 280CleanAndRemoveTriggers -- NOT marked for removal
and never logs "CleanAndRemoveTriggers -- Removing trigger object" in that pass, while UpdateCachesAndClean -- Governance Objects: 1 ... is present.
Your current changes (including the new UpdatedBlockTip wait at lines 145‑158) don’t change the underlying C++ logic that decides whether a trigger is “marked for removal” or “NOT marked for removal”; they only affect scheduling/catch‑up. So the scenario from the log remains possible, and this assertion will still intermittently fail even when governance behavior is correct.
The node‑5 handling here looks good: you still run setmocktime/mockscheduler on the isolated node but exclude it from assert_debug_log, which matches the intent and the earlier learning that scheduler‑based cleanup must still run on isolated nodes.
To make the test robust while keeping your new synchronization and node‑range changes, I’d recommend relaxing the 5‑minute expectation back to a more generic cleanup log (as before), and rely on the 10‑minute "Governance Objects: 0" plus the final RPC checks to assert actual removal:
- for delta, expected in (
- (5 * 60, ['CleanAndRemoveTriggers -- Removing trigger object']), # mark old triggers for deletion
- (10 * 60, ['UpdateCachesAndClean -- Governance Objects: 0']), # deletion after delay
- ):
+ for delta, expected in (
+ # First run: ensure governance cleanup runs; we only care that UpdateCachesAndClean fired.
+ (5 * 60, ['UpdateCachesAndClean -- Governance Objects:']), # mark old triggers for deletion
+ (10 * 60, ['UpdateCachesAndClean -- Governance Objects: 0']), # deletion after delay
+ ):This keeps the log‑based signal that cleanup ran during the first pass, avoids depending on the precise “Removing trigger object” branch, and should eliminate the intermittent failures you’re seeing.
There was a problem hiding this comment.
Depending on Removing trigger object log line is intentional. Having NOT marked for removal line in logs is a sign of sync issues (scheduler running before block heigh is updated in governance module) 27ac9f2 is designed to solve.
There was a problem hiding this comment.
@UdjinM6, thank you for the clarification! I now understand the intent. The synchronization step you added (waiting for UpdatedBlockTip at the current tip height) ensures that the governance module has processed the latest block before the scheduler-based cleanup runs. With that synchronization in place, the strict check for "Removing trigger object" is the correct verification that the fix works as intended. If "NOT marked for removal" were to appear despite the synchronization, that would indeed indicate a problem that the test should catch.
The approach makes sense: fix the race condition with proper synchronization, then verify the fix with a strict assertion. My concern was misplaced because I didn't account for the fact that the new governance_tip_updated wait should eliminate the timing issue.
🐰💡
✏️ Learnings added
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7051
File: test/functional/feature_governance_cl.py:159-170
Timestamp: 2025-12-09T20:40:38.414Z
Learning: In feature_governance_cl.py, before running governance cleanup with mockscheduler, the test must wait for the governance module to catch up with the latest block height by checking for the "CGovernanceManager::UpdatedBlockTip -- nCachedBlockHeight" log message. This synchronization ensures that subsequent cleanup operations see the correct block height and produce deterministic results (e.g., "CleanAndRemoveTriggers -- Removing trigger object" instead of "NOT marked for removal").
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/governance/**/*.{cpp,h} : Governance implementation must support governance objects (proposals, triggers, superblock management) and on-chain voting with tallying
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: 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: 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.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
ACK 27ac9f2
feature_governance_cl.py --descriptors | ✓ Passed | 24 s
feature_governance_cl.py --descriptors | ✓ Passed | 25 s
feature_governance_cl.py --descriptors | ✓ Passed | 26 s
feature_governance_cl.py --descriptors | ✓ Passed | 27 s
feature_governance_cl.py --descriptors | ✓ Passed | 27 s
feature_governance_cl.py --descriptors | ✓ Passed | 27 s
feature_governance_cl.py --descriptors | ✓ Passed | 28 s
feature_governance_cl.py --descriptors | ✓ Passed | 29 s
feature_governance_cl.py --descriptors | ✓ Passed | 29 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 23 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 26 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 26 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 28 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 28 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 28 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 29 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 31 s
feature_governance_cl.py --legacy-wallet | ✓ Passed | 33 s
3 participants
Issue being fixed or feature implemented
Fixes intermittent failures in
feature_governance_cl.py.Alternative to #7041
What was done?
CleanAndRemoveTriggers -- Removing trigger objectlog message instead of the genericUpdateCachesAndCleanmessage.How Has This Been Tested?
Run tests
Breaking Changes
Checklist: