test: fix flaky v2transport_test off-by-one in Dash short ID range

[UdjinM6]

Issue being fixed or feature implemented

The test was generating "unknown" short IDs in range [168, 255], but 168 is PLATFORMBAN (last valid Dash ID). When InsecureRandRange(88) returned 0, the test would send a valid message that decoded successfully, causing the BOOST_CHECK(!(*ret)[0]) assertion to fail.

Follow-up to #7082

What was done?

Fix by starting the invalid range at 169 instead of 168.

Fixes #7093

How Has This Been Tested?

Run ./src/test/test_dash --run_test=net_tests/v2transport_test in a loop

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 (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

A test in net_tests.cpp is updated to validate handling of invalid short IDs in the v2transport protocol. The test now generates short IDs outside valid Bitcoin/Dash ID ranges and updates corresponding test expectations: the first received message should be null (non-delivery), while the subsequent message should arrive with a valid short ID ("block"). Comments are clarified to indicate intentional use of invalid IDs, and the final call to SendMessage passes an empty payload list for the unknown-ID case.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing an off-by-one bug in the v2transport_test related to Dash short ID range validation.
Description check	✅ Passed	The description clearly explains the bug (off-by-one in short ID range [168, 255]), the root cause (168 is PLATFORMBAN), the fix (start at 169), and how it was tested.
Linked Issues check	✅ Passed	The PR directly addresses the root cause of the flaky test reported in issue #7093 by fixing the off-by-one error in short ID range generation that caused nondeterministic test behavior.
Out of Scope Changes check	✅ Passed	All changes are focused on fixing the specific off-by-one bug in v2transport_test; no unrelated modifications are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7482956 and 289df5f.

📒 Files selected for processing (1)

• src/test/net_tests.cpp

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,hpp,cc}

📄 CodeRabbit inference engine (CLAUDE.md)

Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1

Files:

• src/test/net_tests.cpp

src/{test,wallet/test}/**/*.{cpp,h}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework

Files:

• src/test/net_tests.cpp

🧠 Learnings (3)

📓 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: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

📚 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/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay

Applied to files:

• src/test/net_tests.cpp

📚 Learning: 2025-08-08T07:01:47.332Z

Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.

Applied to files:

• src/test/net_tests.cpp

🧬 Code graph analysis (1)

src/test/net_tests.cpp (1)

src/test/util/setup_common.h (1)

• InsecureRandRange (83-83)

⏰ 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_fuzz-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: Lint / Run linters

🔇 Additional comments (2)

src/test/net_tests.cpp (2)

1552-1567: LGTM! Off-by-one bug correctly fixed.

The changes properly exclude PLATFORMBAN (ID 168) from the invalid short ID range, fixing the flaky test. The new logic correctly generates IDs in two invalid ranges:

• Bitcoin invalid: [33, 127]
• Dash invalid: [169, 255]

The test expectations are correctly updated: the first received message should be null (rejected due to invalid short ID), and the second should be a valid "block" message.

---

1587-1665: Excellent test coverage for version negotiation.

This test case thoroughly validates that PLATFORMBAN (short ID 168) encoding correctly respects peer protocol version:

• Baseline peers (v70235) work with IDs 0-167
• Old peers (v70238) receive long encoding for PLATFORMBAN
• New peers (v70240+) receive short encoding (ID 168)

This ensures backwards compatibility when adding new short IDs.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[knst]

utACK 289df5f

[kwvg]

utACK 289df5f