Set relay fees per node and save them to database #1890

thomash-acinq · 2021-07-19T14:01:26Z

Fees are set per node instead of per channel (setting different fees for different channels to the same node is most probably an error)
Fees are saved to a database so that we can keep a trace of historic fees and new channels with a known node use the fee that we set and not the default fee.

codecov-commenter · 2021-07-20T13:20:57Z

Codecov Report

Merging #1890 (44da4e2) into master (131ae8b) will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1890      +/-   ##
==========================================
+ Coverage   87.34%   87.35%   +0.01%     
==========================================
  Files         159      159              
  Lines       12004    12110     +106     
  Branches      473      461      -12     
==========================================
+ Hits        10485    10579      +94     
- Misses       1519     1531      +12

Impacted Files	Coverage Δ
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala	`100.00% <ø> (ø)`
.../main/scala/fr/acinq/eclair/db/DualDatabases.scala	`0.00% <0.00%> (ø)`
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	`86.29% <72.72%> (-0.11%)`	⬇️
...c/main/scala/fr/acinq/eclair/db/pg/PgPeersDb.scala	`84.37% <80.95%> (-1.99%)`	⬇️
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala	`95.84% <84.61%> (-1.24%)`	⬇️
...cala/fr/acinq/eclair/db/sqlite/SqlitePeersDb.scala	`91.22% <86.36%> (-3.22%)`	⬇️
...cala/fr/acinq/eclair/db/sqlite/SqliteAuditDb.scala	`98.47% <92.30%> (-0.69%)`	⬇️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`53.06% <100.00%> (+2.01%)`	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	`96.06% <100.00%> (+0.01%)`	⬆️
...main/scala/fr/acinq/eclair/db/DbEventHandler.scala	`92.42% <100.00%> (+0.36%)`	⬆️
... and 12 more

pm47

Nice cleanup.

This PR should be rebased on top of master and use the newly introduced RelayFees class instead of tuples.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/RelayFeesDb.scala

pm47 · 2021-08-09T14:57:24Z

eclair-core/src/main/scala/fr/acinq/eclair/db/RelayFeesDb.scala

+
+import java.io.Closeable
+
+trait RelayFeesDb extends Closeable {


What is the rationale for not adding this to the AuditDb? It would fit right in I think.

AuditDb should contain information for auditing only and should not be required for running the node, it should be write only. This new database is required for running the node as eclair reads from it.

eclair reads from it

I missed that! In that case, it belongs to the PeersDb I think. There is an ambiguity though, because we are also using that same table as an append-only, timestamped, event log. How about a very basic key-value table in PeersDb and an event log table in AuditDb? Yes there is duplication, but it's consistent with what we have between PaymentsDb and AuditDb.

We could also just put the relay fee changes in the channel_events table, or introduce a new peer_events table.

Done, I've added a channel_updates table in AuditDb.

- Fees are set per node instead of per channel (setting different fees for different channels to the same node is most probably an error) - Fees are saved to a database so that we can keep a trace of historic fees and new channels with a known node use the fee that we set and not the default fee.

t-bast

Nice, I'm glad we can get rid of the ugly initialRelayFees_opt I added :)
You mention in the PR description that historical fees are kept, but that's not the case in the latest iteration, is it?

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

eclair-core/src/test/scala/fr/acinq/eclair/db/PeersDbSpec.scala

t-bast

LGTM. Let's wait for a final review from @pm47 before merging as he has more experience actually managing node fees than I do ;)

pm47

Like @t-bast I'm also wondering why you have dropped the historical fees, do you feel like it's not worth it anymore? Can also be added later.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/PeersDb.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgPeersDb.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/PeersDb.scala

eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/Relayer.scala

eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala

thomash-acinq · 2021-08-11T13:10:20Z

I've added the historical fees back. They're now in AuditDb.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala

t-bast · 2021-08-11T16:08:29Z

eclair-core/src/main/scala/fr/acinq/eclair/db/DbEventHandler.scala

@@ -116,6 +118,11 @@ class DbEventHandler(nodeParams: NodeParams) extends Actor with ActorLogging {
      auditDb.add(ChannelEvent(e.channelId, e.commitments.remoteParams.nodeId, e.commitments.commitInput.txOut.amount, e.commitments.localParams.isFunder, !e.commitments.announceChannel, event))
      channelsDb.updateChannelMeta(e.channelId, event)

+    case u: LocalChannelUpdate =>


NB: this will trigger a lot of noise, we resend this every time a peer reconnects...but maybe it's ok since it doesn't contain that much data

evd0kim · 2021-09-16T10:26:00Z

Are you sure you haven't broken old way of setting fees? @t-bast
Because this is what i get in response to my old command with channel ids

The form field 'nodeId(s)' was malformed:
Must specify nodeId or nodeIds (but not both)

evd0kim · 2021-09-16T10:29:49Z

That's for sure, you have broke old interface.
Then please maybe update documentation.

pm47 · 2021-09-16T10:40:48Z

That's for sure, you have broke old interface.

There has been a backward-incompatible change, yes. Need to use nodeId(s) instead of channelId(s).

Then please maybe update documentation.

I think @t-bast does that right before release. In the meantime:

curl -u :<eclair_api_password> -X POST -F nodeId=<node> \
     -F feeBaseMsat=<feebase> -F feeProportionalMillionths=<feeproportional> \
     "http://localhost:8080/updaterelayfee"

t-bast · 2021-09-20T07:09:57Z

Then please maybe update documentation.

The documentation always reflects the latest release.
If you're running master, you can't expect the documentation to be up-to-date (otherwise we would need to have different documentation for every commit, which is way too much work).

I'm trying to improve that with #1951 so that release notes are written as master progresses.
That means that when you're running master, you can always check the draft release notes for the next release to see what has changed since the latest version.

thomash-acinq force-pushed the relay-fees-per-node branch from 7b991a9 to 17a65d9 Compare July 20, 2021 13:09

thomash-acinq force-pushed the relay-fees-per-node branch 3 times, most recently from 6887a42 to 2ee89fc Compare July 21, 2021 09:40

thomash-acinq requested a review from t-bast July 21, 2021 11:18

thomash-acinq marked this pull request as ready for review July 21, 2021 11:18

thomash-acinq force-pushed the relay-fees-per-node branch from 2ee89fc to 7fa170b Compare July 29, 2021 07:26

thomash-acinq mentioned this pull request Jul 29, 2021

Different default relay fees for announced and unannounced channels #1893

Merged

thomash-acinq requested a review from pm47 August 3, 2021 08:47

pm47 reviewed Aug 9, 2021

View reviewed changes

thomash-acinq added 3 commits August 9, 2021 17:03

Use RelayFees instead of tuple

fa926d1

Put RelayFees in PeersDb

be0ef4d

thomash-acinq force-pushed the relay-fees-per-node branch from 7fa170b to be0ef4d Compare August 10, 2021 09:25

t-bast reviewed Aug 10, 2021

View reviewed changes

Use all local channels

42d7dc5

t-bast previously approved these changes Aug 10, 2021

View reviewed changes

pm47 reviewed Aug 10, 2021

View reviewed changes

thomash-acinq added 2 commits August 11, 2021 11:38

Fix review comments

9b93677

Add channel updates to auditDb

fb07447

thomash-acinq dismissed t-bast’s stale review via fb07447 August 11, 2021 13:07

thomash-acinq requested review from pm47 and t-bast August 11, 2021 13:10

pm47 reviewed Aug 11, 2021

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated Show resolved Hide resolved

Call to auditDb from DbEventHandler and refactor auditDb

a592e00

thomash-acinq requested a review from pm47 August 11, 2021 15:20

pm47 reviewed Aug 11, 2021

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala Outdated Show resolved Hide resolved

No refactor yet

44da4e2

thomash-acinq requested a review from pm47 August 11, 2021 15:50

pm47 approved these changes Aug 11, 2021

View reviewed changes

thomash-acinq merged commit 8f5f6ac into master Aug 11, 2021

t-bast reviewed Aug 11, 2021

View reviewed changes

t-bast mentioned this pull request Aug 12, 2021

Make updaterelayfee API method take node ids #1701

Closed

t-bast mentioned this pull request Dec 6, 2021

Feature Request #2085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set relay fees per node and save them to database #1890

Set relay fees per node and save them to database #1890

thomash-acinq commented Jul 19, 2021

codecov-commenter commented Jul 20, 2021 •

edited

Loading

pm47 left a comment

pm47 Aug 9, 2021

thomash-acinq Aug 9, 2021

pm47 Aug 9, 2021 •

edited

Loading

thomash-acinq Aug 11, 2021

t-bast left a comment

t-bast left a comment

pm47 left a comment

thomash-acinq commented Aug 11, 2021

t-bast Aug 11, 2021

evd0kim commented Sep 16, 2021

evd0kim commented Sep 16, 2021

pm47 commented Sep 16, 2021

t-bast commented Sep 20, 2021


		import java.io.Closeable

		trait RelayFeesDb extends Closeable {

Set relay fees per node and save them to database #1890

Set relay fees per node and save them to database #1890

Conversation

thomash-acinq commented Jul 19, 2021

codecov-commenter commented Jul 20, 2021 • edited Loading

Codecov Report

pm47 left a comment

Choose a reason for hiding this comment

pm47 Aug 9, 2021

Choose a reason for hiding this comment

thomash-acinq Aug 9, 2021

Choose a reason for hiding this comment

pm47 Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

thomash-acinq Aug 11, 2021

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

thomash-acinq commented Aug 11, 2021

t-bast Aug 11, 2021

Choose a reason for hiding this comment

evd0kim commented Sep 16, 2021

evd0kim commented Sep 16, 2021

pm47 commented Sep 16, 2021

t-bast commented Sep 20, 2021

codecov-commenter commented Jul 20, 2021 •

edited

Loading

pm47 Aug 9, 2021 •

edited

Loading