backport: Merge bitcoin/bitcoin#23418, 25480, 25492

[vijaydasmp]

bitcoin backports

[coderabbitai]

Walkthrough

The changes span several core components of the system. In the network processing module, methods within the peer management class have been modified so that functions dealing with transaction requests and misbehavior now use direct peer references instead of node identifiers. The RPC functionality has been adjusted to convert ping-related metrics using a different function, thereby changing the underlying representation of timing values. In the transaction memory pool, fee management has been revised: existing functions have been renamed and updated to use a saturating addition mechanism, and the internal fee variable has been replaced to streamline fee calculations. Additionally, the build process has been altered by removing MSVC warning suppression directives and an inline utility function for duration conversion. Finally, changes to sanitation suppressions update comment text and remove a specific suppression entry in the UBSan configuration.

Tip

🌐 Web search-backed reviews and chat

• We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
• You can disable this feature by setting web_search: false in the knowledge_base settings.
• Please share any feedback in the Discord discussion.

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/rpc/net.cpp (1)

231-231: LGTM: Completed modernization of time handling

The change completes the consistent update of time handling across all ping-related measurements.

These changes are part of a broader improvement in time handling across the codebase, moving towards type-safe chrono-based time calculations. This reduces the risk of time-related bugs and improves code maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c166e1 and 3847b1a.

📒 Files selected for processing (13)

• src/llmq/dkgsessionhandler.cpp (1 hunks)
• src/llmq/instantsend.cpp (1 hunks)
• src/llmq/signing.cpp (1 hunks)
• src/llmq/signing_shares.cpp (1 hunks)
• src/net_processing.cpp (27 hunks)
• src/net_processing.h (1 hunks)
• src/rpc/net.cpp (1 hunks)
• src/test/denialofservice_tests.cpp (4 hunks)
• src/txmempool.cpp (7 hunks)
• src/txmempool.h (2 hunks)
• src/util/system.cpp (0 hunks)
• src/util/time.h (0 hunks)
• test/sanitizer_suppressions/ubsan (1 hunks)

💤 Files with no reviewable changes (2)

• src/util/time.h
• src/util/system.cpp

✅ Files skipped from review due to trivial changes (1)

• src/llmq/signing_shares.cpp

🔇 Additional comments (31)

src/rpc/net.cpp (3)

225-225: LGTM: Improved time handling using modern C++ chrono

The change from CountSecondsDouble to Ticks<SecondsDouble> represents a move towards more type-safe time handling using the C++ chrono library.

---

228-228: LGTM: Consistent time handling for minimum ping time

The change maintains consistency with the modern time handling approach.

---

225-231: Verify complete migration of time handling methods

Let's verify that all instances of CountSecondsDouble have been migrated to the new Ticks<SecondsDouble> approach in the codebase.

✅ Verification successful

Time handling migration successfully completed

The codebase shows complete adoption of Ticks<SecondsDouble> with no remaining instances of CountSecondsDouble, confirming successful migration of the time handling methods.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of CountSecondsDouble
echo "Checking for remaining CountSecondsDouble usage..."
rg "CountSecondsDouble" --type cpp

# Search for consistent usage of Ticks<SecondsDouble>
echo "Verifying Ticks<SecondsDouble> usage..."
rg "Ticks<SecondsDouble>" --type cpp

Length of output: 685

src/txmempool.cpp (5)

104-104: LGTM: Member variable initialization

The initialization of m_modified_fee with nFee in the constructor is correct and matches the new fee handling approach.

---

458-458: LGTM: Consistent use of SaturatingAdd for fee updates

The use of SaturatingAdd is consistently applied to both descendant and ancestor fee updates, maintaining the overflow protection throughout the codebase.

Also applies to: 467-467

---

1465-1468: LGTM: Fee delta handling with overflow protection

The changes correctly apply SaturatingAdd to fee delta calculations and properly update the modified fees through the mempool entry modification mechanism.

---

515-516: Verify assumption about fee modification state

