Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Oct 21, 2025

Issue being fixed or feature implemented

We don't have BIP-125 (replace-by-fee) and there's no plans to support it ever.

What was done?

Removed leftover that is backported by mistake

How Has This Been Tested?

N/A

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

@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

This pull request removes references to BIP 125 transaction replacement functionality across multiple files. Changes include updating documentation strings and comments in src/init.cpp and src/policy/policy.h to remove BIP 125 mentions, removing the WALLET_INCREMENTAL_RELAY_FEE constant from src/wallet/wallet.h, removing a compile-time assertion from src/wallet/test/wallet_tests.cpp that enforced wallet fee constraints, and updating a comment in the test framework regarding BIP125_SEQUENCE_NUMBER. No runtime logic or control flow is altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes are primarily documentation updates and removal of related constants. While spanning five files, the modifications follow a consistent pattern of eliminating BIP 125 references. The constant removal requires verification that no remaining code depends on WALLET_INCREMENTAL_RELAY_FEE, and the compile-time assertion removal should be confirmed as intentional. Low complexity and mostly homogeneous changes minimize review overhead.

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 PR title "fix: remove mentioning of BIP-125" directly and clearly reflects the primary objective of the changeset. The changes across all modified files consistently focus on removing BIP-125 references from documentation strings, comments, and code assertions. The title is concise, specific enough for someone reviewing history to understand the main change, and uses standard commit prefixing ("fix:") appropriately for a maintenance/cleanup change. The title is neither vague nor misleading, as it accurately represents the main work performed.
Description Check ✅ Passed The PR description is clearly related to the changeset and provides meaningful context for the changes. It explains the motivation (BIP-125 is not implemented and will not be supported), states what was done (removing backported leftovers), and includes appropriate metadata such as testing status and breaking changes. The description is not vague or generic; it specifically addresses why BIP-125 references are being removed and frames this as correcting a mistake in backported code. The description directly aligns with the file modifications removing BIP-125 mentions and the static assertion.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/functional/test_framework/messages.py (1)

43-43: Consider simplifying the comment to remove BIP 125 functional description.

The comment mentions "BIP 125 opt-in and BIP 68-opt-out" but then states "not used in DASH", which is somewhat contradictory. Since BIP 125 is not implemented in Dash, consider simplifying to just state the constant's purpose or non-use without describing what it would do if it were used.

Suggested revision:

-BIP125_SEQUENCE_NUMBER = 0xfffffffd  # Sequence number that is BIP 125 opt-in and BIP 68-opt-out -- not used in DASH
+BIP125_SEQUENCE_NUMBER = 0xfffffffd  # Sequence number for BIP 125/68 -- not used in DASH
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6e8f5 and 9f617e3.

📒 Files selected for processing (5)
  • src/init.cpp (2 hunks)
  • src/policy/policy.h (1 hunks)
  • src/wallet/test/wallet_tests.cpp (0 hunks)
  • src/wallet/wallet.h (0 hunks)
  • test/functional/test_framework/messages.py (1 hunks)
💤 Files with no reviewable changes (2)
  • src/wallet/wallet.h
  • src/wallet/test/wallet_tests.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/policy/policy.h
  • src/init.cpp
test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Functional tests should be written in Python and placed in test/functional/

Files:

  • test/functional/test_framework/messages.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
⏰ 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: win64-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (3)
src/policy/policy.h (1)

36-37: LGTM! Clean removal of BIP 125 reference.

The comment has been appropriately updated to describe the constant's actual purpose in Dash (mempool limiting) without referencing the unimplemented BIP 125 functionality. The change is documentation-only with no functional impact.

src/init.cpp (2)

750-750: LGTM! Clean removal of BIP 125 from user-facing documentation.

The -incrementalrelayfee argument description has been appropriately updated to remove the BIP 125 reference. The new description clearly states its purpose for mempool limiting, which is the actual functionality in Dash.


1258-1258: LGTM! Accurate description of incremental relay fee behavior.

The inline comment has been updated to accurately describe how the incremental relay fee works in the context of mempool limiting, without referencing the unimplemented BIP 125. This is a documentation-only change with no functional impact.

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

utACK 9f617e3

@PastaPastaPasta PastaPastaPasta merged commit 7c767a1 into dashpay:develop Oct 21, 2025
55 of 60 checks passed
@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Oct 21, 2025
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.

3 participants