fix: intermittent error in feature_notification due to extra CL #6672

knst · 2025-05-16T14:16:30Z

Issue being fixed or feature implemented

It can happen if the best block get CL too fast. Locally it fails 1 time out of 10 with error:

2025-05-16T07:09:24.060000Z TestFramework (INFO): Mine single block, wait for chainlock
2025-05-16T07:09:24.275000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "DASH/test/functional/test_framework/test_framework.py", line 162, in main
    self.run_test()
  File "DASH/test/functional/feature_notifications.py", line 121, in run_test
    assert_equal([tip], sorted(os.listdir(self.chainlocknotify_dir)))
  File "DASH/test/functional/test_framework/util.py", line 69, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(['21b88334ae6fc29ee3bb7c89dd760dad5dc8ce1064db370dc697b3da4c4315e0'] == ['21b88334ae6fc29ee3bb7c89dd760dad5dc8ce1064db370dc697b3da4c4315e0', '4fe83a613290bd755fba25da02f493376a96971b119884831a75cea33032718d'])
2025-05-16T07:09:24.326000Z TestFramework (INFO): Stopping nodes

What was done?

We should expect CL not only for tip but for block before tip, if CL has been formed really fast.

How Has This Been Tested?

Run feature_notifications.py multiple times with these extra changes #6671

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

It can happen if the best block get CL too fast

coderabbitai · 2025-05-16T14:18:35Z

Walkthrough

The test for the -chainlocknotify feature in the functional test suite was updated to account for cases where multiple chainlock notification files may exist in the notification directory. The test now tracks the previous best block hash before mining a new block and, after waiting for the new chainlock, inspects the directory. If only one or no notification file is present, the test asserts it matches the new block's hash. If multiple files are found, the test asserts that both the new block's hash and the previous best block's hash are present, updating the assertion logic accordingly.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9cb43c2 and 21e6a4e.

📒 Files selected for processing (1)

test/functional/feature_notifications.py (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

test/functional/feature_notifications.py (2)

test/functional/test_framework/test_framework.py (2)

no_op (797-798)

wait_for_chainlocked_block_all_nodes (1668-1670)

test/functional/test_framework/util.py (1)

assert_equal (64-69)

⏰ Context from checks skipped due to timeout of 90000ms (6)

GitHub Check: x86_64-w64-mingw32 / Build depends
GitHub Check: x86_64-apple-darwin / Build depends
GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
GitHub Check: arm-linux-gnueabihf / Build depends
GitHub Check: x86_64-pc-linux-gnu / Build depends

🔇 Additional comments (2)

test/functional/feature_notifications.py (2)

112-112: Good addition to store the previous block hash

Capturing the previous tip's hash is a good preparation for handling potential race conditions where chainlocks might form rapidly on multiple blocks.

116-120: Robust handling of multiple chainlock notifications

This change successfully addresses the intermittent test failure described in the PR objectives. By checking for both the newly generated block and the previous tip when multiple notification files are found, the test now properly handles cases where chainlocks form rapidly on consecutive blocks.

The conditional logic handles both scenarios appropriately:

When only one file exists (common case): assert it's for the tip block

When multiple files exist (race condition case): assert they include both tip and previous tip

This makes the test more reliable and prevents the approximately 10% failure rate mentioned in the PR description.

✨ Finishing Touches

📝 Generate Docstrings

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

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

UdjinM6

utACK 21e6a4e

PastaPastaPasta

utACK 21e6a4e

…ional tests 0d9418e test: reduce delay in wait_until from 0.5s to 0.05s (Konstantin Akimov) 876d6c8 test: enforce 1 second delay for wait_for_sporks helper (Konstantin Akimov) ec6e7bf test: enforce 1s delay for feature_mnehf test (Konstantin Akimov) 6ab3f7c test: reduce spamming quorum list to logs while waiting (Konstantin Akimov) 9d9975f test: simplify wait_for_quorum_list (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Waiting for 0.5s in functional test for every action is a bit excessive, especially for p2p tests that sending messages by localnetwork and waiting at least 0.5 seconds before checking if message is received. ## What was done? Decreasing default delay from 0.5s to 0.05s. It affects mostly p2p tests, but many other tests become faster too. For quorum formation; for sporks and some other dash specific features bigger delays (0.5s, 1s) are used. Further improvements are blocked by #6673, #6672, #6671 and are out of scope this PR. ## How Has This Been Tested? Speed up on CI for 30% and more. [develop] linux64-test https://gitlab.com/dashpay/dash/-/jobs/10049432489 ALL | ✓ Passed | 7241 s (accumulated) Runtime: 1272 s [PR] linux64-test https://gitlab.com/dashpay/dash/-/jobs/10067158169 ALL | ✓ Passed | 5421 s (accumulated) Runtime: 938 s **-25%** [develop] linux64-nowallet https://gitlab.com/dashpay/dash/-/jobs/10049432511 ALL | ✓ Passed | 2739 s (accumulated) Runtime: 488 s [PR] linux64-nowallet https://gitlab.com/dashpay/dash/-/jobs/10067158174 ALL | ✓ Passed | 1232 s (accumulated) Runtime: 243 s **-49%** [develop] linux64-tsan https://gitlab.com/dashpay/dash/-/jobs/10049432499 ALL | ✓ Passed | 10399 s (accumulated) Runtime: 2023 s [PR] linux64-tsan https://gitlab.com/dashpay/dash/-/jobs/10072993489 ALL | ✓ Passed | 8710 s (accumulated) Runtime: 1543 s **-25%** [develop] Functional tests on localhost (-O3, debug, no sanitizers, -j20): ALL | ✓ Passed | 6680 s (accumulated) Runtime: 372 s [PR] Functional tests on localhost (-O3, debug, no sanitizers, -j20): ALL | ✓ Passed | 4609 s (accumulated) Runtime: 365 s **Benefits of running locally in 20 parallel jobs are very slight. Accumulated time is decreased for 32% as expected, but total time is improved less than 2%.** It is because the slowest tests requires many quorums to be formed and they are still slow. ## Breaking Changes N/A ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 0d9418e; hopefully it doesn't make tests flakey UdjinM6: utACK 0d9418e Tree-SHA512: 32405bd1f229af5146c96aea6031cee3f084d3ebfb3ec515ad743e79c3bc29a5c891d4330688d07b63b0e06ef7cd50240ab8b6d1a3939a56fe3e64a55918edd1

fix: intermittent error in feature_notification due to extra CL

21e6a4e

It can happen if the best block get CL too fast

knst force-pushed the fix-feature-notification branch from 11e8a5b to 21e6a4e Compare May 16, 2025 14:16

knst requested review from PastaPastaPasta and UdjinM6 May 16, 2025 19:40

knst added this to the 23 milestone May 16, 2025

UdjinM6 approved these changes May 19, 2025

View reviewed changes

knst mentioned this pull request May 20, 2025

test: set default delay to 0.05s for wait_until in functional tests #6631

Merged

5 tasks

PastaPastaPasta approved these changes May 20, 2025

View reviewed changes

PastaPastaPasta merged commit 16fcb7a into dashpay:develop May 20, 2025
61 of 62 checks passed

knst deleted the fix-feature-notification branch May 20, 2025 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: intermittent error in feature_notification due to extra CL #6672

fix: intermittent error in feature_notification due to extra CL #6672

Uh oh!

knst commented May 16, 2025

Uh oh!

coderabbitai bot commented May 16, 2025

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

UdjinM6 left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: intermittent error in feature_notification due to extra CL #6672

fix: intermittent error in feature_notification due to extra CL #6672

Uh oh!

Conversation

knst commented May 16, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai bot commented May 16, 2025

Walkthrough

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CodeRabbit Configuration File (`.coderabbit.yaml`)