The code assumes that the initial fee matches the modified fee. While this is likely true for new entries, we should verify this assumption holds in all code paths.

✅ Verification successful

Assumption about fee modification state is valid

The assumption is correct by design. The code makes this assertion right after inserting a new entry and applying any potential fee delta from PrioritiseTransaction, which are the only two operations that could affect the fees at this point.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for places where fees might be modified before reaching this point
rg -B 5 'GetModifiedFee|UpdateModifiedFee' src/

Length of output: 9042

---

112-116: Verify overflow protection in fee updates

The use of SaturatingAdd for fee updates provides protection against integer overflow, which is important for fee calculations. The changes look correct but we should verify that all fee-related calculations are consistently using this protection.

✅ Verification successful

Fee calculations have appropriate overflow protection

The codebase consistently uses proper overflow protection for fee calculations:

• Consensus-critical paths validate fees using MoneyRange checks
• Memory pool modifications use SaturatingAdd
• Transaction validation enforces fee range limits

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining direct fee additions that might need overflow protection
rg -A 2 '\+= .*[Ff]ee' src/

Length of output: 4250

---

Script:

#!/bin/bash
# Check for MoneyRange usage near fee calculations
rg -B 2 -A 2 'MoneyRange.*fee|fee.*MoneyRange' src/

# Check for other potential overflow protections
rg -B 2 -A 2 'Saturating(Add|Sub).*fee|fee.*Saturating(Add|Sub)' src/

Length of output: 2694

src/net_processing.cpp (11)

635-635: LGTM: Unit test misbehavior handling

The UnitTestMisbehaving method provides a clean way to test misbehavior handling in unit tests while maintaining encapsulation of the actual implementation.

---

676-681: Improved misbehavior tracking with peer reference

The new Misbehaving method signature using Peer& provides better encapsulation and direct access to peer state. This change improves code organization and maintainability.

---

740-740: LGTM: Updated SendBlockTransactions signature

The addition of the Peer& parameter to SendBlockTransactions aligns with the pattern of direct peer state access.

---

3494-3495: LGTM: Consistent error handling in message processing

The error handling in message processing is consistent, using the misbehavior tracking system uniformly across different message types.

Also applies to: 3500-3502

---

Line range hint 3983-3996: LGTM: Address rate limiting with token bucket

The implementation of address rate limiting using a token bucket approach is well-designed and properly handles the time-based token accumulation.

---

4086-4086: LGTM: Consistent inventory size checks

The size checks for INV and GETDATA messages are consistent and use appropriate misbehavior scores.

Also applies to: 4193-4193

---

4706-4706: LGTM: Block validation error handling

The error handling for invalid compact blocks and block transactions is appropriate and uses consistent misbehavior scores.

Also applies to: 4832-4832

---

5101-5101: LGTM: Bloom filter validation

The validation of bloom filter size and data is thorough and uses appropriate misbehavior scores for violations.

Also applies to: 5137-5137

---

Line range hint 5166-5175: LGTM: Masternode message validation

The validation of masternode-related messages is comprehensive and uses appropriate misbehavior scores for different types of violations.

Also applies to: 5197-5197, 5213-5213, 5220-5220

---

3628-3634: Verify devnet version check error handling

The devnet version check error handling looks correct but we should verify the misbehavior scores are appropriate for different scenarios.

✅ Verification successful

Misbehavior scores for devnet version mismatches are appropriately calibrated

The different scores (100 for outbound, 1 for inbound) are well-reasoned:

• Higher score for outbound prevents wasteful reconnection attempts to wrong networks
• Lower score for inbound allows honest mistakes without immediate harsh penalties

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistency in devnet version handling
rg -B 5 'devnet' src/

Length of output: 70141

---

1781-1802: Verify thread safety of misbehavior tracking

The misbehavior tracking implementation looks correct but we should verify the thread safety of the score updates and disconnection logic.

✅ Verification successful

Thread safety implementation is correct and comprehensive

The misbehavior tracking implementation demonstrates proper thread safety through:

• Dedicated mutex (m_misbehavior_mutex) protecting all misbehavior state
• Consistent lock usage across all access points
• Clear thread safety annotations using GUARDED_BY
• No potential deadlock scenarios

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential race conditions in peer state access
ast-grep --pattern 'm_misbehavior_score' src/
ast-grep --pattern 'm_should_discourage' src/

Length of output: 423

---

Script:

#!/bin/bash
# Search for misbehavior related variables and mutex with context
rg -A 3 -B 3 "m_misbehavior_score" src/
rg -A 3 -B 3 "m_should_discourage" src/
rg -A 3 -B 3 "m_misbehavior_mutex" src/

Length of output: 8267

test/sanitizer_suppressions/ubsan (1)

3-6: Documentation improvement for suppression pattern format.

The comment now uses a more generic format sanitize-type:ClassName::MethodName instead of a specific example, making it clearer how to add new suppressions.

src/net_processing.h (1)

122-123: Method renamed to clarify its testing purpose.

The Misbehaving method has been replaced with UnitTestMisbehaving, making it explicit that this method is intended for unit testing scenarios. This change improves code clarity and separates test functionality from production code.

src/test/denialofservice_tests.cpp (4)

353-353: Updated test for peer discouragement threshold.

Test now uses UnitTestMisbehaving to verify peer discouragement at threshold, aligning with the new testing approach.

---

372-372: Updated tests for incremental misbehavior.

Tests now use UnitTestMisbehaving to verify:

1. Below-threshold misbehavior (no discouragement)
2. Reaching threshold through multiple violations

Also applies to: 380-380

---

403-403: Updated test for non-IP peer handling.

Test now uses UnitTestMisbehaving to verify discouragement behavior for non-IP peers (e.g., onion addresses).

---

449-449: Updated test for ban time functionality.

Test now uses UnitTestMisbehaving to verify the ban time implementation.

src/llmq/dkgsessionhandler.cpp (1)

113-113: Updated misbehavior reporting method.

The method now uses UnitTestMisbehaving to report peer misbehavior, maintaining consistency with the test-focused approach adopted across the codebase.

src/llmq/signing.cpp (1)

583-583: Verify the impact of switching to UnitTestMisbehaving.

The change from Misbehaving to UnitTestMisbehaving alters how peer misbehavior is handled. This appears to be part of a broader refactor to improve testing capabilities, but we should ensure this doesn't affect production behavior.

Let's verify if this change is consistently applied across the codebase:

✅ Verification successful

The change to UnitTestMisbehaving is a safe refactor

The switch from Misbehaving to UnitTestMisbehaving is part of a consistent pattern across the codebase where UnitTestMisbehaving is the public interface method that internally calls the private Misbehaving implementation. This change improves encapsulation without altering the underlying behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining direct usage of Misbehaving
rg "Misbehaving\(" --type cpp

# Check for new UnitTestMisbehaving usage
rg "UnitTestMisbehaving\(" --type cpp

Length of output: 5967

src/txmempool.h (3)

116-116: LGTM! Improved member variable naming.

The change from feeDelta to m_modified_fee follows better naming conventions with the m_ prefix for member variables and provides a more descriptive name.

---

145-145: LGTM! Simplified fee access.

The updated GetModifiedFee() implementation directly returns m_modified_fee instead of calculating it on the fly, which is more efficient.

---

153-154: LGTM! Improved method naming.

The change from UpdateFeeDelta to UpdateModifiedFee better reflects the method's purpose and maintains consistency with the new member variable name.

src/llmq/instantsend.cpp (1)

941-941: Verify consistent behavior with UnitTestMisbehaving.

Similar to the change in signing.cpp, this replaces Misbehaving with UnitTestMisbehaving. We should ensure this change maintains the same penalty score (20) and behavior.

Let's verify the penalty scores are consistent:

✅ Verification successful

Penalty score of 20 is correct and intentionally different

