Since 991a4a8, the `make_actionbar_model` function introduced an async
call to populate the action bar data.
This surfaced a pre-existing problem where the action bar model would
reinstantiate in any SwiftUI render pass for the chat bubbles in
`ChatroomThreadView`. This issue was not visible before because the whole
computation happened directly on the main actor during the render,
maintaining the illusion of a stable entity. Since the computation was
moved to an async task (for performance and concurrency design reasons),
it caused the action bar items to reload in each render pass, causing
multiple re-renders and the jumpiness witnessed in the ticket.
The issue was addressed by making the action bar model initialization
happen within ChatEventView itself, and wrapping it on `StateObject` to
make that entity stable across re-renders.
This fixes an issue for an unreleased change, so no changelog entry is
necessary.
Changelog-None
Fixes: 991a4a8
Closes: https://github.com/damus-io/damus/issues/3270
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This fixes a crash that would occasionally occur when visiting profiles.
NdbTxn objects were being deinitialized on different threads from their
initialization, causing incorrect reference count decrements in thread-local
transaction dictionaries. This led to premature destruction of shared ndb_txn
C objects still in use by other tasks, resulting in use-after-free crashes.
The root cause is that Swift does not guarantee tasks resume on the same
thread after await suspension points, while NdbTxn's init/deinit rely on
thread-local storage to track inherited transaction reference counts.
This means that `NdbTxn` objects cannot be used in async functions, as
that may cause the garbage collector to deinitialize `NdbTxn` at the end
of such function, which may be running on a different thread at that
point, causing the issue explained above.
The fix in this case is to eliminate the `async` version of the
`NdbNoteLender.borrow` method, and update usages to utilize other
available methods.
Note: This is a rewrite of the fix in https://github.com/damus-io/damus/pull/3329
Note 2: This relates to the fix of an unreleased feature, so therefore no
changelog is needed.
Changelog-None
Co-authored-by: alltheseas <64376233+alltheseas@users.noreply.github.com>
Closes: https://github.com/damus-io/damus/issues/3327
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Note: This is an improvement on an unreleased feature, so no changelog
entry is needed.
Changelog-None
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
No changelog is needed because we already have changelog messages for
the features added.
Changelog-None
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
`seen_event` set was not isolated, which lead to occasional race
conditions between different actors accessing it simultaneously, leading
to crashes.
Closes: https://github.com/damus-io/damus/issues/3311
Changelog-Fixed: Fixed an occasional random crash related to viewing profiles
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit adds a new event subscription task in HomeModel, one which
streams low volume but important filters from NostrDB.
This was done to address an issue where the contact filters in the
general handler task could yield too many notes from NostrDB, hitting
its limits and clipping off important events such as mute-lists, leading
to downstream issues such as unintended mute-list overrides.
This issue was not present since the last public release, therefore no
changelog entry is needed.
Changelog-None
Closes: https://github.com/damus-io/damus/issues/3256
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Some issues were encountered with this feature. Disabling it for now.
Once we have the full Damus Labs UI, we will add the feature there.
Changelog-Changed: Placed the Favorites feature behind a feature flag
Closes: https://github.com/damus-io/damus/issues/3304
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
It was noticed that occasionally the profile name at the event view
would not load to the user's display name.
The issue was fixed by adding a missing profile observer.
Issue introduced after our last public release, no need for changelog
entries.
Changelog-None
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Previously, the note content rendering logic would wait for the note
and its blocks to be available in NostrDB before displaying the parsed
version of the note to the user.
However, it is now possible to parse those on demand, and there are code
paths to ensure that it will do so if those parsed blocks are not
readily available from NostrDB.
Therefore, the wait is not necessary, and removing it fixes the delay we
have been experiencing.
The issue was likely introduced after the last public release, so no
changelog item is needed
Changelog-None
Closes: https://github.com/damus-io/damus/issues/3296
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
A race condition was identified where notes would get dropped if they
get indexed in the time window between when a query is made and the subscription is made.
The issue was fixed by making the subscribe call before making the query
call, to ensure we get all notes from that time when we perform the
query.
This dropped the failure rate for ndb subscription tests from about 20%
down to about 4%.
Local relay model issue was not publicly released, which is why the
changelog entry is "none".
Changelog-None
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This attempts to reduce race conditions coming from Ndb streaming
functions that could lead to lost notes or crashes.
It does so by making two improvements:
1. Instead of callbacks, now the callback handler uses async streams,
which reduces the chances of a callback being called before the last
item was processed by the consumer.
2. The callback handler will now queue up received notes if there are
no listeners yet. This is helpful because we need to issue the
subscribe call to nostrdb before getting the subscription id and
setting up a listener, but in between that time nostrdb may still
send notes which would effectively get dropped without this queuing
mechanism.
Changelog-Fixed: Improved robustness in the part of the code that streams notes from nostrdb
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This may negatively impact performance, but improves accuracy and
prevents profile loading issues
Changelog-None
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This fixes a hang during sign-up, which was caused by a change in
RelayPool handling code that would only send data to handlers with
matching subscription IDs.
It so happens that some handlers want to receive all notes, and they set
the filters to `nil` to achieve that.
Furthermore, some sign-up networking code was moved to prevent race conditions.
No changelog entry because the behaviour was not changed since the last
public release.
Changelog-None
Closes: https://github.com/damus-io/damus/issues/3254
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This is a large refactor that aims to improve performance by offloading
RelayPool computations into a separate actor outside the main thread.
This should reduce congestion on the main thread and thus improve UI
performance.
Also, the internal subscription callback mechanism was changed to use
AsyncStreams to prevent race conditions newly found in that area of the
code.
Changelog-Fixed: Added performance improvements to timeline scrolling
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This attempts to improve the performance of InnerTimelineView by
performing event filtering computations on "EventHolder.insert" instead
of on each view body re-render, to improve SwiftUI performance.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This feature is not production-ready, and is not essential for the
current scope of work, so descoping it and hiding it behind a feature
flag until it is ready.
Changelog-Removed: Removed "Load new content" button
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This improves upon a temporary fix we had for the RelayPool race
condition that would cause timeline staleness.
The root cause was that during app launch, the HomeModel would subscribe
to some filters, and the subscribe function would filter out any relays
not yet connected to avoid unnecessary waiting for EOSEs from disconnected relays.
However, that filtering would cause the subscribe request to not be
queued up or sent back to the relays once connected, causing the relays
to never receive those subscription requests and causing timeline
staleness.
This was fixed by separating the relay list used for the subcription
request from the relay list used for waiting for network EOSEs. This
allows other mechanisms to ensure the subscription will go through even
when the app is initializing and relays are not yet fully connected.
Fixes: 61eb833239
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Previously, we combined the ndb and network stream within a "session
subscription" stream, which was teared down and rebuilt every time the
app went into the background and back to the foreground (This was done to
prevent crashes related to access to Ndb memory when Ndb is closed).
However, this caused complications and instability on the network
stream, leading to timeline staleness.
To address this, the pipeline was modified to merge the ndb and network
streams further upstream, on the multi-session stage, allowing the
session subscription streams to be completely split between Ndb and the
network.
For the ndb stream, we still tear it down and bring it up along the app
foreground state, to prevent memory crashes. However, the network stream
is kept intact between sessions, since RelayPool will now automatically
handle resubscription on websocket reconnection. This prevents
complexity and potential race conditions that could lead to timeline
staleness.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
- Resend subscription requests to relays when websocket connection is
re-established
- More safeguard checks on whether Ndb is opened before accessing its
memory
- Cancel queued unsubscribe requests on app backgrounding to avoid race
conditions with subscribe requests when app enters the foreground
- Call Ndb re-open when Damus is active (not only on active notify), as
experimentally there have been instances where active notify code has
not been run. The operation is idempotent, so there should be no risk
of it being called twice.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Use Apple's unified logging system, and specify proper privacy levels
for each piece of information.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit adds more safeguards to prevent RUNNINGBOARD 0xdead10cc
crashes, by:
1. Using the `beginBackgroundTask(withName:expirationHandler:)` to
request additional background execution time before completely
suspending the app. See https://developer.apple.com/documentation/xcode/sigkill
2. Reorganizing app closing/cleanup tasks to be done in parallel when
possible to decrease time needed to cleanup resources.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This should prevent RUNNINGBOARD 0xdead10cc crashes related to
ProfileManager and app background states.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This should prevent background crashes caused by race conditions between
usages of Ndb and the Ndb/app lifecycle operations.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Through some local experimentation, it seems that network relays can support higher subscription limits.
Increase internal limits to avoid hitting issues with subscriptions
waiting on subscription pool to clear and appearing stale.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This reduces the overall subscription usage throughout the app, thus
reducing issues associated with too many subscriptions being used at
once, and the resulting staleness.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit improves the loading speed for the home timeline (and likely
other areas of the app) by employing various techniques and changes:
- Network EOSE timeout reduced from 10 seconds down to 5 seconds
- Network EOSE does not wait on relays with broken connections
- Offload HomeModel handler event processing to separate tasks to
avoid a large backlog
- Give SubscriptionManager streamers more fine-grained EOSE signals for
local optimization
- Only wait for Ndb EOSE on the home timeline for faster loading
- Add logging with time elapsed measurements for easier identification of
loading problems
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This is done to prevent hang ups when the device is offline.
Changelog-Added: Added the ability to load saved notes if device is offline
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit takes a step back from the full local relay model by
treating NostrDB as one of the many relays streamed from, instead of the
one exclusive relay that other classes rely on.
This was done to reduce regression risk from the local relay model
migration, without discarding the migration work already done.
The full "local relay model" behavior (exclusive NDB streaming) was
hidden behind a feature flag for easy migration later on.
Closes: https://github.com/damus-io/damus/issues/3225
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit improves NostrNetworkManager interfaces to be easier to use,
and with more options on how to read data from the Nostr network
This reduces the amount of duplicate logic in handling streams, and also
prevents possible common mistakes when using the standard subscribe method.
This fixes an issue with the mute list manager (which prompted for this
interface improvement, as the root cause is similar to other similar
issues).
Closes: https://github.com/damus-io/damus/issues/3221
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit implements nostr network subscriptions that survive between
sessions, as well as improved handling of RelayPool opening/closing with
respect to the app lifecycle.
This prevents stale data after users swap out and back into Damus.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Previously, HomeModel could listen to all subscriptions throughout the
app, and it would handle reaction and repost counting.
Once moved to the local relay model, HomeModel no longer had access to
all subscriptions, causing those counts to disappear.
The issue was fixed by doing the counting from ThreadModel itself, which
better isolates concerns throughout the app.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit fixes a crash that caused the app to crash when getting all
the follows from a profile.
This issue was caused by a use-after-free memory error on inherited
transactions after the original transaction is deinitialized.
The issue was fixed by introducing a reference count on all transactions
and only deallocating the C transaction when the ref count goes to zero.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
The widespread usage of the SubscriptionManager caused new crashes to
occur when swapping apps.
This was caused due to an access to Ndb memory after Ndb has been closed
from the app background signal.
The issue was fixed with improved task management logic and ensuring all
subscription tasks are finished before closing Ndb.
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit introduces a verification step at the relay connection
level, to help ensure notes get validated at the source and prevent
security issues associated with untrusted relays.
`RelayConnection.swift` — the source that initially handles WebSocket
messages — was analyzed, and measures were put in place to prevent
(or at least minimize) unverified nostr event data being spread
throughout the app.
The following measures were taken:
1. A note verification step was added prior to the `self.handleEvent(.nostr_event(ev))` call (which sends a Nostr response to the rest of the app for logical handling).
a. From code analysis, there is only one such call in `RelayConnection.swift`.
2. `NostrConnectionEvent`, the object that gets passed to event handlers, had its interface modified to remove the "message" case, since:
a. that could be a source of unverified nostr events.
b. it is redundant an unneeded due to the `.nostr_event` case.
c. there were no usages of it around the codebase
3. The raw websocket event handler had its label renamed to "handleUnverifiedWSEvent", to make it clear to the caller about the verification status of the data.
a. Usages of this were inspected and no significant risk was detected.
4. A new `verify` method in NdbNote was created to verify Nostr notes, and unit tests were added to confirm tampering detections around all the major fields in a Nostr note.
5. Care was taken to ensure the performance regression is as little as
possible.
It is worth noting that we will not need this once the local relay model
architecture is introduced, since that architecture ensures note
validation before it reaches the rest of the application and the user.
In other words, this is a temporary fix.
However, since the migration to that new architecture is a major
undertaking that will take some time to be completed, this fix was written
in order to address security concerns while the migration is unfinished.
This fix was written in a way that attempts to be as effective as
possible in reducing security risks without a risky and lenghty
refactor of the code that would delay the fix from being published.
Changelog-Fixed: Improved security around note validation
Closes: https://github.com/damus-io/damus/issues/1341
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
This commit fixes a crash that occurred when swapping between Damus and
other apps.
When Damus enters background mode, NostrDB is closed and its resources
released. When Damus re-enters foreground mode, NostrDB is reopened.
However, an issue with the transaction inheritance logic
caused a race condition where a side menu profile lookup would get an
obsolete transaction containing pointers that have been freedwhen
NostrDB was closed, causing a "use-after-free" memory error.
The issue was fixed by improving the transaction inheritance logic to
double-check if the "generation" counter (which auto increments when
Damus closes and re-opens) matches the generation marked on the
thread-specific transaction. This effectively prevents lookups from
inheriting an obsolete transaction from a previous NostrDB generation.
Closes: https://github.com/damus-io/damus/issues/3167
Changelog-Fixed: Fixed an issue where the app would crash when swapping between apps
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Daniel D’Aquino