Skip to content

Commit 7964c37

Browse files
Merge #7794: fix(wallet): floor the denomination gap threshold so rebalancing cannot oscillate
076b093 fix(coinjoin): floor the denomination gap threshold so rebalancing cannot oscillate (pasta) Pull request description: ## Issue being fixed or feature implemented CoinJoin promotion and demotion use a gap threshold so that converting coins between two adjacent denominations does not flip back and forth. Since the threshold was scaled with the goal (`GetGapThreshold(nGoal) = max(nGoal / 5, 1)`), any `-coinjoindenomsgoal` below 25 gets a gap of 4 or less. That is too small to absorb the 10:1 asymmetry of a conversion: a promotion moves the smaller denomination's deficit by up to +10 and the larger one's by -1, so with a gap of `g` an immediate reversal is possible whenever `2g + 1 <= PROMOTION_RATIO`, i.e. `g <= 4`. Example at the minimum goal of 10 (gap 2), all coins fully mixed: - 10 smaller / 7 larger: deficits 0 vs 3, `3 > 0 + 2`, so the wallet promotes. - Result 0 smaller / 8 larger: deficits 10 vs 2, `10 > 2 + 2`, so the wallet demotes back to 10 / 7. - Once the 10 demoted outputs are mixed again, the promotion fires again, and so on. Only wallets that set the goal below 25 are affected; the default of 50 (gap 10) is not. ### Why this is a real problem The decision functions fire on a one-sided deficit comparison with no other hysteresis: - [`GetGapThreshold`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L131) - [`ShouldPromoteDenoms`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L224-L233) and [`ShouldDemoteDenoms`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/common.h#L242-L251) - The client passes wallet totals and `GetDenomsGoal()` straight through ([client.cpp](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/client.cpp#L2494-L2520)), and the goal can be set as low as [`MIN_COINJOIN_DENOMS_GOAL = 10`](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/options.h#L19) ([clamp](https://github.com/dashpay/dash/blob/c14104b172a2/src/coinjoin/options.cpp#L72)). The new regression test `rebalance_never_immediately_reverses` checks every goal from 10 to 200 and every pair of counts from 0 to twice the goal: after any promotion the matching demotion must not fire, and the reverse. Run against unfixed develop (c14104b) with only the test added, it fails 710 times, at exactly goals 10 through 24, and the 10/7 example above is among the failures: <details><summary>Unfixed run (abridged)</summary> ``` $ src/test/test_dash --color_output=false --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses Running 1 test case... test/coinjoin_inouts_tests.cpp:1386: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: demoting 5/0 is undone by a promotion test/coinjoin_inouts_tests.cpp:1386: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: demoting 6/0 is undone by a promotion ... test/coinjoin_inouts_tests.cpp:1383: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 10: promoting 10/7 is undone by a demotion ... test/coinjoin_inouts_tests.cpp:1383: error: in "coinjoin_inouts_tests/rebalance_never_immediately_reverses": goal 24: promoting 25/19 is undone by a demotion *** 710 failures are detected in the test module "Dash Core Test Suite" exit=201 ``` Goals with failures: 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24. </details> ### Why it matters A wallet with a low goal can keep promoting and demoting the same value indefinitely. Each round pays mixing fees and puts another 10-input promotion and 1:10 demotion on-chain, which works against the privacy that rebalancing is meant to protect. The cycle is slow, because demoted outputs have to be remixed before they can be promoted again, and it only affects the wallet owner: no funds are lost and nothing outside the wallet changes. ## What was done? - Added `MIN_GAP_THRESHOLD = (PROMOTION_RATIO + 1) / 2` (5) in `src/coinjoin/common.h`, with the `2 * gap + 1 > PROMOTION_RATIO` derivation in its comment. - `GetGapThreshold` now returns `std::max(nGoal / GAP_DIVISOR, MIN_GAP_THRESHOLD)`. - Added the `rebalance_never_immediately_reverses` test to `src/test/coinjoin_inouts_tests.cpp`, next to the existing gap-threshold tests. ### Why this is the correct minimal fix A promotion needs `Dl > Ds + g`. Afterwards `Dl` drops by 1 and `Ds` rises by at most `PROMOTION_RATIO`. Capping deficits at the goal only shrinks these changes. A demotion right after would need `Ds' > Dl' + g`, so `Ds' >= Ds + 2g + 1`, which is impossible once `2g + 1 > PROMOTION_RATIO`. Demote-then-promote is symmetric. Five is the smallest gap that satisfies this, and it only changes goals 10 to 24. Goals of 25 and above, including the default of 50, keep their current threshold. The floor stays below the goal, so conversion is still reachable at the minimum goal: at goal 10 the extreme 10-vs-0 imbalance (gap 10 > 5) still promotes and demotes. The existing `gap_threshold_reachable_at_every_goal` test, which guards against the threshold disabling the feature, still passes unchanged. Adding a cooldown or remembering past conversions would also stop the loop, but it would need new wallet state when the threshold alone is the root cause. The deficit logic and the fully-mixed gates are unchanged. ## How Has This Been Tested? Built `src/test/test_dash` on macOS arm64 (`--enable-debug --enable-werror`). - Before the fix (develop c14104b + new test only): `src/test/test_dash --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses` fails with 710 failures, all at goals 10 to 24 (see above). - After the fix: - `src/test/test_dash --run_test=coinjoin_inouts_tests/rebalance_never_immediately_reverses`: no errors. - `src/test/test_dash --run_test=coinjoin_inouts_tests`: 52 test cases, no errors. This includes `gap_threshold_reachable_at_every_goal` and the other promote/demote decision tests. - `coinjoin_tests` (14 cases), `coinjoin_queue_tests` (11), `coinjoin_dstxmanager_tests` (5): no errors. - `contrib/devtools/clang-format-diff.py` on the diff reports no changes. `test/lint/lint-whitespace.py`, `lint-includes.py`, `lint-format-strings.py`, `lint-tests.py` and `lint-circular-dependencies.py` pass. ## Breaking Changes None. With `-coinjoindenomsgoal` between 10 and 24, the deficit gap a conversion needs rises from 2–4 to 5. No other behavior changes. This fixes a regression introduced in the v24.0.0 release candidates and should also go to the v24.0.x branch. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 93b9fb1489afff38b62d9193b8505e322bb91deda426b87dfd8a61a25a587466ee4006af86ee8276a9aa6d79ae031a3f64f1aef5cc5415153270b14d1a805211
2 parents 3789ec0 + 076b093 commit 7964c37

2 files changed

Lines changed: 33 additions & 2 deletions

File tree

‎src/coinjoin/common.h‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,15 +120,23 @@ constexpr CAmount GetMaxCollateralAmount() { return GetCollateralAmount() * 4; }
120120
// Promotion/demotion constants (post-V24 feature)
121121
constexpr int PROMOTION_RATIO = 10; // 10 smaller denomination coins = 1 larger denomination coin
122122
constexpr int GAP_DIVISOR = 5; // Deficit gap required to trigger promotion/demotion, as 1/N of the goal
123+
/**
124+
* Smallest gap that stops a conversion from making the opposite conversion worthwhile. A
125+
* conversion needs the receiving deficit to exceed the other by more than the gap, and it moves
126+
* the two deficits by at most PROMOTION_RATIO and 1, so reversing it right away would need
127+
* 2 * gap + 1 <= PROMOTION_RATIO. Any gap with 2 * gap + 1 > PROMOTION_RATIO rules that out.
128+
*/
129+
constexpr int MIN_GAP_THRESHOLD = (PROMOTION_RATIO + 1) / 2;
123130

124131
/**
125132
* How far behind the other denomination a denomination has to be before converting is worth it;
126133
* the gap is what keeps promotion and demotion from oscillating. It is a fraction of the goal
127134
* rather than a constant because the largest gap two denominations can show is the goal itself:
128135
* a constant of 10 was unsatisfiable at MIN_COINJOIN_DENOMS_GOAL (also 10) and silently
129-
* disabled the feature there. At the default goal of 50 this still yields 10.
136+
* disabled the feature there. At the default goal of 50 this still yields 10; below a goal of 25
137+
* it is floored at MIN_GAP_THRESHOLD.
130138
*/
131-
constexpr int GetGapThreshold(int nGoal) { return std::max(nGoal / GAP_DIVISOR, 1); }
139+
constexpr int GetGapThreshold(int nGoal) { return std::max(nGoal / GAP_DIVISOR, MIN_GAP_THRESHOLD); }
132140

133141
/**
134142
* Which side(s) of the session denomination a participant occupies. A standard entry mixes at

‎src/test/coinjoin_inouts_tests.cpp‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include <script/script.h>
2020
#include <streams.h>
2121
#include <test/util/setup_common.h>
22+
#include <tinyformat.h>
2223
#include <uint256.h>
2324
#include <util/check.h>
2425
#include <util/time.h>
@@ -1368,6 +1369,28 @@ BOOST_AUTO_TEST_CASE(gap_threshold_reachable_at_every_goal)
13681369
BOOST_CHECK_EQUAL(CoinJoin::GetGapThreshold(DEFAULT_COINJOIN_DENOMS_GOAL), 10);
13691370
}
13701371

1372+
BOOST_AUTO_TEST_CASE(rebalance_never_immediately_reverses)
1373+
{
1374+
// A conversion must never leave the wallet in a state where the opposite conversion fires,
1375+
// otherwise the wallet keeps paying for 10:1 and 1:10 conversions that undo each other
1376+
// (with a gap of 2, 10 smaller and 7 larger coins at goal 10 would promote to 0/8 and then
1377+
// demote straight back).
1378+
// Counts above the goal are included so the deficit cap is covered as well.
1379+
const int ratio = CoinJoin::PROMOTION_RATIO;
1380+
for (int goal = MIN_COINJOIN_DENOMS_GOAL; goal <= 200; ++goal) {
1381+
for (int smaller = 0; smaller <= 2 * goal; ++smaller) {
1382+
for (int larger = 0; larger <= 2 * goal; ++larger) {
1383+
if (TestShouldPromote(smaller, larger, goal) && TestShouldDemote(larger + 1, smaller - ratio, goal)) {
1384+
BOOST_ERROR(strprintf("goal %d: promoting %d/%d is undone by a demotion", goal, smaller, larger));
1385+
}
1386+
if (TestShouldDemote(larger, smaller, goal) && TestShouldPromote(smaller + ratio, larger - 1, goal)) {
1387+
BOOST_ERROR(strprintf("goal %d: demoting %d/%d is undone by a promotion", goal, larger, smaller));
1388+
}
1389+
}
1390+
}
1391+
}
1392+
}
1393+
13711394
BOOST_AUTO_TEST_CASE(should_promote_small_gap_false)
13721395
{
13731396
const int goal = 100;

0 commit comments

Comments
 (0)