The lower penalty score in instantsend.cpp is a deliberate design choice, as indicated by the code comment: the peer might simply be unlucky due to quorum changes, so a less harsh penalty is appropriate compared to clear protocol violations that receive 100 points.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check all UnitTestMisbehaving calls and their scores
rg "UnitTestMisbehaving.*\(\w+,\s*(\d+)\)" --type cpp

Length of output: 282

---

Script:

#!/bin/bash
# Get context around UnitTestMisbehaving calls
echo "=== signing.cpp ==="
rg "UnitTestMisbehaving.*\(\w+,\s*(\d+)\)" src/llmq/signing.cpp -B 5 -A 5
echo -e "\n=== instantsend.cpp ==="
rg "UnitTestMisbehaving.*\(\w+,\s*(\d+)\)" src/llmq/instantsend.cpp -B 5 -A 5
echo -e "\n=== signing_shares.cpp ==="
rg "UnitTestMisbehaving.*\(\w+,\s*(\d+)\)" src/llmq/signing_shares.cpp -B 5 -A 5

# Also check for any comments mentioning misbehaving scores
echo -e "\n=== Comments about misbehaving ==="
rg "misbehav\w+" src/llmq/ -i --type cpp

Length of output: 2943

[UdjinM6]

25144: I don't like this approach, we need to find a better way

[vijaydasmp]

Sure , I will try another approach

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/net_processing.cpp (2)

676-681: LGTM! Thread-safe misbehavior tracking implementation.

The new Misbehaving implementation correctly uses mutex protection for the misbehavior score and adds useful stats tracking. The logging is also improved to show the score transition.

Consider adding these improvements:

 void Misbehaving(Peer& peer, int howmuch, const std::string& message)
 {
     assert(howmuch > 0);
+    // Early return if peer has NetPermissionFlags::NoBan
+    if (m_connman.HasPermission(peer.m_id, NetPermissionFlags::NoBan)) {
+        return;
+    }
     LOCK(peer.m_misbehavior_mutex);
     const int score_before{peer.m_misbehavior_score};
     peer.m_misbehavior_score += howmuch;
     const int score_now{peer.m_misbehavior_score};
+    // Track maximum score seen for this peer
+    peer.m_max_misbehavior_score = std::max(peer.m_max_misbehavior_score, score_now);

This would:

1. Add early exit for peers with ban immunity
2. Track the maximum misbehavior score seen for each peer

Also applies to: 1781-1802

---

3156-3156: LGTM! Consistent usage of Misbehaving across the codebase.

The Misbehaving function is used consistently throughout the codebase with appropriate error messages and penalty scores.

Consider standardizing the error message format across all call sites:

-Misbehaving(*peer, 20, strprintf("headers message size = %u", nCount));
+Misbehaving(*peer, 20, strprintf("%s: message size = %u exceeds limit %u", __func__, nCount, MAX_HEADERS));

This would:

1. Include the function name for better error tracing
2. Consistently show the limit that was exceeded
3. Use a standard format across all oversized message errors

Also applies to: 3163-3163, 3983-3983, 4086-4086, 4193-4193, 4706-4706, 4832-4832, 4896-4896, 5101-5101, 5137-5137

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3847b1a and 4640813.

📒 Files selected for processing (6)

• src/net_processing.cpp (27 hunks)
• src/net_processing.h (1 hunks)
• src/rpc/net.cpp (1 hunks)
• src/test/denialofservice_tests.cpp (4 hunks)
• src/util/system.cpp (0 hunks)
• src/util/time.h (0 hunks)

💤 Files with no reviewable changes (2)

• src/util/time.h
• src/util/system.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• src/test/denialofservice_tests.cpp
• src/rpc/net.cpp

⏰ Context from checks skipped due to timeout of 90000ms (2)

• GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
• GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)

🔇 Additional comments (3)

src/net_processing.h (1)

122-123: Verify the backport's completeness and security implications.

The replacement of Misbehaving with UnitTestMisbehaving represents a significant change in peer behavior management. This change requires careful verification to ensure:

1. All production code paths have been updated to handle peer misbehavior appropriately
2. The change doesn't weaken the network's defense against malicious peers

Let's verify the completeness of this backport:

✅ Verification successful

The implementation is correct and complete

The UnitTestMisbehaving method is intentionally designed as a test-friendly wrapper around the core Misbehaving implementation. The production code correctly uses Misbehaving directly, while UnitTestMisbehaving provides a simplified interface for test code. This matches the common pattern of exposing test-specific methods while keeping the core implementation intact.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all calls to Misbehaving have been properly migrated

# Check for any remaining references to the old Misbehaving method
echo "Checking for remaining Misbehaving references..."
rg -l "Misbehaving\(" --type cpp

# Look for the new UnitTestMisbehaving usage
echo "Checking UnitTestMisbehaving usage..."
rg -l "UnitTestMisbehaving\(" --type cpp

# Check test files to ensure they're using the new method
echo "Checking test files..."
rg -l "Misbehaving" --type cpp test/

Length of output: 666

---

Script:

#!/bin/bash
# Check the implementation details and relationship between Misbehaving and UnitTestMisbehaving

# Look at the implementation in net_processing.cpp
echo "=== Checking net_processing.cpp implementations ==="
rg "Misbehaving|UnitTestMisbehaving" src/net_processing.cpp -B 2 -A 5

# Check if there are any TODOs or comments about migration
echo -e "\n=== Checking for related comments ==="
rg -i "todo.*misbehav|migrate.*misbehav" src/

# Look for any test documentation about this change
echo -e "\n=== Checking test documentation ==="
rg "Misbehaving" test/functional/README.md test/README.md 2>/dev/null || true

Length of output: 10028

src/net_processing.cpp (2)

635-635: LGTM! Clean unit test helper implementation.

The UnitTestMisbehaving function provides a clean way to test misbehavior handling while ensuring the peer exists through Assert.

---

Line range hint 5166-5175: LGTM! Appropriate penalty levels for masternode-specific misbehavior.

The code correctly differentiates between non-malicious failures (1 point) and protocol violations (100 points) for masternode-related messages.

Also applies to: 5197-5197, 5213-5213, 5220-5220

[vijaydasmp]

dropped 25144 at the moment, will look at it separately , requesting review

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (6)

src/net_processing.cpp (6)

3985-3991: Consider using std::clamp for token bucket increment

The token bucket increment calculation could be simplified and made safer using std::clamp.

-            const double increment = Ticks<SecondsDouble>(time_diff) * MAX_ADDR_RATE_PER_SECOND;
-            peer->m_addr_token_bucket = std::min<double>(peer->m_addr_token_bucket + increment, MAX_ADDR_PROCESSING_TOKEN_BUCKET);
+            const double increment = std::clamp(
+                Ticks<SecondsDouble>(time_diff) * MAX_ADDR_RATE_PER_SECOND,
+                0.0,
+                MAX_ADDR_PROCESSING_TOKEN_BUCKET - peer->m_addr_token_bucket
+            );
+            peer->m_addr_token_bucket += increment;

---

6201-6215: Add overflow protection for stalling timeout doubling

The stalling timeout doubling could potentially overflow. Consider adding explicit overflow checks and more descriptive logging.

-            const auto new_timeout = std::min(2 * stalling_timeout, BLOCK_STALLING_TIMEOUT_MAX);
+            const auto new_timeout = std::min(
+                (stalling_timeout > BLOCK_STALLING_TIMEOUT_MAX / 2) ? 
+                    BLOCK_STALLING_TIMEOUT_MAX : 2 * stalling_timeout,
+                BLOCK_STALLING_TIMEOUT_MAX
+            );
             if (stalling_timeout != new_timeout && m_block_stalling_timeout.compare_exchange_strong(stalling_timeout, new_timeout)) {
-                LogPrint(BCLog::NET, "Increased stalling timeout temporarily to %d seconds\n", count_seconds(new_timeout));
+                LogPrint(BCLog::NET, "Increased stalling timeout from %d to %d seconds due to slow peer\n",
+                    count_seconds(stalling_timeout), count_seconds(new_timeout));
             }

