Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metadata to local_channels table #1724

Merged
merged 5 commits into from
Mar 10, 2021
Merged

Add metadata to local_channels table #1724

merged 5 commits into from
Mar 10, 2021

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Mar 8, 2021

(First commit is a refactor/cleanup and can be reviewed independently).

Here is the rationale for implementing channel metadata as additional
columns in the local_channels table of the channels db, as opposed
to a dedicated channel_metadata table of a audit db:

  1. There is a migration to do (in the local_channels table, no less!),
    but it's just a table migration, as opposed to a data migration, if we
    had to populate a new table in a separate database.
  2. We don't need to worry about creating a new metadata line when a new
    channel is created (compared to doing add-or-update stuff). It's only
    updating optional columns in a best-effort manner.
  3. We don't need to worry about inconsistencies between two tables
    located in two separated databases (that's a big one).
  4. We may want to use the metadata during operations, not just for
    audit purposes. For example to close channels that have stayed unused
    for a long time.
  5. The audit db is an append-only log of events and shouldn't be used
    for anything else. There is no UPDATE sql statement in
    *AuditDb.scala. The channel_metadata would break that heuristic.

@pm47
Copy link
Member Author

pm47 commented Mar 8, 2021

@sstone @t-bast the PR isn't finished but I'd like to have your feedback before I go further. I did start by implementing this in the audit db, before changing for this approach.

@pm47 pm47 requested review from t-bast and sstone March 8, 2021 16:12
@t-bast
Copy link
Member

t-bast commented Mar 8, 2021

That sounds reasonable, these are good reasons to go down that route.
As long as we use separate, optional columns and not modify the core ChannelData structures let's try and see where it takes us.

@pm47 pm47 marked this pull request as ready for review March 10, 2021 10:01
Move it to the `db` package (it was in the `payments` package for
historical reasons but doesn't deal only with payment anymore).

Better typing for channel lifecycle event.
Here is the rationale for implementing channel metadata as additional
columns in the `local_channels` table of the `channels` db, as opposed
to a dedicated `channel_metadata` table of a `audit` db:

1) There is a migration to do (in the `local_channels` table, no less!),
but it's just a table migration, as opposed to a data migration, if we
had to populate a new table in a separate database.
2) We don't need to worry about creating a new metadata line when a new
channel is created (compared to doing add-or-update stuff). It's only
_updating_ optional columns in a best-effort manner.
3) We don't need to worry about inconsistencies between two tables
located in two separated databases (that's a big one).
4) We may want to use the metadata during operations, not just for
audit purposes. For example to close channels that have stayed unused
for a long time.
5) The audit db is an append-only log of events and shouldn't be used
for anything else. There is no `UPDATE` sql statement in
`*AuditDb.scala`. The `channel_metadata` would break that heuristic.
@codecov-io
Copy link

codecov-io commented Mar 10, 2021

Codecov Report

Merging #1724 (b46a27f) into master (afa378f) will decrease coverage by 0.07%.
The diff coverage is 81.92%.

@@            Coverage Diff             @@
##           master    #1724      +/-   ##
==========================================
- Coverage   86.30%   86.22%   -0.08%     
==========================================
  Files         151      151              
  Lines       11701    11781      +80     
  Branches      501      498       -3     
==========================================
+ Hits        10098    10158      +60     
- Misses       1603     1623      +20     
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 54.74% <ø> (ø)
...ain/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala 85.13% <60.86%> (-11.17%) ⬇️
...main/scala/fr/acinq/eclair/db/DbEventHandler.scala 90.32% <84.37%> (ø)
...a/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala 95.94% <95.65%> (-0.36%) ⬇️
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 70.83% <100.00%> (ø)
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala 98.97% <100.00%> (ø)
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala 99.06% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/PaymentEvents.scala 97.61% <0.00%> (-2.39%) ⬇️
...main/scala/fr/acinq/eclair/router/Validation.scala 90.76% <0.00%> (-1.54%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumClient.scala 73.52% <0.00%> (-0.37%) ⬇️
... and 5 more

@pm47 pm47 merged commit ded5ce0 into master Mar 10, 2021
@pm47 pm47 deleted the channel-meta branch March 10, 2021 18:32
tompro pushed a commit to tompro/eclair that referenced this pull request Mar 21, 2021
* rename `Auditor` to `DbEventHandler`

Move it to the `db` package (it was in the `payments` package for
historical reasons but doesn't deal only with payment anymore).

Better typing for channel lifecycle event.

* add meta info to local_channels table

Here is the rationale for implementing channel metadata as additional
columns in the `local_channels` table of the `channels` db, as opposed
to a dedicated `channel_metadata` table of a `audit` db:

1) There is a migration to do (in the `local_channels` table, no less!),
but it's just a table migration, as opposed to a data migration, if we
had to populate a new table in a separate database.
2) We don't need to worry about creating a new metadata line when a new
channel is created (compared to doing add-or-update stuff). It's only
_updating_ optional columns in a best-effort manner.
3) We don't need to worry about inconsistencies between two tables
located in two separated databases (that's a big one).
4) We may want to use the metadata during operations, not just for
audit purposes. For example to close channels that have stayed unused
for a long time.
5) The audit db is an append-only log of events and shouldn't be used
for anything else. There is no `UPDATE` sql statement in
`*AuditDb.scala`. The `channel_metadata` would break that heuristic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants