search

fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 210 forks source link

Gateway registrations are stored on servers by `gateway_id` #3389

Closed tvolk131 closed 3 months ago

tvolk131 commented 7 months ago

Fixes #3290

m1sterc001guy commented 7 months ago

I know we discuss on the dev call awhile back that this might be a good candidate to do a database migration for. Any thoughts?

m1sterc001guy commented 7 months ago

Some tests likely need to change as well

tvolk131 commented 7 months ago

I know we discuss on the dev call awhile back that this might be a good candidate to do a database migration for. Any thoughts?

Ahh yes, I forgot about that! Thanks for the reminder, I'll give that a shot.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (c83d7a9) 60.16% compared to head (12566d7) 60.10%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3389 +/- ## ========================================== - Coverage 60.16% 60.10% -0.07% ========================================== Files 193 193 Lines 40361 40382 +21 ========================================== - Hits 24283 24271 -12 - Misses 16078 16111 +33 ``` | [Files](https://app.codecov.io/gh/fedimint/fedimint/pull/3389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint) | Coverage Δ | | |---|---|---| | [modules/fedimint-ln-server/src/lib.rs](https://app.codecov.io/gh/fedimint/fedimint/pull/3389?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint#diff-bW9kdWxlcy9mZWRpbWludC1sbi1zZXJ2ZXIvc3JjL2xpYi5ycw==) | `67.32% <43.47%> (-0.47%)` | :arrow_down: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/fedimint/fedimint/pull/3389/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fedimint)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tvolk131 commented 7 months ago

This is ready for review, except I'm not sure what strategy we want to use to test the migration. The V0 data uses the same pubkey for node_pub_key and gateway_id:

https://github.com/fedimint/fedimint/blob/b76f0a9459e6387f1be1f7dec3115e7a285c9493/modules/fedimint-ln-server/src/lib.rs#L1435-L1439

...

https://github.com/fedimint/fedimint/blob/b76f0a9459e6387f1be1f7dec3115e7a285c9493/modules/fedimint-ln-server/src/lib.rs#L1512-L1528

So asserting that gateways are keyed by gateway_id in the migration test passes even if the migration doesn't run. I think it would be worth rewriting create_db_with_v0_data() to generate more realistic data. Does this seem reasonable?

m1sterc001guy commented 7 months ago

So asserting that gateways are keyed by gateway_id in the migration test passes even if the migration doesn't run. I think it would be worth rewriting create_db_with_v0_data() to generate more realistic data. Does this seem reasonable?

When adding a migration, you should create a new test, called create_db_with_v1_data that tests the migration from a "v1 backup", where the V1 backup contains a db backup of the new data.

That said, this is a bit weird, since the database change here is actually backwards compatible, since they're both the same kind of key. Usually the database change would cause the v0 db backup to be invalid, which is not the case for this change. So I would agree that we should change the v0 test to use different keys for node_pub_key and gateway_id.

elsirion commented 7 months ago

Do you want to backport to 0.1? Otherwise don't worry about DB migrations.

elsirion commented 6 months ago

This PR currently only consists of DB migration code it seems, what's the status here?

tvolk131 commented 6 months ago

I think we decided we don't want to backport, so we're going to skip the migration. Apologies for the delay on this one - I've been busy and simply haven't gotten around to removing the migration.

m1sterc001guy commented 4 months ago

@tvolk131 What is the status of this?

elsirion commented 4 months ago

Since we stabilized Fedimint since then we'll need migrations now :see_no_evil:

m1sterc001guy commented 4 months ago

Yes but this should be pretty straight forward, might be a good candidate if we want to ship a simple migration

tvolk131 commented 4 months ago

@tvolk131 What is the status of this?

No update, haven't touched this in a while but I can circle back to it this week!

Since we stabilized Fedimint since then we'll need migrations now 🙈

Ope! Sorry I didn't get around to this sooner! But as Justin mentioned, could be a great test for a simple migration.

m1sterc001guy commented 3 months ago

Fixed in https://github.com/fedimint/fedimint/pull/4114