From ba569b20e1862b6c1debec7a91362011fa3884d1 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Wed, 19 Aug 2026 16:09:31 -0300 Subject: [PATCH] Fix spurious seqno increment when a merge adopts an incoming config When _merge runs while dirty it builds the result as a MutableConfigMessage, whose multi-config constructor unconditionally increments the seqno when nothing needed merging. That is correct when the surviving config is our own (and is unwound by the existing nothing-to-do branch), but when the survivor is an *incoming* message -- reachable while dirty via an exact (seqno, hash) duplicate (another device pushed the identical change) or via lagged-diff containment of such a duplicate -- we adopted it at the inflated seqno. That burned a seqno no stored message occupies, so the peer's next ordinary change (correctly landing on the value we consumed) looked like a conflict and produced a pointless conflict-resolution push, and it eroded the config_lags() conflict window. Adopt the incoming message at its own seqno instead, exactly as a non-dirty merge would; the merged (conflict-resolution) path is untouched. --- src/config/base.cpp | 12 ++++++- tests/test_bugs.cpp | 83 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/config/base.cpp b/src/config/base.cpp index 440844d1f..8cc503d7d 100644 --- a/src/config/base.cpp +++ b/src/config/base.cpp @@ -609,9 +609,19 @@ std::unordered_set ConfigBase::_merge( // seqno increment. /* do nothing */ } else { - _config = std::move(new_conf); assert(((old_seqno == 0 && mine.empty()) || !superconf_is_mine) && *superconf < all_hashes.size()); + if (_state == ConfigState::Dirty) { + // Because we were dirty, new_conf was built as a MutableConfigMessage, which + // unconditionally increments the seqno past the adopted config's own value. But we + // are adopting an incoming, already-stored message as-is, so we must land on *its* + // seqno/hash (exactly as a non-dirty merge would); re-parse it to get an + // unincremented copy. + _config = std::make_unique( + all_confs[*superconf], _config->verifier, _config->signer, config_lags()); + } else { + _config = std::move(new_conf); + } set_state(ConfigState::Clean); _curr_hashes.clear(); auto& hashes = all_hashes[*superconf]; diff --git a/tests/test_bugs.cpp b/tests/test_bugs.cpp index e8b4a6cc2..088962547 100644 --- a/tests/test_bugs.cpp +++ b/tests/test_bugs.cpp @@ -188,7 +188,9 @@ TEST_CASE("Merge config matching local changse", "[config][merge_matching_dirty] "0511111111111111111111111111111111111111111111111111111111111111{:02}", i), fmt::format("barney{}", i)); auto [seqno_i, data_i, obs_i] = c1.push(); - REQUIRE(seqno_i == i); + // One behind i because the first merge above adopted c2's identical config at c2's own + // seqno rather than consuming a new one. + REQUIRE(seqno_i == i - 1); c1.confirm_pushed(seqno_i, {"fakehash" + std::to_string(i)}); CHECK_FALSE(c1.needs_push()); CHECK_FALSE(c1.is_dirty()); @@ -213,3 +215,82 @@ TEST_CASE("Merge config matching local changse", "[config][merge_matching_dirty] CHECK(c1.is_dirty()); CHECK_FALSE(c1.is_clean()); } + +// When we are dirty and merge an incoming config that makes the identical change, we adopt the +// incoming message as-is: same data, same seqno, standing behind its storage hash. There was a bug +// where the adoption consumed an extra seqno that no stored message occupies, so that the peer's +// *next* ordinary change (correctly landing on the value we had burned) looked like a conflict: +// merging it produced a pointless conflict-resolution push instead of a clean adoption. +TEST_CASE("Merge matching local changes adopts without consuming a seqno", "[config][merge]") { + const auto seed = "0123456789abcdef0123456789abcdef00000000000000000000000000000000"_hexbytes; + std::array ed_pk; + std::array ed_sk; + crypto_sign_ed25519_seed_keypair( + ed_pk.data(), ed_sk.data(), reinterpret_cast(seed.data())); + + session::config::Contacts c1{session::to_span(seed), std::nullopt}; + c1.set_name("050000000000000000000000000000000000000000000000000000000000000000", "alfonso"); + auto [seqno1, data1, obs1] = c1.push(); + REQUIRE(seqno1 == 1); + c1.confirm_pushed(seqno1, {"fakehash1"}); + + auto dump1 = c1.dump(); + session::config::Contacts c2{session::to_span(seed), dump1}; + + // Both devices make the same change; c2 gets its push in first. + c1.set_name("051111111111111111111111111111111111111111111111111111111111111111", "barney"); + c2.set_name("051111111111111111111111111111111111111111111111111111111111111111", "barney"); + + auto [seqno2, data2, obs2] = c2.push(); + REQUIRE(seqno2 == 2); + c2.confirm_pushed(seqno2, {"fakehash2"}); + + REQUIRE(c1.is_dirty()); + auto r = c1.merge(std::vector>>{ + {{"fakehash2"s, session::to_span(data2[0])}}}); + CHECK(r == std::unordered_set{{"fakehash2"s}}); + CHECK(c1.is_clean()); + CHECK_FALSE(c1.needs_push()); + CHECK(c1.curr_hashes() == std::unordered_set{{"fakehash2"s}}); + + // c2, still on seqno 2, makes an ordinary change of its own; c1 must accept it as a clean + // adoption at seqno 3, not as a conflict with a phantom seqno 3 of c1's. + c2.set_name("052222222222222222222222222222222222222222222222222222222222222222", "carl"); + auto [seqno3, data3, obs3] = c2.push(); + REQUIRE(seqno3 == 3); + c2.confirm_pushed(seqno3, {"fakehash3"}); + + r = c1.merge(std::vector>>{ + {{"fakehash3"s, session::to_span(data3[0])}}}); + CHECK(r == std::unordered_set{{"fakehash3"s}}); + CHECK(c1.is_clean()); + CHECK_FALSE(c1.needs_push()); + CHECK(c1.curr_hashes() == std::unordered_set{{"fakehash3"s}}); + CHECK(c1.get("052222222222222222222222222222222222222222222222222222222222222222") + .value() + .name == "carl"); + + // A new change by c1 must consume seqno 4, agreeing with c2's numbering, and obsolete exactly + // the messages both devices know about. + c1.set_name("053333333333333333333333333333333333333333333333333333333333333333", "dora"); + auto [seqno4, data4, obs4] = c1.push(); + CHECK(seqno4 == 4); + CHECK(as_set(obs4) == make_set("fakehash1"s, "fakehash2"s, "fakehash3"s)); + + // Same identical-change situation, but the peer had already pushed a further change on top by + // the time we merge: both messages arrive together and we adopt the newest at its own seqno. + session::config::Contacts c3{session::to_span(seed), dump1}; + c3.set_name("051111111111111111111111111111111111111111111111111111111111111111", "barney"); + REQUIRE(c3.is_dirty()); + r = c3.merge(std::vector>>{ + {{"fakehash2"s, session::to_span(data2[0])}, + {"fakehash3"s, session::to_span(data3[0])}}}); + CHECK(r == std::unordered_set{{"fakehash2"s, "fakehash3"s}}); + CHECK(c3.is_clean()); + CHECK_FALSE(c3.needs_push()); + CHECK(c3.curr_hashes() == std::unordered_set{{"fakehash3"s}}); + + c3.set_name("054444444444444444444444444444444444444444444444444444444444444444", "edgar"); + auto [seqno5, data5, obs5] = c3.push(); + CHECK(seqno5 == 4); +}