Stop using a constant for monitor `update_id`s after closure #3355

TheBlueMatt · 2024-10-10T21:14:56Z

Because `ChannelManager` doesn't have a corresponding `Channel`
after the channels are closed, we'd always used an `update_id` of
`u64::MAX` for any `ChannelMonitorUpdate`s we need to build after
the channel is closed.

This completely breaks the abstraction of `update_id`s and leaks
into persistence logic - because we might have more than one
`ChannelMonitorUpdate` with the same (`u64::MAX`) value, suddenly
instead of being able to safely use `update_id` as IDs, the
`MonitorUpdatingPersister` has to have special logic to handle
this.

Worse, because we don't have a unique ID with which to refer to
post-close `ChannelMonitorUpdate`s we cannot track when they
complete async persistence. This means we cannot properly support
async persist for forwarded payments where the inbound edge has hit
the chain prior to the preimage coming to us.

Here we rectify this by using consistent `update_id`s even after a
channel has closed. In order to do so we have to keep some state
for all channels for which the `ChannelMonitor` has not been
archived (after which point we can be confident we will not need to
update them). While this violates our long-standing policy of
having no state at all in `ChannelManager`s for closed channels,
its only a `(ChannelId, u64)` pair per channel, so shouldn't be
problematic for any of our users (as they already store a whole
honkin `ChannelMonitor` for these channels anyway).

While limited changes are made to the connection-count-limiting
logic, reviewers should carefully analyze the interactions the new
map created here has with that logic.

Sadly, this requires some very annoying upgrade semantics - users cannot upgrade directly from 0.0.123 and earlier to 0.1 as long as they have pending forwarded HTLCs which have not yet been resolved. Instead, all HTLCs must be resolved (and any new HTLCs must have been forwarded using LDK 0.0.124) prior to the upgrade. I'm not particularly happy about this, but in practice I think its okay - those using LDK with forwarding are generally pretty active and upgrade quickly, or at least have the engineering chops to figure out the upgrade pattern (and could backport the inclusion of counterparty_node_id in HTLC forwarding).

codecov · 2024-10-10T21:22:10Z

Codecov Report

Attention: Patch coverage is 72.47387% with 79 lines in your changes missing coverage. Please review.

Project coverage is 89.57%. Comparing base (2c1e828) to head (4156259).
Report is 419 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	74.05%	42 Missing and 20 partials ⚠️
lightning/src/util/persist.rs	52.38%	10 Missing ⚠️
lightning/src/ln/functional_tests.rs	50.00%	0 Missing and 3 partials ⚠️
lightning/src/chain/channelmonitor.rs	85.71%	0 Missing and 2 partials ⚠️
lightning/src/ln/reload_tests.rs	50.00%	0 Missing and 1 partial ⚠️
lightning/src/sync/debug_sync.rs	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main    #3355     +/-   ##
=========================================
  Coverage   89.57%   89.57%             
=========================================
  Files         125      129      +4     
  Lines      101756   105481   +3725     
  Branches   101756   105481   +3725     
=========================================
+ Hits        91151    94488   +3337     
- Misses       7884     8236    +352     
- Partials     2721     2757     +36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

G8XSU

Review checkpoint: reviewed till commit c7b12ff

G8XSU · 2024-10-23T19:30:02Z

lightning/src/ln/channelmanager.rs

+					let context = &chan.context();
+					let logger = WithChannelContext::from(&self.logger, context, None);
+					log_trace!(logger, "Removing channel {} now that signer is unblocked", context.channel_id());
+					update_maps_on_chan_removal!(self, peer_state, context);


Doubt: what was the impact of these dangling references on channel_remove, would it lead to ever increasing memory if there is lot of churn on channels ?

Not completely unbounded - once the ChannelMonitor gets pruned (and the node restarted) they drop off. I think that's a fair tradeoff given each ChannelMonitor weighs a lot more in-memory (and on disk) than this new struct.

G8XSU · 2024-10-23T19:57:56Z

lightning/src/ln/channelmanager.rs

-		let mut failed_htlcs = Vec::new();
+		let empty_peer_state = || {
+			PeerState {
+				channel_by_id: new_hash_map(),


use hashmap_with_capacity?

We don't have a good guesstimate at capacity at this point, we only know the total channel count, not the per-peer channel count. We generally won't have more than one or two channels per peer anyway, so reallocation should be rare.

jkczyz

Still need to look at the last couple commits.

lightning/src/ln/channelmanager.rs

jkczyz · 2024-10-23T21:22:27Z

lightning/src/ln/channelmanager.rs

@@ -2753,6 +2753,12 @@ macro_rules! handle_error {
 	} };
 }

+/// When a channel is removed, two things need to happen:
+/// (a) This must be called in the same `per_peer_state` lock as the channel-closing action,
+/// (b) [`ChannelManager::finish_close_channel`] needs to be called unlocked.


Could you specify which lock? Same below.

No locks, basically...

lightning/src/ln/channelmanager.rs

pending_changelog/matt-no-upgrade-skip.txt

TheBlueMatt · 2024-10-28T17:53:53Z

Rebased and addressed feedback.

valentinewallace · 2024-10-31T17:59:53Z

lightning/src/ln/channelmanager.rs

+					let context = &chan.context();
+					let logger = WithChannelContext::from(&self.logger, context, None);
+					log_trace!(logger, "Removing channel {} now that the signer is unblocked", context.channel_id());
+					update_maps_on_chan_removal!(self, peer_state, context);


It looks like we don't have test coverage for this. Maybe not too crucial but if we want to add it, it looks like it'd be as easy as checking that ChannelManager::outpoint_to_peer (or another manager map) is empty at the end of async_signer_tests test_closing_signed.

Indeed, probably best to address in an async signing PR. I'll tag this comment there.

lightning/src/chain/channelmonitor.rs

valentinewallace

Lot to take in here

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-11-04T19:35:14Z

lightning/src/ln/channelmanager.rs

+				// override the `update_id`, taking care to handle old monitors where the
+				// `latest_update_id` is already `u64::MAX`.
+				let latest_update_id = entry.into_mut();
+				*latest_update_id = latest_update_id.saturating_add(1);


We don't seem to have coverage of the u64::MAX case (same on deser), though that might be tricky to set up in a test :/

Right, that's an upgrade test, so we'd need to go take a dependency on older LDK to build a MAX monitor without doing it by hand. We could do it all by hand (building a ChannelManager state we think is representative), but I'm somewhat hesitant as I assume it'll end up being wrong in some subtle way....

valentinewallace · 2024-11-04T22:23:24Z

lightning/src/ln/channelmanager.rs

+				// We shouldn't consider peer entries for any peers that either have no funded
+				// channels or are disconnected and had some funded channel in the past (and we're
+				// just keeping the entry around for closed_channel_monitor_update_ids).
+				peer.total_channel_count() > 0 || (!peer.is_connected && peer.closed_channel_monitor_update_ids.len() > 0)


Missing coverage for this change

That's because this change does nothing lol. peers_without_funded_channels will already ignore peers which have no channels (which this condition is targeting checking for...). I removed it.

Looks like this wasn't removed?

🤦‍♂️ forgot to push.

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-11-05T18:36:45Z

lightning/src/ln/channelmanager.rs

+						counterparty_node_id,
+						funding_txo: *funding_txo,
+						channel_id,
+						update: monitor_update,


Should we set the monitor_update.counterparty_node_id here as well for posterity?

Doesn't matter. Its only used to set the ChannelMonitor.counterparty_node_id which is already set in this branch.

lightning/src/ln/channelmanager.rs

jkczyz · 2024-11-05T19:09:35Z

lightning/src/ln/channelmanager.rs

+			let short_to_chan_info = self.short_to_chan_info.read().unwrap();
+			short_to_chan_info.get(&prev_hop.short_channel_id).map(|(cp_id, _)| *cp_id)


nit: drop the local variable for a single, chained expression

Chained expressions will lead the eventual rustfmt to be super duper vertical, cause it'll insist on read and unwrap each getting their own line.

YMMV, but personally I'd find it more readable at a glance than when having short_to_chan_info repeated three times.

Yea, its kinda bad either way, rustfmt just kinda backs us into a corner here. I personally find this way at least somewhat readable because we are taking a lock, and naming the lock the same as the mutex seems...fine?

Right, I don't mind that. More so the density of the code.

lightning/src/ln/channelmanager.rs

lightning/src/chain/channelmonitor.rs

jkczyz · 2024-11-05T19:54:51Z

lightning/src/ln/channelmanager.rs

@@ -1299,6 +1299,13 @@ pub(super) struct PeerState<SP: Deref> where SP::Target: SignerProvider {
 	/// will remove a preimage that needs to be durably in an upstream channel first), we put an
 	/// entry here to note that the channel with the key's ID is blocked on a set of actions.
 	actions_blocking_raa_monitor_updates: BTreeMap<ChannelId, Vec<RAAMonitorUpdateBlockingAction>>,
+	/// The latest [`ChannelMonitor::get_latest_update_id`] value for all closed channels as they
+	/// exist on-disk/in our [`chain::Watch`]. This *ignores* all pending updates not yet applied


s/ignores/excludes ?

Don't they mean the same thing here? Not sure what the difference is you're suggesting needs to be captured.

Either is fine, I suppose. "ignores" seems more like an action whereas "excludes" seems more to described a state.

Right, we're not taking any active actions, we're just ignoring as a side effect of not doing anything, so that feels like a state?

lightning/src/ln/channelmanager.rs

`update_maps_on_chan_removal` is used to perform `ChannelManager` state updates when a channel is being removed, prior to dropping the `peer_state` lock. In a future commit we'll use it to update fields in the `per_peer_state`, but in order to do so we'll need to have access to that state in the macro. Here we get set up for this by passing the per-peer state to `update_maps_on_chan_removal`, which is sadly a fairly large patch.

When a channel is closed, we have to call `update_maps_on_chan_removal` in the same per-peer-state lock as the removal of the `ChannelPhase` object. We forgot to do so in `ChannelManager::signer_unblocked` leaving dangling references to the channel. We also take this opportunity to include more context in the channel-closure log in `ChannelManager::signer_unblocked` and add documentation to `update_maps_on_chan_removal` and `finish_close_channel` to hopefully avoid this issue in the future.

In 453ed11 we started tracking the counterparty's `node_id` in `HTLCPreviousHopData`, however we were still trying to look it up using `prev_short_channel_id` in `claim_funds_from_hop`. Because we now usually have the counterparty's `node_id` directly accessible, we should skip the `prev_short_channel_id` lookup. This will also be more important in the next commit where we need to look up state for our counterparty to generate `ChannelMonitorUpdate`s whether we have a live channel or not.

Instead of first building a map from peers to a list of channels then pulling out of that to build the `per_peer_state`, we build `per_peer_state` immediately and store channels in it immediately. This avoids an unnecessary map indirection but also gives us access to the new fields in `per_peer_state` when reading `Channel`s which we'll need in a coming commit.

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-11-05T21:06:38Z

lightning/src/ln/channelmanager.rs

+									return None;
+								}
+
+								if prev_hop.counterparty_node_id.is_none() {


As discussed offline, it's worth looking into whether the checks below should also apply to HTLC receives as well as HTLC forwards.

Mmm, you're right we do need to consider it, but only in unclaimed payments.

If we're doing a re-claim-on-startup there's only two cases and I think we're safe in both:

pre-Stop relying on ChannelMonitor persistence after manager read #3322 (and for old claims) we call directly into the ChannelMonitor with provide_payment_preimage_unsafe_legacy,

post-Stop relying on ChannelMonitor persistence after manager read #3322 we use impl From<&MPPClaimHTLCSource> for HTLCClaimSource which always sets counterparty_node_id.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-11-11T18:48:22Z

I believe I've addressed all the feedback except for missing test coverage.

valentinewallace · 2024-11-12T19:09:20Z

lightning/src/ln/channelmanager.rs

+		for (payment_hash, payment) in claimable_payments.iter() {
+			for htlc in payment.htlcs.iter() {
+				if htlc.prev_hop.counterparty_node_id.is_none() {
+					log_error!(args.logger,


Wouldn't there be value in doing more of the checks that we're doing above for forwards, like checking the monitor claimable balances and the short_to_chan_info map and failing read if we know we're going to panic? Wondering if there's a way to chain these htlcs to the monitor.get_all_current_outbound_htlcs() above to DRY the checks.

Mmm, I was thinking they don't apply because we don't need to check if we need to replay a claim (we know its pending and may claim it when the user wants to), but indeed we should match the "if the channel is still open, just error log and continue anyway" logic, so I did that. I don't think there's much room to DRY them given its just one of the many checks in the forwarding block.

lightning/src/chain/channelmonitor.rs

lightning/src/util/persist.rs

valentinewallace · 2024-11-12T20:14:52Z

lightning/src/ln/channelmanager.rs

+				// We shouldn't consider peer entries for any peers that either have no funded
+				// channels or are disconnected and had some funded channel in the past (and we're
+				// just keeping the entry around for closed_channel_monitor_update_ids).
+				peer.total_channel_count() > 0 || (!peer.is_connected && peer.closed_channel_monitor_update_ids.len() > 0)


Looks like this wasn't removed?

valentinewallace

Looks good mod outstanding feedback and Jeff taking a look

valentinewallace · 2024-11-12T20:19:18Z

lightning/src/util/persist.rs

 	fn update_persisted_channel(
 		&self, funding_txo: OutPoint, update: Option<&ChannelMonitorUpdate>,
 		monitor: &ChannelMonitor<ChannelSigner>,
 	) -> chain::ChannelMonitorUpdateStatus {
+		const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX;


Could we avoid defining this in two places?

jkczyz · 2024-11-12T20:11:49Z

lightning/src/ln/channelmanager.rs

@@ -1299,6 +1299,13 @@ pub(super) struct PeerState<SP: Deref> where SP::Target: SignerProvider {
 	/// will remove a preimage that needs to be durably in an upstream channel first), we put an
 	/// entry here to note that the channel with the key's ID is blocked on a set of actions.
 	actions_blocking_raa_monitor_updates: BTreeMap<ChannelId, Vec<RAAMonitorUpdateBlockingAction>>,
+	/// The latest [`ChannelMonitor::get_latest_update_id`] value for all closed channels as they
+	/// exist on-disk/in our [`chain::Watch`]. This *ignores* all pending updates not yet applied


Either is fine, I suppose. "ignores" seems more like an action whereas "excludes" seems more to described a state.

jkczyz · 2024-11-12T20:16:32Z

lightning/src/ln/channelmanager.rs

+			let short_to_chan_info = self.short_to_chan_info.read().unwrap();
+			short_to_chan_info.get(&prev_hop.short_channel_id).map(|(cp_id, _)| *cp_id)


YMMV, but personally I'd find it more readable at a glance than when having short_to_chan_info repeated three times.

lightning/src/ln/channelmanager.rs

jkczyz · 2024-11-12T20:49:53Z

lightning/src/ln/channelmanager.rs

 							// monitor updates still in flight. In that case, we shouldn't
 							// immediately free, but instead let that monitor update complete
 							// in the background.


Is this comment still relevant?

Yea, the behavior hasn't changed, we just no longer debug-assert that the new monitor update is already in-flight.

jkczyz

LGTM. Feel free to squash, IMO.

Currently we store in-flight `ChannelMonitorUpdate`s in the per-peer structure in `ChannelManager`. This is nice and simple as we're generally updating it when we're updating other per-peer data, so we already have the relevant lock(s) and map entries. Sadly, when we're claiming an HTLC against a closed channel, we didn't have the `counterparty_node_id` available until it was added in 0.0.124 (and now we only have it for HTLCs which were forwarded in 0.0.124). This means we can't look up the per-peer structure when claiming old HTLCs, making it difficult to track the new `ChannelMonitorUpdate` as in-flight. While we could transition the in-flight `ChannelMonitorUpdate` tracking to a new global map indexed by `OutPoint`, doing so would result in a major lock which would be highly contended across channels with different peers. Instead, as we move towards tracking in-flight `ChannelMonitorUpdate`s for closed channels we'll keep our existing storage, leaving only the `counterparty_node_id` issue to contend with. Here we simply accept the issue, requiring that `counterparty_node_id` be available when claiming HTLCs against a closed channel. On startup, we explicitly check for any forwarded HTLCs which came from a closed channel where the forward happened prior to 0.0.124, failing to deserialize, or logging an warning if the channel is still open (implying things may work out, but panics may occur if the channel closes prior to HTLC resolution). While this is a somewhat dissapointing resolution, LDK nodes which forward HTLCs are generally fairly well-upgraded, so it is not anticipated to be an issue in practice.

In the next commit we'll drop the magic `u64::MAX` `ChannelMonitorUpdate::update_id` value used when we don't know the `ChannelMonitor`'s `latest_update_id` (i.e. when the channel is closed). In order to do so, we will store further information about `ChannelMonitor`s in the per-peer structure, keyed by the counterparty's node ID, which will be used when applying `ChannelMonitorUpdate`s to closed channels. By taking advantage of the change in the previous commit, that information is now reliably available when we generate the `ChannelMonitorUpdate` (when claiming HTLCs), but in order to ensure it is available when applying the `ChannelMonitorUpdate` we need to use `BackgroundEvent::MonitorUpdateRegeneratedOnStartup` instead of `BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup` where possible. Here we do this, leaving `ClosedMonitorUpdateRegeneratedOnStartup` only used to ensure very old channels (created in 0.0.118 or earlier) which are not in the `ChannelManager` are force-closed on startup.

Because `ChannelManager` doesn't have a corresponding `Channel` after the channels are closed, we'd always used an `update_id` of `u64::MAX` for any `ChannelMonitorUpdate`s we need to build after the channel is closed. This completely breaks the abstraction of `update_id`s and leaks into persistence logic - because we might have more than one `ChannelMonitorUpdate` with the same (`u64::MAX`) value, suddenly instead of being able to safely use `update_id` as IDs, the `MonitorUpdatingPersister` has to have special logic to handle this. Worse, because we don't have a unique ID with which to refer to post-close `ChannelMonitorUpdate`s we cannot track when they complete async persistence. This means we cannot properly support async persist for forwarded payments where the inbound edge has hit the chain prior to the preimage coming to us. Here we rectify this by using consistent `update_id`s even after a channel has closed. In order to do so we have to keep some state for all channels for which the `ChannelMonitor` has not been archived (after which point we can be confident we will not need to update them). While this violates our long-standing policy of having no state at all in `ChannelManager`s for closed channels, its only a `(ChannelId, u64)` pair per channel, so shouldn't be problematic for any of our users (as they already store a whole honkin `ChannelMonitor` for these channels anyway). While limited changes are made to the connection-count-limiting logic, reviewers should carefully analyze the interactions the new map created here has with that logic.

If a peer creates a channel with us which never reaches the funding stage (or never gets any commitment updates after creation), we'll avoid inserting the `update_id` into `closed_channel_monitor_update_ids` at runtime to avoid keeping a `PeerState` entry around for no reason. However, on startup we still create a `ChannelMonitorUpdate` with a `ChannelForceClosed` update step to ensure the `ChannelMonitor` is locked and shut down. This is pretty redundant, and results in a bunch of on-startup `ChannelMonitorUpdate`s for any old but non-archived `ChannelMonitor`s. Instead, here, we check if a `ChannelMonitor` already saw a `ChannelForceClosed` update step before we generate the on-startup `ChannelMonitorUpdate`. This also allows us to skip the `closed_channel_monitor_update_ids` insertion as we can be confident we'll never have a `ChannelMonitorUpdate` for this channel at all.

TheBlueMatt · 2024-11-13T01:24:29Z

Squashed without further changes.

TheBlueMatt added this to the 0.1 milestone Oct 10, 2024

G8XSU self-requested a review October 10, 2024 21:35

jkczyz self-requested a review October 17, 2024 17:37

G8XSU reviewed Oct 23, 2024

View reviewed changes

jkczyz reviewed Oct 23, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch 2 times, most recently from a5f0571 to 52ef7f8 Compare October 29, 2024 20:19

valentinewallace reviewed Oct 31, 2024

View reviewed changes

valentinewallace reviewed Nov 4, 2024

View reviewed changes

valentinewallace reviewed Nov 5, 2024

View reviewed changes

jkczyz reviewed Nov 5, 2024

View reviewed changes

TheBlueMatt added 4 commits November 5, 2024 20:13

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch from 52ef7f8 to 55976a8 Compare November 5, 2024 20:14

jkczyz reviewed Nov 5, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Nov 5, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch from 55976a8 to 938d2a2 Compare November 11, 2024 18:48

TheBlueMatt mentioned this pull request Nov 11, 2024

Handle fallible per commitment point in channel establishment #3109

Open

valentinewallace reviewed Nov 12, 2024

View reviewed changes

jkczyz reviewed Nov 12, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch 2 times, most recently from 19e90b2 to 7a2b272 Compare November 12, 2024 21:18

jkczyz reviewed Nov 12, 2024

View reviewed changes

TheBlueMatt added 4 commits November 13, 2024 01:24

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch from 7a2b272 to 0bf2975 Compare November 13, 2024 01:24

f always insert a latest_update_id even if the chan is closed

4156259

TheBlueMatt force-pushed the 2024-10-mon-ids-after-close branch from 82771bb to 4156259 Compare November 13, 2024 14:21

		let short_to_chan_info = self.short_to_chan_info.read().unwrap();
		short_to_chan_info.get(&prev_hop.short_channel_id).map(\|(cp_id, _)\| *cp_id)

Stop using a constant for monitor update_ids after closure #3355

Are you sure you want to change the base?

Stop using a constant for monitor update_ids after closure #3355

Conversation

TheBlueMatt commented Oct 10, 2024

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

G8XSU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 13, 2024

Stop using a constant for monitor `update_id`s after closure #3355

Stop using a constant for monitor `update_id`s after closure #3355

codecov bot commented Oct 10, 2024 •

edited

Loading

valentinewallace Nov 5, 2024 •

edited

Loading