---

6231-6261: Extract magic numbers into named constants

The headers sync timeout logic uses magic numbers that should be named constants. Also consider more descriptive variable names.

+    // Maximum acceptable age of our chain tip during headers sync
+    static constexpr auto HEADERS_SYNC_MAX_TIP_AGE = 10min;
+
     if (state.fSyncStarted && state.m_headers_sync_timeout < std::chrono::microseconds::max()) {
         // Detect whether this is a stalling initial-headers-sync peer
-        if (m_chainman.m_best_header->GetBlockTime() <= GetAdjustedTime() - nMaxTipAge) {
+        if (m_chainman.m_best_header->GetBlockTime() <= GetAdjustedTime() - HEADERS_SYNC_MAX_TIP_AGE) {
             if (current_time > state.m_headers_sync_timeout && nSyncStarted == 1 && 
                 (m_num_preferred_download_peers - state.fPreferredDownload >= 1)) {

---

6263-6266: Improve documentation of peer eviction logic

The peer eviction logic could benefit from more detailed documentation explaining the criteria and rationale.

+        // Check if outbound peers should be evicted based on:
+        // 1. Chain staleness - peer's chain work is below our minimum requirements
+        // 2. Sync timeout - peer has not provided new blocks within timeout period
+        // 3. Download window - peer is stalling our block download progress
         ConsiderEviction(*pto, *peer, GetTime<std::chrono::seconds>());

---

6271-6287: Extract block download configuration into named constants

The block download logic uses magic numbers and could benefit from more descriptive variable names.

+    // Maximum number of blocks that can be in-flight to a single peer
+    static constexpr int MAX_PARALLEL_BLOCK_DOWNLOADS = MAX_BLOCKS_IN_TRANSIT_PER_PEER;
+
     if (CanServeBlocks(*peer) && pto->CanRelay() && 
         ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || 
           !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && 
-        state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
+        state.nBlocksInFlight < MAX_PARALLEL_BLOCK_DOWNLOADS) {
         std::vector<const CBlockIndex*> vToDownload;
         NodeId staller = -1;
-        FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, 
+        FindNextBlocksToDownload(*peer, MAX_PARALLEL_BLOCK_DOWNLOADS - state.nBlocksInFlight,
             vToDownload, staller);

---

6298-6311: Improve object expiry documentation and naming

The object expiry logic could benefit from more descriptive variable names and detailed documentation of the rate limiting mechanism.

+    // Check for expired in-flight objects and clean them up.
+    // We randomize the check interval to avoid all peers checking at the same time.
     if (state.m_object_download.m_check_expiry_timer <= current_time) {
         for (auto it=state.m_object_download.m_object_in_flight.begin(); 
              it != state.m_object_download.m_object_in_flight.end();) {
-            if (it->second <= current_time - GetObjectExpiryInterval(it->first.type)) {
+            const auto& object_inv = it->first;
+            const auto& request_time = it->second;
+            const auto expiry_interval = GetObjectExpiryInterval(object_inv.type);
+            if (request_time <= current_time - expiry_interval) {
                 LogPrint(BCLog::NET, "timeout of inflight object %s from peer=%d\n", 
-                    it->first.ToString(), pto->GetId());
+                    object_inv.ToString(), pto->GetId());

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4640813 and 140622f.

📒 Files selected for processing (4)

• src/net_processing.cpp (1 hunks)
• src/rpc/net.cpp (1 hunks)
• src/util/system.cpp (0 hunks)
• src/util/time.h (0 hunks)

💤 Files with no reviewable changes (2)

• src/util/time.h
• src/util/system.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/rpc/net.cpp

[UdjinM6]

utACK 140622f

[vijaydasmp]

Hello @PastaPastaPasta, requesting review

[PastaPastaPasta]

utACK 140622f