backport: Merge bitcoin#29009, 29079

[vijaydasmp]

bitcoin backports

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

This pull request refactors the fuzzing and test infrastructure for message processing. The primary change involves modifying ProcessMessagesOnce in the test utilities to return a boolean value indicating whether additional message processing is needed. The fuzz tests (process_message.cpp and process_messages.cpp) are updated to use this return value to drive a loop that continues processing until no further work remains. The message handling flow is restructured to use CSerializedNetMsg, ReceiveMsgFrom, and a processing loop with error handling, replacing the previous direct ProcessMessage invocation approach.

Sequence Diagram

sequenceDiagram
participant FuzzTest as Fuzz Test
participant ConnMan as ConnMan
participant PeerMan as PeerMan
participant MsgProc as Message Processor
FuzzTest->>ConnMan: ReceiveMsgFrom(net_msg)
activate ConnMan
ConnMan->>ConnMan: Store message in queue
deactivate ConnMan
loop Process until no more work
FuzzTest->>ConnMan: ProcessMessagesOnce(node)
activate ConnMan
ConnMan->>MsgProc: ProcessMessages(node)
activate MsgProc
MsgProc-->>ConnMan: bool (more work needed?)
deactivate MsgProc
ConnMan-->>FuzzTest: return bool
deactivate ConnMan
alt More messages to process
FuzzTest->>PeerMan: SendMessages()
activate PeerMan
PeerMan->>PeerMan: Flush pending sends
deactivate PeerMan
else No more messages
Note over FuzzTest: Exit loop
end
end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title accurately describes the main change: backporting two specific Bitcoin commits (29009 and 29079).
Description check	✅ Passed	The description is very brief but clearly related to the changeset, indicating it contains Bitcoin backports as confirmed by the objectives.

_{✏️ 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 34a9eebf7f5ed45ba60878413d85247fd2fe819e.

📒 Files selected for processing (3)

• src/test/fuzz/process_message.cpp
• src/test/fuzz/process_messages.cpp
• src/test/util/net.h

🧰 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/util/net.h
• src/test/fuzz/process_message.cpp
• src/test/fuzz/process_messages.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/util/net.h
• src/test/fuzz/process_message.cpp
• src/test/fuzz/process_messages.cpp

🧠 Learnings (4)

📓 Common learnings

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

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: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.

Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.

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-12-22T15:42:48.595Z

Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.

Applied to files:

• src/test/util/net.h

📚 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/fuzz/process_message.cpp

📚 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,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing

Applied to files:

• src/test/fuzz/process_message.cpp

🧬 Code graph analysis (1)

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

src/net.h (1)

• NetEventsInterface (1143-1179)

🔇 Additional comments (6)

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

73-76: LGTM!

The signature change from void to bool correctly propagates the return value from ProcessMessages, which indicates whether there is more work to be done. This aligns with the NetEventsInterface::ProcessMessages interface and enables the fuzz tests to loop until all messages are fully processed, supporting the deadlock detection objectives of this backport.

src/test/fuzz/process_messages.cpp (2)

65-65: LGTM!

Using MAX_PROTOCOL_MESSAGE_LENGTH as the maximum byte vector length addresses the OSS-Fuzz issue (bitcoin#29079) by constraining fuzz input to valid protocol message sizes.

---

72-81: LGTM!

The loop-based processing correctly implements the deadlock detection mechanism from bitcoin#29009:

• Continues processing until ProcessMessagesOnce returns false (no more work)
• Resets fPauseSend each iteration to prevent artificial blocking
• The empty catch for std::ios_base::failure is appropriate for fuzz testing where malformed data is expected
• Moving SendMessages inside the loop ensures complete send/receive cycles on each iteration

src/test/fuzz/process_message.cpp (3)

1-1: LGTM!

Copyright year updated to "2020-present" consistent with upstream Bitcoin Core convention.

---

72-77: LGTM!

The refactored message handling uses the proper public interface:

• CSerializedNetMsg construction with m_type and data fields
• MAX_PROTOCOL_MESSAGE_LENGTH constraint aligns with fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH bitcoin/bitcoin#29079
• FlushSendBuffer before ReceiveMsgFrom ensures clean state
• std::move for the net message is correct for ownership transfer

---

79-87: LGTM!

The processing loop correctly mirrors the pattern in process_messages.cpp:

• Loops until ProcessMessagesOnce returns false, ensuring complete message processing
• Resets fPauseSend to prevent artificial blocking
• Empty catch for std::ios_base::failure handles expected malformed fuzz input
• SendMessages inside the loop ensures proper send/receive cycle completion

This implements the deadlock detection mechanism from bitcoin#29009.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.}

[UdjinM6]

utACK 34a9eebf7f5ed45ba60878413d85247fd2fe819e

[vijaydasmp]

Hello @PastaPastaPasta , requesting review