test: gateway short channel id assignments #4275
Conversation
okjodom
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4275 +/- ##
==========================================
+ Coverage 58.06% 58.12% +0.06%
==========================================
Files 197 197
Lines 43755 43740 -15
==========================================
+ Hits 25405 25425 +20
+ Misses 18350 18315 -35 ☔ View full report in Codecov by Sentry. |
| @@ -1167,6 +1167,82 @@ async fn test_gateway_shows_info_about_all_connected_federations() -> anyhow::Re | |||
| .await | |||
| } | |||
|
|
|||
| #[tokio::test(flavor = "multi_thread")] | |||
| async fn test_gateway_can_leave_connected_federations() -> anyhow::Result<()> { | |||
There was a problem hiding this comment.
Should we also add a devimint test that reboots the gateway in between to verify that the short channel ids are correct after the restart (or maybe the existing devimint test can be modified to cover this)?
There was a problem hiding this comment.
IIRC the bug we observed only happened after restarting the gateway. That accurate @douglaz?
I would try to leave the federation with the lowest SCID, then restart gateway, then join a new previously un-joined federation.
Tricky thing is that we may need at least 2 federations for this devimint test?
We can't really tests a gatewayd restart in this test because it uses memdb?
There was a problem hiding this comment.
The duplicated id was certainly after restarting. But we also had issues (same? others?) before restarting.
There was a problem hiding this comment.
Adding these tests highlighted several bugs / slight logical with scid assignments. If we backported this to 0.2.1 and updated a live gateway, future scid assignments when connecting / disconnecting feds would be predictable as defined by the new test spec.
Tricky thing is that we may need at least 2 federations for this devimint test?
Yes. We have a gateway reboot test, but we can't improve it's coverage to scid assignments because devimint is limited. Relevant to #3985
We can't really tests a gatewayd restart in this test because it uses memdb?
Well this also is a limitation for testing persisted states
okjodom
force-pushed
the
test-scids
branch
5 times, most recently
from
d27b2af
to
2c44e17
Compare
okjodom
changed the title
okjodom
requested review from
elsirion,
m1sterc001guy,
douglaz and
justinmoon
|
Needs rebase |
- renames `channel_id_generator` to `max_used_scid` - fixes channel id tracking in load clients. here we only need to track the max used channel id assigned to a loaded client. this change ensures we only increment the channel id when connecting to new federations.
- when a federation is removed from the gateway scid_to_federation map. this frees up scid slots for potential reuse by other connected feds
INITIAL_SCID = 0because we always increment the scid value before assignment.