fix(chat): address adversarial review of the send-path change
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) <noreply@anthropic.com>
This commit is contained in:
@@ -179,6 +179,9 @@ public:
|
|||||||
// message (to a conversation whose peer key we know) / a new-conversation contact request.
|
// 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 sendChatMessage(const std::string& conversationId, const std::string& text);
|
||||||
void startChatConversation(const std::string& peerZaddr, 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
|
// 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
|
// (in-memory only, not persisted) so the screenshot sweep captures the populated UI. No-op when
|
||||||
// the chat feature is off.
|
// the chat feature is off.
|
||||||
|
|||||||
@@ -2934,11 +2934,16 @@ bool App::broadcastChatMemos(const chat::OutgoingChatMemos& memos, const std::st
|
|||||||
{
|
{
|
||||||
if (memos.recipientZaddr.empty()) return false;
|
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_) {
|
if (lite_wallet_) {
|
||||||
const bool ok = broadcastChatMemosLite(memos);
|
const bool ok = broadcastChatMemosLite(memos);
|
||||||
// The lite backend has no per-send completion callback here; resolve optimistically on a
|
// The lite backend has no per-send completion callback here; resolve optimistically on a
|
||||||
// successful queue (its own broadcast log surfaces a later failure).
|
// 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;
|
return ok;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2947,7 +2952,10 @@ bool App::broadcastChatMemos(const chat::OutgoingChatMemos& memos, const std::st
|
|||||||
return false;
|
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
|
// 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.
|
// 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.
|
// 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,
|
submitZSendMany(from, memos.recipientZaddr, 0.0, fee, /*memo*/"", recipients,
|
||||||
"HushChat / broadcast", /*markFeeGapRetry*/ true,
|
"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,
|
chat_service_.resolveOutgoing(echoLocalId,
|
||||||
ok ? chat::ChatDelivery::Sent : chat::ChatDelivery::Failed);
|
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 (!chat::hushChatFeatureEnabledAtBuild() || !chat_service_.hasIdentity()) return;
|
||||||
if (peerZaddr.empty() || text.empty()) 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();
|
const std::string myReply = chatReplyZaddr();
|
||||||
if (myReply.empty()) {
|
if (myReply.empty()) {
|
||||||
ui::Notifications::instance().error(TR("chat_toast_no_zaddr"));
|
ui::Notifications::instance().error(TR("chat_toast_no_zaddr"));
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
const std::string cid = generateChatLocalId("", 16); // opaque per-conversation id
|
|
||||||
chat::OutgoingChatMemos memos;
|
chat::OutgoingChatMemos memos;
|
||||||
if (chat_service_.composeContactRequest(myReply, peerZaddr, cid, text, memos)
|
if (chat_service_.composeContactRequest(myReply, peerZaddr, cid, text, memos)
|
||||||
!= chat::ChatComposeStatus::Ok) {
|
!= chat::ChatComposeStatus::Ok) {
|
||||||
|
|||||||
@@ -129,6 +129,35 @@ bool ChatDatabase::append(const ChatMessage& message)
|
|||||||
return sqlite3_changes(db_) > 0;
|
return sqlite3_changes(db_) > 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
bool ChatDatabase::upsert(const ChatMessage& message)
|
||||||
|
{
|
||||||
|
if (!key_ready_ || !ensureOpen()) return false;
|
||||||
|
|
||||||
|
std::vector<unsigned char> nonce;
|
||||||
|
std::vector<unsigned char> 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<int>(nonce.size()), SQLITE_TRANSIENT);
|
||||||
|
sqlite3_bind_blob(stmt, 4, cipher.data(), static_cast<int>(cipher.size()), SQLITE_TRANSIENT);
|
||||||
|
const bool done = sqlite3_step(stmt) == SQLITE_DONE;
|
||||||
|
sqlite3_finalize(stmt);
|
||||||
|
return done;
|
||||||
|
}
|
||||||
|
|
||||||
std::vector<ChatMessage> ChatDatabase::load()
|
std::vector<ChatMessage> ChatDatabase::load()
|
||||||
{
|
{
|
||||||
std::vector<ChatMessage> out;
|
std::vector<ChatMessage> out;
|
||||||
|
|||||||
@@ -43,6 +43,11 @@ public:
|
|||||||
// Returns true if newly inserted; false on duplicate or while locked.
|
// Returns true if newly inserted; false on duplicate or while locked.
|
||||||
bool append(const ChatMessage& message);
|
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
|
// 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.
|
// while locked or if none. Rows that fail to decrypt/parse are skipped.
|
||||||
std::vector<ChatMessage> load();
|
std::vector<ChatMessage> load();
|
||||||
|
|||||||
@@ -110,14 +110,17 @@ bool ChatService::recordOutgoing(const ChatMessage& message) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
bool ChatService::recordOutgoingPending(const ChatMessage& message) {
|
bool ChatService::recordOutgoingPending(const ChatMessage& message) {
|
||||||
// In-memory only — deliberately NOT persisted yet, so "Sending" never survives to a restart; the
|
// Persist immediately (as Sending) so a send survives an app quit before the broadcast resolves;
|
||||||
// row is written by resolveOutgoing() once the broadcast resolves to Sent/Failed.
|
// resolveOutgoing() then UPSERTS the row to the final status. A stray persisted Sending (crash mid-
|
||||||
return store_.append(message);
|
// 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) {
|
void ChatService::resolveOutgoing(const std::string& txid, ChatDelivery delivery) {
|
||||||
const ChatMessage* updated = store_.updateDelivery(txid, 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
|
} // namespace dragonx::chat
|
||||||
|
|||||||
@@ -76,9 +76,9 @@ public:
|
|||||||
// only local record of what we sent.)
|
// only local record of what we sent.)
|
||||||
bool recordOutgoing(const ChatMessage& message);
|
bool recordOutgoing(const ChatMessage& message);
|
||||||
|
|
||||||
// Two-phase echo for delivery tracking: record in-memory only (shows immediately as Sending),
|
// Two-phase echo for delivery tracking: record + persist immediately as Sending (survives an app
|
||||||
// then resolveOutgoing() flips the status once the broadcast completes AND persists it with the
|
// quit), then resolveOutgoing() upserts the final status once the broadcast completes. A stray
|
||||||
// final status — so a restart never shows a stuck "Sending".
|
// persisted Sending (crash mid-broadcast) loads back as Sent.
|
||||||
bool recordOutgoingPending(const ChatMessage& message);
|
bool recordOutgoingPending(const ChatMessage& message);
|
||||||
void resolveOutgoing(const std::string& txid, ChatDelivery delivery);
|
void resolveOutgoing(const std::string& txid, ChatDelivery delivery);
|
||||||
|
|
||||||
|
|||||||
@@ -513,7 +513,14 @@ void RenderChatTab(App* app)
|
|||||||
ImGui::TextUnformatted(TR("chat_send_failed"));
|
ImGui::TextUnformatted(TR("chat_send_failed"));
|
||||||
ImGui::PopStyleColor(); ImGui::PopFont();
|
ImGui::PopStyleColor(); ImGui::PopFont();
|
||||||
ImGui::SameLine();
|
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.
|
// In-flight send → subtle right-aligned "sending…"; the op callback resolves it.
|
||||||
if (sending) {
|
if (sending) {
|
||||||
|
|||||||
Reference in New Issue
Block a user