Extended API for fine-grained payment tracking #867

akumaigorodski · 2019-02-14T12:14:15Z

This is (yet another) attempt to sneak in an extended API which tries to improve on current limitations:

send method has a hard timeout so can't be relied upon when payment takes a long time.
Irregular cases, such as on-chain refunds and lost dusty payments, are not accounted for.

Above limitations are tolerable when Eclair is run by user directly, but are problematic when API is used by a service which sends/receives payments on behalf of it's users since a service has to be able to tell users what is going on with their payments as well as reliably track these payments at all times.

New API strategy is this:

send becomes a fire-and-forget method.
Two new Payment events are added: PaymentSettlingOnChain and PaymentLostOnChain.
Outgoing payment is considered PENDING until websoket sends PaymentSent | PaymentLostOnChain | PaymentSettlingOnChain(isDone = true), alternatively user may call sentinfo API endpoint which would return the same objects.
Incoming payment is considered PENDING until it is expired or websocket sends PaymentReceived | PaymentLostOnChain | PaymentSettlingOnChain(isDone = true), alternatively user may call receivedinfo API endpoint which would return the same objects.

One serious change this requires is https://github.com/btcontract/eclair/commit/a76ec92e3c1d09608c3db37f819de416fc0cbbc1#diff-aeb98076f262543b70a86753bd3719adR53 which adds paymentHash to HtlcTimeoutTx. This is needed for PaymentSettlingOnChain event which has paymentHash and txid fields. txid there belongs to a final CSV-delayed refunding transaction and allows API user to link off-chain failed payment to on-chain refunding transaction.

Also send `PaymentSettlingOnChan` and `PaymentLostOnChain` when uncooperative closing happens.

…database

akumaigorodski · 2019-02-14T12:23:40Z

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

+      val inFlightHtlcs = commitments.localCommit.spec.htlcs.map(htlc => htlc.add.paymentHash -> htlc.add.amountMsat).toMap
+      var onChainHashes = Set.empty[BinaryData] // this is needed to collect hashes of payments which can be resolved on-chain
+
+      val successAndDelayedTxs = for {


An advantage of using for-sequences here:

if either success or delayed fails then no tx is published as opposed to current approach where success may be published but delayed won't be (because applying on-chain fees would make it dusty for example).

if either success or delayed fails then no PaymentSettlingOnChain will be sent, but PaymentLostOnChain will be sent for this payment instead.

Further, if LocalCommitPublished can be modified to store (success, delayed) and (timeout, delayed) tuples directly then UI and API may inform how much blocks is left for each delayed payment: CLTV(timeout) + CSV(delayed) as well as total on-chain fees paid: timeout.input.amount - delayed.output.amount.

akumaigorodski · 2019-02-14T12:37:01Z

IntegrationSpec seems to be failing, I'm not sure what's going on here, any advice?

araspitzu · 2019-02-14T12:43:09Z

IntegrationSpec seems to be failing, I'm not sure what's going on here, any advice?

Looks like integration-test ecalir nodes couldn't find a config key, maybe because of this addition? https://github.com/ACINQ/eclair/pull/867/files#diff-8f6c9262b4235c9580874fdd69f54b7cR234 Notice that config key has been moved in the router block, and has become router.broadcast-interval

pm47

Concept OK, but I have several problems with the implementation:

Please submit a separate PR for the formatting changes. I'm not convinced by most of them to be honest, and they make the review more difficult;
The audit db isn't supposed to be used as a random access db but more like an append-only log for audit purpose. I know that there is a demand for a real payment database and I think we should probably add one;
I understand the need to index payments by payment_hash (although it is not a real index!) but this could be inferred from the local commitment.
Just because we call claimCurrentLocalCommitTxOutputs/claimRemoteCommitTxOutputs doesn't mean that this will be confirmed. If an htlc is fulfilled in the local tx, and if local commit wins the race despite remote publishing its local commit, then the code will produce an invalid event AFAICT.

Let me think about this a bit more and I may come up with an alternate proposal.

pm47 · 2019-03-05T16:11:52Z

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

@@ -202,7 +204,7 @@ object NodeParams {
    NodeParams(
      keyManager = keyManager,
      alias = nodeAlias,
-      color = Color(color.data(0), color.data(1), color.data(2)),
+      color = Color(color.data.head, color.data(1), color.data(2)),


Same comment as above. For this particular change it hurts readibility more than anything, as previous code was more consistent.

pm47 · 2019-03-05T16:17:26Z

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

@@ -179,7 +181,7 @@ object NodeParams {
      val p = PublicKey(e.getString("nodeid"))
      val gf = BinaryData(e.getString("global-features"))
      val lf = BinaryData(e.getString("local-features"))
-      (p -> (gf, lf))
+      (p, (gf, lf))


Are you following intellij recommendations or some automated code modification suggestions? This makes review more difficult are there are lot of unrelated changes.

Can you revert and submit a separate PR for those?

Also by convention I Iike using -> for defining tuples in the context of a Map.

pm47 · 2019-03-05T16:30:15Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

+      }
+
+      def receive: Receive = {
+        case message: PaymentFailed => flowInput.offer(Serialization write message)


The json serialization should probably be part of the flow instead of duplicating Serialization.write(...)

pm47 · 2019-03-05T16:33:19Z

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


  val forwarder = context.actorOf(Props(new Forwarder(nodeParams)), "forwarder")

  // this will be used to detect htlc timeouts
-  context.system.eventStream.subscribe(self, classOf[CurrentBlockCount])
+  implicitStream.subscribe(self, classOf[CurrentBlockCount])


I don't think we should refer to the implicit variable here. Same for logging: we use an implicit that is passed to the Helper functions for context, but we call directly log from within the actor. Does that make sense?

pm47 · 2019-03-05T16:43:49Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala

@@ -50,15 +50,15 @@ class LocalPaymentHandler(nodeParams: NodeParams) extends Actor with ActorLoggin
    case PurgeExpiredRequests =>
      context.become(run(hash2preimage.filterNot { case (_, pr) => hasExpired(pr) }))

-    case ReceivePayment(amount_opt, desc, expirySeconds_opt, extraHops) =>
+    case ReceivePayment(amount_opt, desc, expirySeconds_opt, extraHops, fallbackAddressOpt) =>


Options should be suffixed with _opt by convention

pm47 · 2019-03-05T16:49:11Z

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

@@ -230,10 +230,10 @@ object PaymentRequest {
      f.version match {
        case 17 if prefix == "lnbc" => Base58Check.encode(Base58.Prefix.PubkeyAddress, data)
        case 18 if prefix == "lnbc" => Base58Check.encode(Base58.Prefix.ScriptAddress, data)
-        case 17 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)
-        case 18 if prefix == "lntb" => Base58Check.encode(Base58.Prefix.ScriptAddressTestnet, data)
+        case 17 if prefix == "lntb" || prefix == "lnbcrt" => Base58Check.encode(Base58.Prefix.PubkeyAddressTestnet, data)


# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala # eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala

This is a simpler alternative to #867, with the following limitations: - no `OnChainRefundsDb` and associated API calls - `PaymentSettlingOnChain` event will be sent exactly once per payment and have less information - we don't touch `HtlcTimeoutTx` - use json4s type hints instead of manual attributes to case classes

pm47 · 2019-04-15T13:24:56Z

This is has been superseded by #884 and #885.

anton added 5 commits February 14, 2019 12:36

Add payment hash to HtlcTimeoutTx in backwards compatible way.

a29b5f3

Also send `PaymentSettlingOnChan` and `PaymentLostOnChain` when uncooperative closing happens.

Record PaymentSettlingOnChain and PaymentLostOnChain events in a …

eb401bd

…database

Record sent, received and failed payment infos in AuditDb

59da1ac

Add sentinfo and receivedinfo API endpoints

fe33608

Minor bugfix

2598e9a

akumaigorodski commented Feb 14, 2019

View reviewed changes

anton added 3 commits February 15, 2019 11:09

Fix NodeParams fields

c87239e

Allow to generate payment requests with fallback on-chain addresses

fc5006c

Make stream parameter implicit

8d696f1

pm47 self-assigned this Mar 5, 2019

pm47 requested changes Mar 5, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into extended-api-2

25a8866

# Conflicts: # eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala # eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala

pm47 mentioned this pull request Mar 6, 2019

Send events when HTLCs settle on-chain #884

Merged

pm47 closed this Apr 15, 2019

akumaigorodski deleted the extended-api-2 branch November 3, 2019 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended API for fine-grained payment tracking #867

Extended API for fine-grained payment tracking #867

akumaigorodski commented Feb 14, 2019 •

edited

Loading

akumaigorodski Feb 14, 2019

akumaigorodski commented Feb 14, 2019

araspitzu commented Feb 14, 2019 •

edited

Loading

pm47 left a comment

pm47 Mar 5, 2019

pm47 Mar 5, 2019

pm47 Mar 5, 2019

pm47 Mar 5, 2019

pm47 Mar 5, 2019

pm47 Mar 5, 2019

pm47 commented Apr 15, 2019

Extended API for fine-grained payment tracking #867

Extended API for fine-grained payment tracking #867

Conversation

akumaigorodski commented Feb 14, 2019 • edited Loading

akumaigorodski Feb 14, 2019

Choose a reason for hiding this comment

akumaigorodski commented Feb 14, 2019

araspitzu commented Feb 14, 2019 • edited Loading

pm47 left a comment

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 Mar 5, 2019

Choose a reason for hiding this comment

pm47 commented Apr 15, 2019

akumaigorodski commented Feb 14, 2019 •

edited

Loading

araspitzu commented Feb 14, 2019 •

edited

Loading