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

AB testing #1930

Merged
merged 23 commits into from Sep 9, 2021
Merged

AB testing #1930

merged 23 commits into from Sep 9, 2021

Conversation

thomash-acinq
Copy link
Member

@thomash-acinq thomash-acinq commented Aug 31, 2021

Enable doing AB testing to find the best path finding parameters.

When a new payment needs to use path finding, it will be assigned randomly an experiment (which may be the control experiment) and use its set of parameters. When the payment finishes (either with a success or failure), we record metrics such as the fee we paid and the time the payment took.
After sending enough payments we can compute the success rate, average fee and average duration of payments for a given set of parameters and choose the best one.

Each experiments is assigned some percentage of the traffic. It is advised to start low with new experiments in case the new parameters are terrible but should then be increased to collect enough data timely. Experiments can be at 0% in which case they will never be used by live relay traffic but can still be used manually using the API.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #1930 (c4ecd4a) into master (663094e) will increase coverage by 0.04%.
The diff coverage is 19.50%.

@@            Coverage Diff            @@
##           master   #1930      +/-   ##
=========================================
+ Coverage    8.96%   9.00%   +0.04%     
=========================================
  Files         158     159       +1     
  Lines       12330   12423      +93     
  Branches      530     535       +5     
=========================================
+ Hits         1105    1119      +14     
- Misses      11225   11304      +79     
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 0.00% <0.00%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 2.39% <0.00%> (-0.20%) ⬇️
...main/scala/fr/acinq/eclair/db/DbEventHandler.scala 0.00% <0.00%> (ø)
.../main/scala/fr/acinq/eclair/db/DualDatabases.scala 0.00% <0.00%> (ø)
...c/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala 0.00% <0.00%> (ø)
.../scala/fr/acinq/eclair/payment/PaymentEvents.scala 0.00% <ø> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 0.00% <0.00%> (ø)
...scala/fr/acinq/eclair/payment/send/Autoprobe.scala 0.00% <0.00%> (ø)
...clair/payment/send/MultiPartPaymentLifecycle.scala 0.00% <0.00%> (ø)
...r/acinq/eclair/payment/send/PaymentInitiator.scala 0.00% <0.00%> (ø)
... and 12 more

@thomash-acinq thomash-acinq marked this pull request as ready for review September 2, 2021 07:51
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that it's a pretty simple change. Haven't reviewed the metrics+db part yet.

Note to self: requires updating eclair-front.

eclair-core/src/main/resources/reference.conf Outdated Show resolved Hide resolved
eclair-core/src/main/resources/reference.conf Outdated Show resolved Hide resolved
eclair-core/src/main/resources/reference.conf Outdated Show resolved Hide resolved
Comment on lines 425 to 434
case class RouteParams(randomize: Boolean,
maxFeeBase: MilliSatoshi,
maxFeePct: Double,
routeMaxLength: Int,
routeMaxCltv: CltvExpiryDelta,
ratios: WeightRatios,
mpp: MultiPartParams,
includeLocalChannelCost: Boolean,
experimentName: String,
experimentPercentage: Int) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just merge RouteParams and PathFindingConf, because they do not have the same meaning, as the special handling of includeLocalChannelCost shows. PathFindingConf is static but RouteParams depends on a particular route request.

How about (cc @t-bast interested in your opinion):

  case class PathFindingConf(randomizeRouteSelection: Boolean,
                             boundaries: SearchBoundaries,
                             weights: WeightRatios,
                             mpp: MultiPartParams)

  // new class that groups some parameters, not sure about Search
  case class SearchBoundaries(maxFeeBase: MilliSatoshi,
                              maxFeePct: Double,
                              maxRouteLength: Int,
                              maxCltv: CltvExpiryDelta)

  case class RouteParams(conf: PathFindingConf,
                         includeLocalChannelCost: Boolean)

Copy link
Member

@pm47 pm47 Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with this, it would make sense to create subsections:

    defaults {
      randomize-route-selection = true

      boundaries {
        max-route-length = 6
        max-cltv = 1008
        fee-threshold-sat = 21
        max-fee-pct = 0.03
      }

      ratios {
        base = 0.0
        cltv = 0.05
        channel-age = 0.4
        channel-capacity = 0.55
        hop-cost-base-msat = 500
        hop-cost-millionths = 200
      }

      mpp {
        min-amount-satoshis = 15000
        max-parts = 5
      }
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed PathFindingConf to be a nested case class as you suggested but I kept RouteParams as a separate flat case class because many of its fields are modified in many places and it's not possible to modify nested fields in a nice way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at quicklens:

import com.softwaremill.quicklens.ModifyPimp
val aliceParams2 = Alice.nodeParams.modify(_.routerConf.requestNodeAnnouncements).setTo(true)

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feature, I really like the way it's presented and configurable in reference.conf.
There are important details about what exact events we should measure that we need to discuss, the details can be subtle and may highly influence the resulting data.

t-bast
t-bast previously approved these changes Sep 9, 2021
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@thomash-acinq thomash-acinq merged commit 768a745 into master Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants