Repository navigation
staticaddr: store static loop-in times in UTC - #1252
claravanstaden wants to merge 5 commits into
Conversation
1127542 to
4637564
Compare
| SwapHash: loopIn.SwapHash[:], | ||
| Preimage: loopIn.SwapPreimage[:], | ||
| InitiationTime: loopIn.InitiationTime, | ||
| InitiationTime: loopIn.InitiationTime.UTC(), |
There was a problem hiding this comment.
Shall we plan for a migration for existing swap times too?
There was a problem hiding this comment.
I don't think so, no. SQLite swaps are mostly saved with the timezone (except for some countries like Nepal, etc), so they will convert correctly when read from the database. For Postgres, we simply don't know the timezone which it was saved it, so we'd be guessing the correct time if we do a migration, which I don't think is better.
This is OK because these time values are just used for display anyway and for ordering a swap's own updates. This PR changes the order to by ID instead of time anyway.
| // UTC and with the PostgreSQL-compatible timestamp precision. | ||
| func sqlStoreUpdateTime(clock clock.Clock) time.Time { | ||
| return clock.Now().Truncate(time.Microsecond) | ||
| return clock.Now().UTC().Truncate(time.Microsecond) |
There was a problem hiding this comment.
Monitor still uses timestamps to reject stale updates in isInitialStaticAddressLoopInStale (loopd/swapclient_server.go:703).
On Postgres in UTC+2, an old update can read back as 14:01, while the next update after upgrading is stored as 12:05 UTC. If a monitor subscribes before that next update, its snapshot records 14:01. It then drops the newer state as stale, potentially including completion.
Recovery does not always prevent this: self-transitions skip persistence, so the old timestamp can survive until the next state change. Sorting by ID fixes database state selection, but this monitor comparison still assumes timestamps increase.
Could we use the persisted update ID for monitor stale detection too, with a regression test for an old snapshot followed by a newer UTC update?
There was a problem hiding this comment.
Thanks for catching this. The monitor now compares the update IDs instead of the timestamps, and the store keeps the ID of each swap's latest update.
I added some tests for this: TestStaticAddressLoopInHighWaterKeepsNewerUpdateWithEarlierTime covers an old snapshot followed by a newer update with an earlier time, and TestMonitorSuppressesStaticAddressLoopInSnapshotLiveDuplicate now checks the same end to end.
The static address itests pass locally on Postgres in UTC+2, except static_address_deposit_expiry, which fails the same way on master.
4637564 to
de6e58b
Compare
GetStaticAddressLoopInSwapsByStates took a swap's latest update to be the one with the latest timestamp. Timestamps written in different time zones don't sort in order: east of UTC, an update written in local time has a later timestamp than an update written in UTC after it. A swap could then be selected by a state it had already left, for example a finished swap by a pending state, so that it is recovered at every start. Two updates with the same timestamp also returned the swap twice. Take the update that was inserted last instead, and order the updates of a swap by id, since the last one sets the swap's state.
The static loop-in store wrote the risk decision time, the initiation time and the update times in the clock's local time zone. A Postgres TIMESTAMP column drops the time zone, so outside UTC these times read back shifted by the offset. The payment deadline is counted from the risk decision time, so it moved by the offset: later east of UTC, and already over when the server accepted the risk west of UTC. SQLite keeps the time zone and wasn't affected. Write the times in UTC, like the loop-out, deposit and withdrawal stores. Times stored before this change keep their shift.
A swap's update times don't always increase: an update stored in local time before the times were stored in UTC reads back as UTC on Postgres, so east of UTC it reads back later than the next update. The update ids do increase. Return the id of an inserted update, and keep the id of a swap's latest update next to its time, so that updates can be ordered by id.
The monitor drops a live static loop-in update that is older than the swap's update in the initial snapshot, so that the same update isn't sent twice. It compared the update times, so after an upgrade east of UTC on Postgres, a snapshot with an update stored in local time made the monitor drop the swap's newer updates, including its completion, for the rest of the subscription. Compare the ids of the updates instead.
de6e58b to
659b751
Compare
3 participants
A fix for an edge case bug where the static loop-in store writes its times (risk decision, initiation and updates) in local time, instead of UTC. On Postgres, a
TIMESTAMPcolumn drops the time zone, so outside UTC these times read back shifted by the offset. SQLite keeps the time zone, so the time reads back correctly, except in zones whose offset is shown as a number, such as Iran (+0330) or Nepal (+0545), where it can't be read back at all. The payment deadline starts at the risk decision time, so it moved by the offset: later east of UTC, and already over west of UTC, which cancels the invoice as soon as the server accepts the risk. Reproduced with Postgres in America/New_York: the invoice was cancelled 0 s after the server accepted the risk.loop monitorupdates by id too. The monitor drops a live update that is older than the swap's update in its initial snapshot, so east of UTC an old local-time update in the snapshot made it drop the swap's newer updates, including its completion.TestSqlStoreTimesOutsideUTCfails on Postgres without the fix, andTestLatestLoopInUpdateIsLastInsertedfails on both databases without the first commit.TestStaticAddressLoopInHighWaterKeepsNewerUpdateWithEarlierTimeandTestMonitorSuppressesStaticAddressLoopInSnapshotLiveDuplicatefail without the monitor commit.Pull Request Checklist
docs/release-notes/release-notes-next.md, or apply theno-changeloglabel (required by CI)