Reduce mem and cpu usage of onchaind for long-lived channels #4250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZmnSCPxj
reviewed
Dec 2, 2020
rustyrussell
force-pushed
the
guilt/onchaind-optimize
branch
from
22d49e1
to
f120be3
Compare
cdecker
approved these changes
Dec 4, 2020
Comment on lines
+155
to
+157
| /* FIXME: This doesn't work for elements? */ | ||
| if (chainparams->is_elements) | ||
| return; |
There was a problem hiding this comment.
Good question, I don't think it should be too much of an issue, given that we currently only have one main-asset input, which also makes the below if (!amount_asset_is_main(&amt)) always false, but better keep it in in case we eventually add multi-input and multi-asset channels.
Was there a concrete error that was triggered on elements, or is this more of a precaution?
There was a problem hiding this comment.
Yes, we had a -1ULL satoshis value. Then added this test and this function caused an onchaind failure (presumably by giving the wrong result).
niftynei
approved these changes
Dec 5, 2020
Under some circumstances, valgrind complains. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, this made valgrind OOM, and chewed much CPU. I dumped the input and output into a file to allow easy replay. This will break as soon as we change onchaind's wire format, but it will serve its purpose until then! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We assert() this in onchaind while grinding fees; better to free newtx. Before this we hit 530MB, after a mere 2.5MB. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: onchaind uses much less memory on unilateral closes for old channels.
We try signatures to see which HTLC (we can have many) is the right one; we can trivially match htlcs against commitment tx outputs, but the CTLV can vary, and that's inside the htlc tx itself. By sorting them, it's easy to skip comparing duplicates: Time before: 2m32.547s Time after: 1m6.984s Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The commitment tx uses the same feerate as the HTLC txs (which we have to grind to find), but we can't use it directly since the fee could be increased by the presence of dust HTLCs. We can still use it to cap the maximum though. Time before: 1m6.984s Time after: 0m15.794s Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: onchaind is much faster when unilaterally closing old channels.
rustyrussell
force-pushed
the
guilt/onchaind-optimize
branch
from
40931f1
to
ea4d6dc
Compare
|
Trivial rebase, plus changelog additions. |
|
ACK ea4d6dc |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Noticed my node's onchaind was chewing memory and CPU (I run under valgrind!), caught the IO from onchaind and created this replay test. It will break as soon as we change onchaind's wire format, but it's a good test for now.