Add a globalbalance API call #1737

pm47 · 2021-03-23T09:20:00Z

For now it just gives detailed info on the current pending mutual closes.

codecov-commenter · 2021-06-01T15:43:39Z

Codecov Report

Merging #1737 (4b78392) into master (85ed433) will decrease coverage by 0.70%.
The diff coverage is 47.66%.

@@            Coverage Diff             @@
##           master    #1737      +/-   ##
==========================================
- Coverage   88.94%   88.24%   -0.71%     
==========================================
  Files         150      154       +4     
  Lines       11239    11417     +178     
  Branches      447      426      -21     
==========================================
+ Hits         9997    10075      +78     
- Misses       1242     1342     +100

Impacted Files	Coverage Δ
...ain/scala/fr/acinq/eclair/balance/Monitoring.scala	`0.00% <0.00%> (ø)`
.../main/scala/fr/acinq/eclair/channel/Register.scala	`81.48% <ø> (ø)`
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`47.09% <6.25%> (-5.05%)`	⬇️
...n/scala/fr/acinq/eclair/balance/BalanceActor.scala	`16.66% <16.66%> (ø)`
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala	`57.83% <57.83%> (ø)`
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala	`92.89% <100.00%> (+0.03%)`	⬆️
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala	`80.45% <100.00%> (+0.57%)`	⬆️
...ala/fr/acinq/eclair/balance/ChannelsListener.scala	`100.00% <100.00%> (ø)`
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala	`95.86% <100.00%> (-1.01%)`	⬇️
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala	`94.11% <100.00%> (+0.17%)`	⬆️
... and 10 more

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

pm47 · 2021-06-29T14:26:08Z

eclair-core/src/main/scala/fr/acinq/eclair/balance/ChannelsListener.scala

+
+import scala.concurrent.Promise
+
+object ChannelsListener {


The goal of this actor is to maintain a local cache of all channel data, that can be used by the API, instead of querying thousands of channel actors. Surprisingly, this is turns out to not be noticeably faster than the ask+Future.sequence pattern.

Maybe we should remove this? Or we could keep it and use that for all remaining API calls, because it saves some burden of the channel actors? I'm not sure.

I think it's useful to have it, when we'll have more load on the channel actors it should be noticeably useful, don't you think?

pm47 · 2021-06-29T15:28:00Z

Squashed and rebased.

t-bast

First pass, I haven't looked in details in the actual balance computation, but I'm proposing a few architectural changes and found some nits.

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

t-bast · 2021-06-30T09:52:24Z

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

@@ -53,15 +57,31 @@ case class AuditResponse(sent: Seq[PaymentSent], received: Seq[PaymentReceived],

 case class TimestampQueryFilters(from: Long, to: Long)

+case class MutualCloseStatus(unpublished: Satoshi, unconfirmed: Satoshi, confirmed: Satoshi)


nit: MutualCloseBalance would be more consistent IMO

Actually, this is just additional information about mutual close transactions, useful to track evictions. The mutual close balance is tracked as part of onchain balance, see:

/** * Mutual close transactions are always immediately published, and will be taken into account by bitcoin core, so we * don't count them in the balance. */ case class ClosingBalance

But now that I think of it, we should probably treat mutual closes like other transactions, in the offchain balance, and prune it afterwards. The goal of pruning was initially to remove duplicates for transactions that were published (and possibly confirmed but less than mindepth), but it should also nicely handle evicted transactions.

Ok, I remember now why the mutual close transactions were handled like this. The problem is that there can be several competing mutual closes, and in that case the simple pruning logic that we have to make sure we count the amount exactly once doesn't work. This would lead us to possibly count twice the amount in the competing-mutual-closes scenario.

On the other hand, what we had before meant that we didn't count the amount in the case where the mutual close was evicted. Not perfect either, and this is why we had the additional MutualCloseStatus item.

In general I would say that it is better to undershoot rather than overshoot the balance computation, but here the trade-off makes sense for consistency, and also it is easy to tell when we are potentially counting a mutual close twice: the mutualCloseBalance should always be zero after pruning.

So I went ahead and did it with 4b78392, with a detailed comment on what is going on.

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

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/ExtendedBitcoinClient.scala

eclair-core/src/main/scala/fr/acinq/eclair/balance/ChannelsListener.scala

eclair-core/src/main/scala/fr/acinq/eclair/balance/CheckBalance.scala

eclair-core/src/main/scala/fr/acinq/eclair/balance/Monitoring.scala

eclair-core/src/main/scala/fr/acinq/eclair/balance/BalanceActor.scala

eclair-core/src/test/scala/fr/acinq/eclair/balance/CheckBalanceSpec.scala

t-bast · 2021-07-02T11:58:43Z

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

@@ -2370,6 +2374,24 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
    // at best we have a little less than 450 000 + 250 000 + 100 000 + 50 000 = 850 000 (because fees)
    assert(amountClaimed === 814880.sat)

+    val commitments = alice.stateData.asInstanceOf[DATA_CLOSING].commitments


We can leave it like that for now, but I'm not a big fan of adding new unrelated checks inside existing tests (especially when these tests are already quite long and complex).

It's a bit more code, but I would not put any balance test inside this file, I would instead write these tests in CheckBalanceSpec (where you would setup channels and HTLCs similarly to what's done here, but with the flexibility of changing the scenarios tested without impacting the rest of the test).

eclair-node/src/main/scala/fr/acinq/eclair/api/serde/JsonSerializers.scala

t-bast

LGTM. I haven't looked in details at the actual balance computation details, real-world experience is the best judge for whether it works or not ;)

And we can easily implement several balance computation algorithms and run them in parallel, and see which one mirrors more closely reality.

It returns an overall balance, separating onchain, offchain, and removing duplicates (e.g. mutual closes that haven't reached min depth still have an associated channel, but they already appear in the on-chain balance). We also take into account known preimages, even if the htlc hasn't been formally resolved. Metrics have also been added. fixup: typo Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> better serializers naming consistency treat mutual close transactions like other closes move utxo metrics to balance actor Also refactor to make it consistent with the existing actor. NB: there is still duplication in the metrics, will handle that separately. rename UnlockOutpointUtxo -> UnlockOutpoint refactor interaction between api and balance actor fixed half-finished comment improve the event stream synchronization trick remove left-over logs Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> make GetGlobalBalance return a Try, not a Future make merge easier in feature branches

pm47 · 2021-07-07T14:48:41Z

Rebased and squashed.

pm47 force-pushed the balance-api branch from a100d3d to 4628102 Compare April 2, 2021 16:05

pm47 force-pushed the balance-api branch 2 times, most recently from 24f0e57 to 9e272ae Compare May 7, 2021 14:02

pm47 force-pushed the balance-api branch 3 times, most recently from 2439f6c to 0c77de5 Compare May 19, 2021 14:43

pm47 force-pushed the balance-api branch from 3836ad2 to de39ffe Compare June 1, 2021 15:26

pm47 force-pushed the balance-api branch 2 times, most recently from 0452c6e to 6a756c4 Compare June 3, 2021 08:33

pm47 marked this pull request as ready for review June 29, 2021 14:13

pm47 requested a review from t-bast June 29, 2021 14:13

pm47 commented Jun 29, 2021

View reviewed changes

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

pm47 commented Jun 29, 2021

View reviewed changes

pm47 force-pushed the balance-api branch from 6a756c4 to 382b71a Compare June 29, 2021 15:27

t-bast reviewed Jun 30, 2021

View reviewed changes

pm47 requested a review from t-bast July 1, 2021 15:27

t-bast reviewed Jul 2, 2021

View reviewed changes

t-bast previously approved these changes Jul 5, 2021

View reviewed changes

pm47 dismissed t-bast’s stale review via 53d9aa7 July 6, 2021 14:47

pm47 mentioned this pull request Jul 7, 2021

Add json columns in Postgres #1865

Merged

pm47 force-pushed the balance-api branch from 53d9aa7 to f16d3ae Compare July 7, 2021 14:48

t-bast approved these changes Jul 7, 2021

View reviewed changes

pm47 merged commit bd57d41 into master Jul 8, 2021

pm47 deleted the balance-api branch July 8, 2021 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a globalbalance API call #1737

Add a globalbalance API call #1737

pm47 commented Mar 23, 2021

codecov-commenter commented Jun 1, 2021 •

edited

pm47 Jun 29, 2021

t-bast Jun 30, 2021

pm47 commented Jun 29, 2021

t-bast left a comment

t-bast Jun 30, 2021

pm47 Jun 30, 2021

pm47 Jul 1, 2021 •

edited

t-bast Jul 2, 2021

t-bast left a comment

pm47 commented Jul 7, 2021

		@@ -53,15 +57,31 @@ case class AuditResponse(sent: Seq[PaymentSent], received: Seq[PaymentReceived],

		case class TimestampQueryFilters(from: Long, to: Long)

		case class MutualCloseStatus(unpublished: Satoshi, unconfirmed: Satoshi, confirmed: Satoshi)


		import scala.concurrent.Promise

		object ChannelsListener {

Add a globalbalance API call #1737

Add a globalbalance API call #1737

Conversation

pm47 commented Mar 23, 2021

codecov-commenter commented Jun 1, 2021 • edited

Codecov Report

pm47 Jun 29, 2021

Choose a reason for hiding this comment

t-bast Jun 30, 2021

Choose a reason for hiding this comment

pm47 commented Jun 29, 2021

t-bast left a comment

Choose a reason for hiding this comment

t-bast Jun 30, 2021

Choose a reason for hiding this comment

pm47 Jun 30, 2021

Choose a reason for hiding this comment

pm47 Jul 1, 2021 • edited

Choose a reason for hiding this comment

t-bast Jul 2, 2021

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

pm47 commented Jul 7, 2021

codecov-commenter commented Jun 1, 2021 •

edited

pm47 Jul 1, 2021 •

edited