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.

Sign up for GitHub

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

Jump to bottom

Optimize watching for spending txs #1699

Merged
merged 7 commits into from
Feb 24, 2021
Merged

Optimize watching for spending txs #1699

merged 7 commits into from
Feb 24, 2021

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Feb 19, 2021

Since we almost always know which transactions will spend the utxos that we are watching, we can optimize the watcher to look for those instead of starting from scratch.

@codecov-io
Copy link

codecov-io commented Feb 19, 2021 •
edited
Loading

Codecov Report

Merging #1699 (848ac2b) into master (d9c0b86) will increase coverage by 0.09%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master    #1699      +/-   ##
==========================================
+ Coverage   86.08%   86.17%   +0.09%     
==========================================
  Files         151      151              
  Lines       11523    11527       +4     
  Branches      493      500       +7     
==========================================
+ Hits         9919     9933      +14     
+ Misses       1604     1594      -10
Impacted Files Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.75% <80.00%> (-0.03%) ⬇️
...cala/fr/acinq/eclair/blockchain/WatcherTypes.scala 100.00% <100.00%> (ø)
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 99.29% <100.00%> (+0.03%) ⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 77.23% <100.00%> (ø)
...nq/eclair/blockchain/electrum/ElectrumClient.scala 73.52% <0.00%> (-0.37%) ⬇️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.76% <0.00%> (+0.54%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 93.46% <0.00%> (+2.69%) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 78.49% <0.00%> (+3.22%) ⬆️

@pm47
try txs in hints before scanning the blockchain
b7f5ba3 
This is a pretty sensitive part of the code. This implementation first
tries to get the txs provided in hints and verifies that one of them
indeed spend the watched tx (there can only be one because we watch a
single utxo). If (and only if) the spending tx checks out, then we
bypass the resource-intensive blockchain scanning.

Note: it still works if the hints are wrong.
@pm47
Copy link
Member Author

pm47 commented Feb 22, 2021

This is much more efficient that what we currently and will significantly reduce resource usage at startup, but this part of the code is obviously sensitive. I think it is worth it, can you review extra carefully @sstone @t-bast? I've done my best to reduce the possibility of regressions.

@pm47 pm47 marked this pull request as ready for review February 22, 2021 11:38
@pm47 pm47 requested review from t-bast and sstone February 22, 2021 11:38
@sstone
Copy link
Member

sstone commented Feb 22, 2021

LGTM from a functional point of view but we never test that we actually use the provided hints for find a spending txs. What about extracting the relevant methods from ZmqWatcher and adding them to ExtendedBitcoinClient ?

@pm47
Copy link
Member Author

pm47 commented Feb 22, 2021

LGTM from a functional point of view but we never test that we actually use the provided hints for find a spending txs.

Indeed, I verified manually by running the tests with logging enabled. The point of those tests (when run automatically) is to prove non-regression (particularly with a wrong hint), they don't prove the optimization.

What about extracting the relevant methods from ZmqWatcher and adding them to ExtendedBitcoinClient?

I don't know, WDYT @t-bast?

t-bast
t-bast reviewed Feb 22, 2021
View reviewed changes
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.

This is definitely an improvement, whenever we can avoid a call to getMempool let's avoid it!

What about extracting the relevant methods from ZmqWatcher and adding them to ExtendedBitcoinClient?

I don't have a strong opinion here, it could make sense in ExtendedBitcoinClient but if only ZmqWatcher is using it, then it also makes sense to keep it in ZmqWatcher...maybe leave it like that for now and after the re-architecting that will happen around ZmqWatcher we'll see?

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/WatcherTypes.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/WatcherTypes.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala Outdated Show resolved Hide resolved
t-bast
t-bast approved these changes Feb 24, 2021
View reviewed changes
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 🚀

@pm47 pm47 merged commit c1bf9bd into master Feb 24, 2021
@pm47 pm47 deleted the better-watch-spent branch February 24, 2021 13:37
tompro pushed a commit to tompro/eclair that referenced this pull request Mar 7, 2021
@pm47 @tompro
Optimize watching for spending txs (ACINQ#1699)
40ec335 
Since we almost always know which transactions will spend the utxos that we are watching, we can optimize the watcher to look for those instead of starting from scratch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-bast t-bast t-bast approved these changes

@sstone sstone Awaiting requested review from sstone

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pm47 @codecov-io @sstone @t-bast