Skip to content

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented May 11, 2021

We use one actor per topic, but each actor previously registered to multiple topics so we received duplicate events and consumed twice the necessary bandwidth.

Thanks @btcontract for noticing this in #1789

We use one actor per topic, but each actor previously registered to multiple
topics so we received duplicate events and consumed twice the necessary
bandwidth.
@t-bast t-bast requested a review from pm47 May 11, 2021 07:27
@codecov-commenter
Copy link

Codecov Report

Merging #1793 (83f549e) into master (a8d4e07) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1793      +/-   ##
==========================================
- Coverage   89.37%   89.35%   -0.03%     
==========================================
  Files         143      143              
  Lines       10932    10933       +1     
  Branches      478      476       -2     
==========================================
- Hits         9771     9769       -2     
- Misses       1161     1164       +3     
Impacted Files Coverage Δ
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 80.23% <100.00%> (ø)
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 95.12% <100.00%> (+0.12%) ⬆️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 96.09% <0.00%> (-1.57%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.39% <0.00%> (-0.09%) ⬇️

@akumaigorodski
Copy link
Contributor

akumaigorodski commented May 11, 2021

To me it seems a bit strange that every instance of ZMQActor has logic for both rawtx and rawblock yet subscribes to a single topic, maybe it's more straightforward to use a single instance of ZMQActor which handles both topics inside?

@t-bast
Copy link
Member Author

t-bast commented May 11, 2021

I agree that it's a bit strange to implement all topics whereas we will subscribe to a single one, but:

  • There should really be two distinct actors to ensure parallelism and isolation (we really want two distinct tcp sockets)
  • It's better and simpler to have all the code in the same place (instead of separating the logic in two distinct actor types)

If we opened a single socket I would totally agree with you, but given the separate sockets setup I think the existing code makes sense.

@t-bast t-bast merged commit 55b50ec into master May 11, 2021
@t-bast t-bast deleted the zmq-actor-subscriptions branch May 11, 2021 09:24
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.

5 participants