feat: disable default dht discovery forwarding#7128
feat: disable default dht discovery forwarding#7128SWvheerden merged 5 commits intotari-project:developmentfrom
Conversation
- Disabled default forwarding of DHT discovery messages as this is not required for the proper operation of the network. - Adjusted the DHT dedup cache default config to be a lot more conservative before starting new dedup cycle - the amount of time an identical inbound message will be ignored before being processed again.
WalkthroughA new configuration option, Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant Dht as Dht Node
participant ForwardLayer as Forward Layer
Config->>Dht: Load DhtConfig (enable_forwarding, dedup_cache_capacity, etc.)
Dht->>Dht: Check PeerFeatures::DHT_STORE_FORWARD
Dht->>Dht: Check config.enable_forwarding
alt Both conditions true
Dht->>ForwardLayer: Activate
else
Dht->>ForwardLayer: Do not activate
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/config/presets/c_base_node_c.toml(1 hunks)common/config/presets/d_console_wallet.toml(1 hunks)comms/dht/src/config.rs(4 hunks)comms/dht/src/dht.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
🔇 Additional comments (5)
comms/dht/src/config.rs (4)
114-116: LGTM! New configuration option properly implemented.The
enable_forwardingfield is well-documented and appropriately defaults tofalse, aligning with the PR objective to disable default forwarding.
55-55: Documentation comments correctly updated for new cache defaults.The updated comments accurately reflect the significantly increased default values.
Also applies to: 58-58
174-175: Cache parameter defaults appropriately increased.The more conservative cache settings (20x capacity increase and 144x longer trim interval) should help reduce redundant message processing as intended.
191-191: Correct default value for forwarding control.Setting
enable_forwardingtofalseby default properly implements the objective to disable default DHT discovery forwarding.comms/dht/src/dht.rs (1)
275-276: Forwarding logic correctly implemented.The condition properly requires both the node's
DHT_STORE_FORWARDcapability and the newenable_forwardingconfiguration flag to be true before enabling message forwarding. This provides the intended control over DHT message forwarding behavior.
Test Results (CI) 2 files 62 suites 20m 33s ⏱️ For more details on these failures, see this check. Results for commit 30816e5. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)0 tests ±0 0 ✅ ±0 0s ⏱️ ±0s For more details on these parsing errors, see this check. Results for commit fe30606. ± Comparison against base commit 41add9f. ♻️ This comment has been updated with latest results. |
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
I am slightly weary that this might cause some unintended breakage down the line
2 participants
Description
Motivation and Context
Mainnet is being flooded with messages that do not contribute to the blockchain.
How Has This Been Tested?
System-level testing shows markedly reduced overheads, inbound pipeline finish times and overall memory usage. The dedup cache also does its job much better now so the base node does not can ignore the endless round robbin of the messages.
What process can a PR reviewer use to test or verify this change?
Code review.
System-level testing.
Breaking Changes
Summary by CodeRabbit
New Features
Improvements
Documentation