Skip to content

test: return chainlock height from wait_for_wallet_tx_chainlocked#766

Open
xdustinface wants to merge 1 commit into
v0.42-devfrom
test/flaky-chainlock-promotion-height
Open

test: return chainlock height from wait_for_wallet_tx_chainlocked#766
xdustinface wants to merge 1 commit into
v0.42-devfrom
test/flaky-chainlock-promotion-height

Conversation

@xdustinface
Copy link
Copy Markdown
Collaborator

@xdustinface xdustinface commented May 14, 2026

The helper's BlockProcessed { chain_lock: Some(_), .. } branch returned the tx's own block height instead of the chainlock's height, which broke test_chainlock_promotes_in_block_tx whenever the SPV client processed the tx's block after the chainlock had already arrived (the common ordering when filter sync runs ahead of block delivery). In that case the wallet emits BlockProcessed with the chainlock attached, the helper returned the tx block height, and the test's promoted_at >= cl_height assertion failed even though the tx was correctly chainlocked.

Summary by CodeRabbit

  • Tests
    • Improved wallet transaction chain lock verification in test infrastructure for enhanced reliability.

Review Change Stack

The helper's `BlockProcessed { chain_lock: Some(_), .. }` branch returned the tx's own block height instead of the chainlock's height, which broke `test_chainlock_promotes_in_block_tx` whenever the SPV client processed the tx's block after the chainlock had already arrived (the common ordering when filter sync runs ahead of block delivery). In that case the wallet emits `BlockProcessed` with the chainlock attached, the helper returned the tx block height, and the test's `promoted_at >= cl_height` assertion failed even though the tx was correctly chainlocked.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7aec60aa-4f27-467d-ad17-02084a219517

📥 Commits

Reviewing files that changed from the base of the PR and between 1288884 and 5bd26f8.

📒 Files selected for processing (1)
  • dash-spv/tests/dashd_masternode/helpers.rs

📝 Walkthrough

Walkthrough

The test helper function wait_for_wallet_tx_chainlocked is updated to handle the BlockProcessed event path by extracting the chain lock's block height directly and returning it when the target transaction is found in inserted or updated records, replacing the previous context-derived height logic.

Changes

Wallet Transaction Chain Lock Helper Update

Layer / File(s) Summary
BlockProcessed match arm — chain lock height extraction
dash-spv/tests/dashd_masternode/helpers.rs
The BlockProcessed event handler extracts chain_lock.block_height and returns it directly when the target txid is found, simplifying the logic that previously derived height from the matching record's context with fallback behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit finds the quickest way,
Through chain lock heights to seize the day—
No roundabout through context goes,
Direct extraction—wisdom knows! 🐰⛓️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: returning chainlock height from the test helper function instead of transaction block height.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/flaky-chainlock-promotion-height

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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

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

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.59%. Comparing base (5297d61) to head (5bd26f8).
⚠️ Report is 2 commits behind head on v0.42-dev.

Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #766      +/-   ##
=============================================
+ Coverage      72.26%   72.59%   +0.33%     
=============================================
  Files            320      320              
  Lines          70275    70271       -4     
=============================================
+ Hits           50785    51016     +231     
+ Misses         19490    19255     -235     
Flag Coverage Δ
core 76.30% <ø> (ø)
ffi 48.42% <ø> (+2.32%) ⬆️
rpc 20.00% <ø> (ø)
spv 89.83% <ø> (+0.07%) ⬆️
wallet 71.27% <ø> (+0.01%) ⬆️
see 18 files with indirect coverage changes

@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label May 14, 2026
@xdustinface xdustinface requested a review from ZocoLini May 14, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant