feat: add -dustprotectionthreshold CLI option and unify GUI settings#7265
feat: add -dustprotectionthreshold CLI option and unify GUI settings#7265UdjinM6 wants to merge 4 commits intodashpay:developfrom
-dustprotectionthreshold CLI option and unify GUI settings#7265Conversation
Add a command-line option (-dustprotectionthreshold=<n>) to automatically lock UTXOs from external transactions at or below <n> duffs, protecting against dust attacks. Move dust detection logic into CWallet (IsDustProtectionTarget, CheckAndLockDustOutputs, LockExistingDustOutputs) so it works for both CLI (dashd) and GUI (dash-qt) without duplication. Convert GUI dust protection from standalone QSettings to the CLI-shared settings framework (Prune pattern), so CLI flags take precedence and the Options dialog shows an override label. The -prev suffix remembers the user's threshold when disabling, restoring it on re-enable. Includes QSettings migration for existing users upgrading from the old GUI-only implementation, with clamping to the 1,000,000 duff maximum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover external dust locking, self-send exclusion, above-threshold exclusion, disabled threshold, restart persistence, multi-wallet dust protection, and invalid argument rejection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
✅ Review complete (commit 30b132e) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd0e8879e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a dust-protection feature that auto-locks wallet UTXOs at-or-below a configurable threshold for incoming external transactions. Introduces CLI option Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI / Node Init"
participant GUI as "Options Dialog"
participant Options as "OptionsModel"
participant WalletModel as "WalletModel"
participant Interfaces as "interfaces::Wallet"
participant WalletCore as "CWallet (core)"
User->>CLI: start node with -dustprotectionthreshold=500
CLI->>WalletCore: parse & validate threshold
WalletCore->>WalletCore: set m_dust_protection_threshold
WalletCore->>WalletCore: LockExistingDustOutputs() on startup
Note over WalletCore: Incoming external tx (value ≤ threshold)
WalletCore->>WalletCore: IsDustProtectionTarget() -> true
WalletCore->>WalletCore: CheckAndLockDustOutputs() -> lock UTXO(s)
User->>GUI: open Options Dialog
GUI->>Options: load settings
Options->>Options: detect -dustprotectionthreshold override
alt CLI override present
Options->>GUI: disable dust controls
else
Options->>GUI: enable dust controls
end
User->>GUI: change threshold
GUI->>Options: setOption(DustProtectionThreshold)
Options->>WalletModel: notify threshold change
WalletModel->>Interfaces: setDustProtectionThreshold(threshold)
Interfaces->>WalletCore: set m_dust_protection_threshold
WalletModel->>Interfaces: lockExistingDustOutputs()
Interfaces->>WalletCore: LockExistingDustOutputs()
WalletCore->>WalletCore: re-classify & lock matching UTXO(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/optionsmodel.cpp`:
- Around line 381-385: Init() currently only validates DustProtection but not
the remembered DustProtectionThreshold, so a malformed saved value (used by
getOption(DustProtectionThreshold) via dustprotectionthreshold-prev fallback)
can bypass startup checks; update Init() in optionsmodel.cpp to also load and
validate the remembered threshold by calling getOption(DustProtectionThreshold)
(or explicitly reading the stored dustprotectionthreshold-prev key via
SettingName(DustProtectionThreshold)), parse/validate it the same way as the
live value, and if invalid reset it to a safe default and persist that corrected
value (and/or clear the prev entry) so subsequent queries cannot throw; ensure
you reference the DustProtectionThreshold symbol, the Init() function, and
SettingName(...) when locating the code to change.
In `@src/wallet/wallet.cpp`:
- Around line 2818-2819: The current check uses IsMine(txout) == ISMINE_NO which
still allows watch-only outputs; change the gating to require spendability
instead. Replace the IsMine(txout) check so that only outputs where
IsMine(txout) == ISMINE_SPENDABLE are processed (return false otherwise),
ensuring only spendable dust outputs (txout) are auto-locked; reference the
IsMine function and the ISMINE_SPENDABLE constant when making this change.
- Around line 3381-3382: The format string in the strprintf call for the dust
protection threshold uses %d but MAX_DUST_PROTECTION_THRESHOLD is a
CAmount/int64_t; update the format to a 64-bit-safe specifier (e.g. use "%"
PRId64 from <inttypes.h>) and ensure the argument is cast to int64_t
(referencing walletInstance->m_dust_protection_threshold and
MAX_DUST_PROTECTION_THRESHOLD in the strprintf call) so the printed maximum
value is correctly formatted across platforms.
In `@test/functional/wallet_dust_protection.py`:
- Around line 158-173: Instead of using a count from node3.listunspent() to
infer dust-only UTXOs, capture the exact dust outpoints before restarting: call
node3.listunspent() and filter by the dust criterion (e.g., amounts <
dustprotectionthreshold/relay logic) to build a set of txid:vout strings, then
after restart compare node3.listlockunspent() against that expected set (using
node3.listlockunspent() and matching txid/vout) to assert the same outpoints are
locked; update the test around restart_node(3, ["-dustrelayfee=0",
"-dustprotectionthreshold=10000"]) and restart_node(3, ["-dustrelayfee=0"]) to
use this explicit expected_outpoints set rather than num_dust.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 521b4d83-2ccb-4197-a8f4-60f34dedbd0a
📒 Files selected for processing (12)
src/interfaces/wallet.hsrc/qt/optionsdialog.cppsrc/qt/optionsmodel.cppsrc/qt/optionsmodel.hsrc/qt/walletmodel.cppsrc/qt/walletmodel.hsrc/wallet/init.cppsrc/wallet/interfaces.cppsrc/wallet/wallet.cppsrc/wallet/wallet.htest/functional/test_runner.pytest/functional/wallet_dust_protection.py
💤 Files with no reviewable changes (1)
- src/qt/walletmodel.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/functional/wallet_dust_protection.py`:
- Line 173: Rename the ambiguous loop variable 'l' used in the set
comprehensions to a clear name (e.g., 'entry' or 'locked_item') to satisfy lint
E741; specifically update the comprehension that builds locked_outpoints =
{(l['txid'], l['vout']) for l in locked} and the similar comprehension at the
later occurrence (line ~181) so they use the new identifier and adjust any
internal indexing (['txid'], ['vout']) to match the new name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab819c2a-2033-4bb6-a49b-866d6c745c5a
📒 Files selected for processing (1)
test/functional/wallet_dust_protection.py
Compare the exact set of (txid, vout) pairs instead of just a count, making the assertion more precise and resilient to test-order changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
feat/dust-protection-cli
branch
from
8aa5b12 to
03f4ed0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/functional/wallet_dust_protection.py (1)
35-35: Consider centralizing threshold literals to reduce drift.
10000(and max1000000) appears in several places. A class-level constant (or two) would make future threshold-boundary updates safer.Also applies to: 161-161, 169-169, 224-224, 248-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/wallet_dust_protection.py` at line 35, Introduce module- or test-class-level constants (e.g., DUST_PROTECTION_THRESHOLD = 10000 and DUST_PROTECTION_THRESHOLD_MAX = 1000000) in test/functional/wallet_dust_protection.py and replace all hardcoded occurrences of 10000 and 1000000 (including the array entry ["-dustrelayfee=0", "-dustprotectionthreshold=10000"] and the other noted locations) with those constants; update any string arguments or comparisons to format or reference the constants so threshold changes are centralized and avoid literal drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/functional/wallet_dust_protection.py`:
- Line 35: Introduce module- or test-class-level constants (e.g.,
DUST_PROTECTION_THRESHOLD = 10000 and DUST_PROTECTION_THRESHOLD_MAX = 1000000)
in test/functional/wallet_dust_protection.py and replace all hardcoded
occurrences of 10000 and 1000000 (including the array entry ["-dustrelayfee=0",
"-dustprotectionthreshold=10000"] and the other noted locations) with those
constants; update any string arguments or comparisons to format or reference the
constants so threshold changes are centralized and avoid literal drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 53b4330b-56e7-4e0b-b139-9d7520604476
📒 Files selected for processing (1)
test/functional/wallet_dust_protection.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified 7 findings from claude-general against source code. 1 blocking downgraded to suggestion (code duplication, not a bug), 1 finding invalidated (settings bypass doesn't exist), 1 dropped as duplicate. 5 findings confirmed with adjusted severities.
Reviewed commit: 03f4ed0
🟡 4 suggestion(s) | 💬 1 nitpick(s)
4 additional findings
🟡 suggestion: Duplicate InputIsMine: static copy in wallet.cpp duplicates public function in receive.cpp
src/wallet/wallet.cpp (line 1)
An identical static InputIsMine was added at src/wallet/wallet.cpp:59 that duplicates the public version at src/wallet/receive.cpp:13. Both have the exact same implementation (AssertLockHeld, GetWalletTx lookup, IsMine check). The static copy is used only at wallet.cpp:2823 in the dust protection check. Since wallet.cpp does not include receive.h, including it and removing the static copy would eliminate the duplication.
🟡 suggestion: Same constant name DEFAULT_DUST_PROTECTION_THRESHOLD used for two different values (0 vs 10000)
src/qt/optionsmodel.h (line 1)
src/wallet/wallet.h:115 defines DEFAULT_DUST_PROTECTION_THRESHOLD = 0 (CLI: disabled by default). src/qt/optionsmodel.h:28 defines DEFAULT_DUST_PROTECTION_THRESHOLD = 10000 (GUI: enabled with sensible default). Different scopes prevent ODR issues, and the design is intentional (CLI opt-in vs GUI protected-by-default). However, reusing the same identifier for different values is confusing. Consider renaming the GUI constant to DEFAULT_GUI_DUST_PROTECTION_THRESHOLD to match the existing MAX_GUI_DUST_PROTECTION_THRESHOLD pattern at optionsmodel.h:30.
🟡 suggestion: dustProtectionChanged signal emitted twice when toggling DustProtection off
src/qt/optionsmodel.cpp (line 1)
At src/qt/optionsmodel.cpp:1043-1053: when toggling DustProtection OFF, line 1045 recursively calls setOption(DustProtection, true, "-prev") to save the previous state. If the -prev value changed (typical after toggling ON which clears it at line 1051), the recursive call emits dustProtectionChanged() at line 1052, then the outer call emits it again at line 1052. Same double-emission pattern exists for DustProtectionThreshold (lines 1055-1063) when protection is disabled. Connected slots will be invoked twice per state change.
🟡 suggestion: setDustProtectionThreshold in wallet interface has no range validation
src/wallet/interfaces.cpp (line 1)
src/wallet/interfaces.cpp:347-351 directly assigns the threshold to m_wallet->m_dust_protection_threshold with no bounds checking. The CLI path (wallet.cpp:3376-3384) validates both minimum and maximum. The GUI optionsmodel (line 1060) enforces minimum via std::max<int64_t>(..., 1) but has no max check — it relies on the UI spinbox cap. A corrupted settings file or programmatic caller could set a value exceeding MAX_DUST_PROTECTION_THRESHOLD (1,000,000).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] line 1: Duplicate InputIsMine: static copy in wallet.cpp duplicates public function in receive.cpp
An identical static `InputIsMine` was added at `src/wallet/wallet.cpp:59` that duplicates the public version at `src/wallet/receive.cpp:13`. Both have the exact same implementation (AssertLockHeld, GetWalletTx lookup, IsMine check). The static copy is used only at wallet.cpp:2823 in the dust protection check. Since wallet.cpp does not include `receive.h`, including it and removing the static copy would eliminate the duplication.
In `src/qt/optionsmodel.h`:
- [SUGGESTION] line 1: Same constant name DEFAULT_DUST_PROTECTION_THRESHOLD used for two different values (0 vs 10000)
`src/wallet/wallet.h:115` defines `DEFAULT_DUST_PROTECTION_THRESHOLD = 0` (CLI: disabled by default). `src/qt/optionsmodel.h:28` defines `DEFAULT_DUST_PROTECTION_THRESHOLD = 10000` (GUI: enabled with sensible default). Different scopes prevent ODR issues, and the design is intentional (CLI opt-in vs GUI protected-by-default). However, reusing the same identifier for different values is confusing. Consider renaming the GUI constant to `DEFAULT_GUI_DUST_PROTECTION_THRESHOLD` to match the existing `MAX_GUI_DUST_PROTECTION_THRESHOLD` pattern at optionsmodel.h:30.
In `src/qt/optionsmodel.cpp`:
- [SUGGESTION] line 1: dustProtectionChanged signal emitted twice when toggling DustProtection off
At `src/qt/optionsmodel.cpp:1043-1053`: when toggling DustProtection OFF, line 1045 recursively calls `setOption(DustProtection, true, "-prev")` to save the previous state. If the `-prev` value changed (typical after toggling ON which clears it at line 1051), the recursive call emits `dustProtectionChanged()` at line 1052, then the outer call emits it again at line 1052. Same double-emission pattern exists for DustProtectionThreshold (lines 1055-1063) when protection is disabled. Connected slots will be invoked twice per state change.
In `src/wallet/interfaces.cpp`:
- [SUGGESTION] line 1: setDustProtectionThreshold in wallet interface has no range validation
`src/wallet/interfaces.cpp:347-351` directly assigns the threshold to `m_wallet->m_dust_protection_threshold` with no bounds checking. The CLI path (wallet.cpp:3376-3384) validates both minimum and maximum. The GUI optionsmodel (line 1060) enforces minimum via `std::max<int64_t>(..., 1)` but has no max check — it relies on the UI spinbox cap. A corrupted settings file or programmatic caller could set a value exceeding MAX_DUST_PROTECTION_THRESHOLD (1,000,000).
- Rename GUI constant to DEFAULT_GUI_DUST_PROTECTION_THRESHOLD to avoid confusion with the CLI default (0) in wallet.h - Add range validation (std::clamp) in wallet interface setter to guard against corrupted settings or programmatic callers - Fix copyright year to 2026 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Duplicate InputIsMine: intentional, avoids circular dependency |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30b132ecd6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean fix commit addressing 3 of 4 prior review suggestions. The dual-default confusion is resolved by renaming the GUI constant to DEFAULT_GUI_DUST_PROTECTION_THRESHOLD. Range validation added via std::clamp. Copyright year fixed. The duplicate InputIsMine finding from the prior review is not addressed — likely intentional (the static helper in wallet.cpp avoids a header include dependency).
Reviewed commit: 30b132e
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Validated PR #7265 at HEAD30b132ecd62afc5d1eacf8b3a8b9df0ca2104e44 against the candidate reviews and CodeRabbit comments. I did not find any concrete, user-impacting issues still present on this head. The remaining live CodeRabbit concern about validating the remembered dust threshold does not reproduce: getOption(DustProtectionThreshold) already routes malformed or non-positive persisted values through SettingToInt(...) and safely falls back to the remembered/default GUI threshold without throwing.
Reviewed commit: 30b132e
3 participants
Issue being fixed or feature implemented
Add a command-line option
-dustprotectionthreshold=<n>to automatically lock UTXOs from external transactions at or below<n>duffs, protecting against dust attacks.Also add a functional test which covers external dust locking, self-send exclusion, above-threshold exclusion, disabled threshold, restart persistence, multi-wallet dust protection, and invalid argument rejection.
What was done?
Move dust detection logic into
CWallet(IsDustProtectionTarget,CheckAndLockDustOutputs,LockExistingDustOutputs) so it works for both CLI (dashd) and GUI (dash-qt) without duplication.Convert GUI dust protection from standalone
QSettingsto the CLI-shared settings framework (Prunepattern), so CLI flags take precedence and the Options dialog shows an override label. The-prevsuffix remembers the user's threshold when disabling, restoring it on re-enable. IncludesQSettingsmigration for existing users upgrading from the old GUI-only implementation, with clamping to the 1,000,000 duff maximum.How Has This Been Tested?
Run tests, run qt and verify it behaves as expected
Breaking Changes
Checklist: