Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Masternodes immediately propagating transactions to all peers can leak some info about network topography, and could be used to identify which masternode or masternodes were likely first sent a transaction. It also places additional load on masternodes.

Allowing masternodes to trickle out transactions to non-masternode peers allows them to batch multiple transactions in a single INV notification, while not affecting the performance of InstantSend as txes from MN to MN continue to be sent immediately.

What was done?

Only immediately send transactions if both the sender and receiver are masternodes

How Has This Been Tested?

Breaking Changes

This will make testing "instantsend latency" a bit harder, as the propogation of the initial transaction may be delayed to us, if we are not a masternode. For testing, we should consider having multiple masternodes connection to us via addnode RPC which should set the NetPermissionFlags::NoBan flag and allow us to receive the TX in more real time.

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Dec 4, 2025
@github-actions
Copy link

github-actions bot commented Dec 4, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

A single file change in src/net_processing.cpp modifies the condition for enabling trickle-relay of inventory. The logic shifts from a global Masternode check combined with NoBan permission to a per-peer check. A new local variable peer_is_mn is introduced, based on peer ProRegTx hash verification, and is used to restrict trickling to cases where both the local node and the connected peer are Masternodes. This narrows the eligibility criteria for periodic inventory relaying.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

  • Areas requiring extra attention:
    • Verification that peer_is_mn determination (based on ProRegTx hash verification) correctly identifies peer Masternode status
    • Impact of the AND condition (local Masternode AND peer Masternode) on inventory propagation and network behavior
    • Whether existing tests cover the modified trickle-relay scenarios, particularly edge cases involving non-Masternode peers
    • Confirmation that the narrowed trickling criteria align with intended network protocol behavior and don't inadvertently degrade inventory distribution

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: trickling transactions from masternodes to non-masternodes, which is the core feature implemented in the changeset.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation, implementation details, testing implications, and acknowledging incomplete checklist items relevant to the code changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b89585 and f69d946.

📒 Files selected for processing (1)
  • src/net_processing.cpp (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/net_processing.cpp
🧠 Learnings (1)
📓 Common learnings
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: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
🔇 Additional comments (1)
src/net_processing.cpp (1)

6230-6236: MN-to-MN trickle bypass logic matches the PR intent

The new peer_is_mn check and fSendTrickle initialization correctly restrict “immediate” (non-trickled) tx invs to:

  • peers with NetPermissionFlags::NoBan, and
  • the case where both local node and peer are verified Masternodes.

This preserves fast MN↔MN propagation while ensuring MN→non-MN traffic is still trickled, which aligns with the stated privacy and load goals. The use of GetVerifiedProRegTxHash() is appropriate (only authenticated MNs benefit), and it reuses the same notion of a “MN peer” already used for the half-delay scheduling below, so there are no new consistency or locking concerns here.


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.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK f69d946

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants