From 76093fe82dc2fcdaaf13fd7205628c2dd294433d Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 16 Jul 2026 20:42:59 -0500 Subject: [PATCH] fix(chat): address adversarial review of the send-path change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four confirmed findings from the review of ef247c9: 1. Persistence regression — the deferred-persist echo (in-memory Sending, written only when the async callback resolved) meant a message broadcast on-chain but whose callback hadn't fired yet was LOST from history if the app quit/crashed in that window. Persist the echo immediately as Sending and UPSERT the final status on resolve (new ChatDatabase::upsert with ON CONFLICT DO UPDATE, since append is INSERT-OR-IGNORE). A stray persisted Sending still loads as Sent. 2. Fee ceiling — dragonxd REJECTS a 0-value tx whose fee exceeds the default miners fee (0.0001), and max(getDefaultFee(), 0.0001) can only raise it, so a default_fee > 0.0001 broke every chat send. Pin chat to exactly kChatMinFeeDrgx, dropping getDefaultFee() from this path (chat always moves 0 value). 3. Lifetime — the resolve callback had no generation guard, so a wallet lock (which doesn't disconnect) between submit and callback could resolve against a cleared store. Capture chat_session_generation_ and bail on mismatch (both the full-node callback and the lite optimistic resolve), matching the identity-fetch pattern. 4. Retry misdirect — Retry on a failed CONTACT REQUEST called sendChatMessage, which (no peer key yet) just showed "waiting for reply". Route it to sendContactRequestForCid() (refactored out of startChatConversation) so it re-sends the request into the SAME conversation. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/app.h | 3 +++ src/app_network.cpp | 26 ++++++++++++++++++++++---- src/chat/chat_database.cpp | 29 +++++++++++++++++++++++++++++ src/chat/chat_database.h | 5 +++++ src/chat/chat_service.cpp | 11 +++++++---- src/chat/chat_service.h | 6 +++--- src/ui/windows/chat_tab.cpp | 9 ++++++++- 7 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/app.h b/src/app.h index 3313425..ccdb255 100644 --- a/src/app.h +++ b/src/app.h @@ -179,6 +179,9 @@ public: // message (to a conversation whose peer key we know) / a new-conversation contact request. void sendChatMessage(const std::string& conversationId, const std::string& text); void startChatConversation(const std::string& peerZaddr, const std::string& text); + // Send a contact request into an existing conversation (used to retry a failed request in place). + void sendContactRequestForCid(const std::string& cid, const std::string& peerZaddr, + const std::string& text); // Debug/sweep convenience: give the Chat tab a demo identity + a few sample conversations // (in-memory only, not persisted) so the screenshot sweep captures the populated UI. No-op when // the chat feature is off. diff --git a/src/app_network.cpp b/src/app_network.cpp index 2ec5144..54f1567 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -2934,11 +2934,16 @@ bool App::broadcastChatMemos(const chat::OutgoingChatMemos& memos, const std::st { if (memos.recipientZaddr.empty()) return false; + // Guard the async resolve against a wallet lock/switch between submit and callback: if the chat + // session was reset in between, this echo belongs to a stale session — don't touch the new store. + const auto sessionGen = chat_session_generation_; + if (lite_wallet_) { const bool ok = broadcastChatMemosLite(memos); // The lite backend has no per-send completion callback here; resolve optimistically on a // successful queue (its own broadcast log surfaces a later failure). - if (ok) chat_service_.resolveOutgoing(echoLocalId, chat::ChatDelivery::Sent); + if (ok && sessionGen == chat_session_generation_) + chat_service_.resolveOutgoing(echoLocalId, chat::ChatDelivery::Sent); return ok; } @@ -2947,7 +2952,10 @@ bool App::broadcastChatMemos(const chat::OutgoingChatMemos& memos, const std::st return false; } - const double fee = std::max(settings_ ? settings_->getDefaultFee() : kChatMinFeeDrgx, kChatMinFeeDrgx); + // Chat moves 0 value, and dragonxd REJECTS a 0-value tx whose fee exceeds the default miners fee + // (0.0001) — so pin chat to exactly kChatMinFeeDrgx, independent of the user's default-fee setting + // (which may be higher). It's also well above the node's min relay fee. + const double fee = kChatMinFeeDrgx; // Pay from a z-address that can actually cover the fee. The memo still advertises the identity // reply address, so the peer replies to the right place regardless of which note paid. @@ -2973,7 +2981,8 @@ bool App::broadcastChatMemos(const chat::OutgoingChatMemos& memos, const std::st // The callback flips the echo to Sent/Failed once the async op resolves — real delivery status. submitZSendMany(from, memos.recipientZaddr, 0.0, fee, /*memo*/"", recipients, "HushChat / broadcast", /*markFeeGapRetry*/ true, - [this, echoLocalId](bool ok, const std::string& /*result*/) { + [this, echoLocalId, sessionGen](bool ok, const std::string& /*result*/) { + if (sessionGen != chat_session_generation_) return; // wallet locked/switched — stale chat_service_.resolveOutgoing(echoLocalId, ok ? chat::ChatDelivery::Sent : chat::ChatDelivery::Failed); }); @@ -3058,13 +3067,22 @@ void App::startChatConversation(const std::string& peerZaddr, const std::string& { if (!chat::hushChatFeatureEnabledAtBuild() || !chat_service_.hasIdentity()) return; if (peerZaddr.empty() || text.empty()) return; + sendContactRequestForCid(generateChatLocalId("", 16), peerZaddr, text); // new opaque cid +} + +// Compose + broadcast a contact request into a SPECIFIC conversation. startChatConversation() mints a +// fresh cid; a Retry of a failed request reuses its existing cid so it stays in the same thread. +void App::sendContactRequestForCid(const std::string& cid, const std::string& peerZaddr, + const std::string& text) +{ + if (!chat::hushChatFeatureEnabledAtBuild() || !chat_service_.hasIdentity()) return; + if (cid.empty() || peerZaddr.empty() || text.empty()) return; const std::string myReply = chatReplyZaddr(); if (myReply.empty()) { ui::Notifications::instance().error(TR("chat_toast_no_zaddr")); return; } - const std::string cid = generateChatLocalId("", 16); // opaque per-conversation id chat::OutgoingChatMemos memos; if (chat_service_.composeContactRequest(myReply, peerZaddr, cid, text, memos) != chat::ChatComposeStatus::Ok) { diff --git a/src/chat/chat_database.cpp b/src/chat/chat_database.cpp index 243247c..3aac67e 100644 --- a/src/chat/chat_database.cpp +++ b/src/chat/chat_database.cpp @@ -129,6 +129,35 @@ bool ChatDatabase::append(const ChatMessage& message) return sqlite3_changes(db_) > 0; } +bool ChatDatabase::upsert(const ChatMessage& message) +{ + if (!key_ready_ || !ensureOpen()) return false; + + std::vector nonce; + std::vector cipher; + std::string plain = serialize(message); + const bool encrypted = encrypt(plain, nonce, cipher); + if (!plain.empty()) sodium_memzero(&plain[0], plain.size()); + if (!encrypted) return false; + + const std::string dedup = dedupHash(message.txid, message.payload_position); + + sqlite3_stmt* stmt = nullptr; + if (sqlite3_prepare_v2(db_, + "INSERT INTO chat_messages (wallet_tag, dedup_hash, nonce, payload) VALUES (?, ?, ?, ?) " + "ON CONFLICT(wallet_tag, dedup_hash) DO UPDATE SET nonce=excluded.nonce, payload=excluded.payload", + -1, &stmt, nullptr) != SQLITE_OK) { + return false; + } + sqlite3_bind_text(stmt, 1, wallet_tag_.c_str(), -1, SQLITE_TRANSIENT); + sqlite3_bind_text(stmt, 2, dedup.c_str(), -1, SQLITE_TRANSIENT); + sqlite3_bind_blob(stmt, 3, nonce.data(), static_cast(nonce.size()), SQLITE_TRANSIENT); + sqlite3_bind_blob(stmt, 4, cipher.data(), static_cast(cipher.size()), SQLITE_TRANSIENT); + const bool done = sqlite3_step(stmt) == SQLITE_DONE; + sqlite3_finalize(stmt); + return done; +} + std::vector ChatDatabase::load() { std::vector out; diff --git a/src/chat/chat_database.h b/src/chat/chat_database.h index fbb7198..1156a60 100644 --- a/src/chat/chat_database.h +++ b/src/chat/chat_database.h @@ -43,6 +43,11 @@ public: // Returns true if newly inserted; false on duplicate or while locked. bool append(const ChatMessage& message); + // Persist-or-overwrite one message by its (txid+position) dedup key. Unlike append(), this + // updates an existing row's payload — used for outgoing echoes whose delivery status changes + // (Sending → Sent/Failed). Returns true on success; false while locked / on error. + bool upsert(const ChatMessage& message); + // Decrypt and return every stored message for the unlocked wallet, in insertion order. Empty // while locked or if none. Rows that fail to decrypt/parse are skipped. std::vector load(); diff --git a/src/chat/chat_service.cpp b/src/chat/chat_service.cpp index 8e3bf4c..359b14a 100644 --- a/src/chat/chat_service.cpp +++ b/src/chat/chat_service.cpp @@ -110,14 +110,17 @@ bool ChatService::recordOutgoing(const ChatMessage& message) { } bool ChatService::recordOutgoingPending(const ChatMessage& message) { - // In-memory only — deliberately NOT persisted yet, so "Sending" never survives to a restart; the - // row is written by resolveOutgoing() once the broadcast resolves to Sent/Failed. - return store_.append(message); + // Persist immediately (as Sending) so a send survives an app quit before the broadcast resolves; + // resolveOutgoing() then UPSERTS the row to the final status. A stray persisted Sending (crash mid- + // broadcast) loads as Sent (see ChatDatabase::deserialize). + const bool appended = store_.append(message); + if (appended && db_) db_->upsert(message); + return appended; } void ChatService::resolveOutgoing(const std::string& txid, ChatDelivery delivery) { const ChatMessage* updated = store_.updateDelivery(txid, delivery); - if (updated && db_) db_->append(*updated); // first persist, carrying the final status + if (updated && db_) db_->upsert(*updated); // overwrite the Sending row with the final status } } // namespace dragonx::chat diff --git a/src/chat/chat_service.h b/src/chat/chat_service.h index b51a438..af9d5d9 100644 --- a/src/chat/chat_service.h +++ b/src/chat/chat_service.h @@ -76,9 +76,9 @@ public: // only local record of what we sent.) bool recordOutgoing(const ChatMessage& message); - // Two-phase echo for delivery tracking: record in-memory only (shows immediately as Sending), - // then resolveOutgoing() flips the status once the broadcast completes AND persists it with the - // final status — so a restart never shows a stuck "Sending". + // Two-phase echo for delivery tracking: record + persist immediately as Sending (survives an app + // quit), then resolveOutgoing() upserts the final status once the broadcast completes. A stray + // persisted Sending (crash mid-broadcast) loads back as Sent. bool recordOutgoingPending(const ChatMessage& message); void resolveOutgoing(const std::string& txid, ChatDelivery delivery); diff --git a/src/ui/windows/chat_tab.cpp b/src/ui/windows/chat_tab.cpp index 978c723..aa0303b 100644 --- a/src/ui/windows/chat_tab.cpp +++ b/src/ui/windows/chat_tab.cpp @@ -513,7 +513,14 @@ void RenderChatTab(App* app) ImGui::TextUnformatted(TR("chat_send_failed")); ImGui::PopStyleColor(); ImGui::PopFont(); ImGui::SameLine(); - if (ImGui::SmallButton(TR("chat_retry"))) { app->sendChatMessage(sel->cid, m.body); s_scroll_to_cid = sel->cid; } + if (ImGui::SmallButton(TR("chat_retry"))) { + // A failed contact request must be re-sent as a request (it has no peer key + // yet) into the SAME conversation — not routed through sendChatMessage, + // which would just say "waiting for reply". + if (request) app->sendContactRequestForCid(sel->cid, m.peer_zaddr, m.body); + else app->sendChatMessage(sel->cid, m.body); + s_scroll_to_cid = sel->cid; + } } // In-flight send → subtle right-aligned "sending…"; the op callback resolves it. if (sending) {