htlcswitch: batch preimage writes/consistency fix by cfromknecht · Pull Request #2501 · lightningnetwork/lnd

cfromknecht · 2019-01-18T01:26:14Z

This PR adds batched writes for witnesses discovered in HTLC forwarding. At the same time, we correct a nuanced consistency issue related to a lack of synchronization with the channel state machine. Forcing the individual preimage writes to be synchronized with the link incurs a heavy performance penalty (about 80% from my measurements). Batching these allows us to minimize the number of db transactions required to write the preimages, allowing us to reinsert the batched write into the link's critical path and resolve the possible inconsistency. In fact, the benchmarks actually showed a slight performance improvement, even with the extra write in the critical path.

See commit messages for more details.

halseth

Very nice fix 👍 🦊

cfromknecht · 2019-02-13T06:43:12Z

@joostjager @halseth ready for another round of review. the changes sprawled quite a bit as you'll see.

I plan to consolidate bc01a41 (the move to lntypes) and a5fc555 (first commit), but wanted to get your feedback on the additional commits before doing so.

Along the way:

lnmock is born, avoiding the same mockPreimageCache implementation in 4 places in the code base.
lntypes is moved to lnhash
channeldb.WitnessCache gained the AddSHA256Witnesses helper, to avoid the extra copied in the preimageBeacon
modfiied the channeldb/witness_cache_test to test batch insertion
added a link-level test to assert that the changes in htlcswitch/link.go behave as expected.

ptal

joostjager

I think having those Hash and Preimage types is proving good (thanks @halseth). I like the strictness it introduces.

cfromknecht · 2019-02-13T19:31:31Z

@joostjager @halseth comments addressed and commits restructured, ptal

cfromknecht · 2019-02-16T01:29:14Z

@joostjager revamped the witness cache to only expose the specific interface, makes everything much cleaner imo, ptal

cfromknecht · 2019-02-16T01:51:12Z

note: planning to squash channeldb+witness_beacon: use sha256 lookup+delete witness (46c6c96) down into channeldb/witness_cache: create AddSHA256Witnesses helper + test (e3766be)

joostjager

Yes, could use some squashing indeed. PR is looking good.

halseth

Most changes look good in general, but I think this PR has sprawled a bit, which can be a bit dangerous when we are dealing with important stuff s.a. preimages 🙊

Would prefer splitting changes unrelated to the original change out 😄

re: squashing commits "bc01a41 (the move to lntypes) and a5fc555 (first commit)", I don't think that is necessary, as it is nice to separate commits that just moves/refactors code, from commits that actually changes behavior.

In this commit, we modify the WitnessCache's AddPreimage method to accept a variadic number of preimages. This enables callers to batch preimage writes in performance critical areas of the codebase, e.g. the htlcswitch. Additionally, we lift the computation of the witnesses' keys outside of the db transaction. This saves us from having to do hashing inside and blocking other callers, and limits extraneous blocking at the call site.

This commit makes use of the batched AddWitness method of the WitnessCache, in order to avoid performing one write for each accepted preimage. Additionally, this fixes an existing hole in the consistency guarantees since the batched writes are now guaranteed to take place before accepting the next CommitSig. Previously, these writes were processed in an unsynchronized go routine that could be delayed arbitrarily long before being executed. With this change, the async_payments_benchmarks actually shows a slight improvement in performance, presumably because we no longer do an individual write per preimage, even though the execution is now explicitly in the critical path. There is likely also a marginal performance improvement from the reduction in goroutine overhead.

cfromknecht · 2019-02-20T01:09:38Z

@halseth @joostjager i took out the commits that removed the preimage cache from lnwallet, should be a little more contained now. it still includes the commits that make the switch to lntypes as requested, ptal

halseth

Thanks, I'm much more comfortable with the current scope of the PR!

Gave it a thorough review, the commit structure made it a breeze. LGTM! 🥇

cfromknecht added htlcswitch bug fix optimization labels Jan 18, 2019

cfromknecht force-pushed the batch-preimage-writes branch 5 times, most recently from ad5f3d5 to d9ad7e6 Compare January 18, 2019 03:16

halseth mentioned this pull request Jan 18, 2019

htlcswitch: reliable HTLC tracking #2265

Closed

2 tasks

Roasbeef requested a review from joostjager February 1, 2019 02:28

joostjager reviewed Feb 1, 2019

View reviewed changes

Comment thread htlcswitch/link.go Outdated

Comment thread htlcswitch/link.go Outdated

joostjager reviewed Feb 4, 2019

View reviewed changes

Comment thread channeldb/witness_cache_test.go Outdated

Comment thread witness_beacon.go Outdated

Comment thread contractcourt/channel_arbitrator.go Outdated

Comment thread htlcswitch/link.go Outdated

Comment thread htlcswitch/link.go Outdated

halseth suggested changes Feb 7, 2019

View reviewed changes

Comment thread channeldb/witness_cache.go Outdated

Comment thread channeldb/witness_cache_test.go Outdated

Comment thread contractcourt/channel_arbitrator.go Outdated

Comment thread contractcourt/htlc_outgoing_contest_resolver.go Outdated

Comment thread witness_beacon.go Outdated

cfromknecht force-pushed the batch-preimage-writes branch 3 times, most recently from 70f2c6d to c173aa0 Compare February 13, 2019 06:40

joostjager reviewed Feb 13, 2019

View reviewed changes

cfromknecht force-pushed the batch-preimage-writes branch 3 times, most recently from 08a4cf0 to f921e0a Compare February 13, 2019 19:27

cfromknecht force-pushed the batch-preimage-writes branch from f921e0a to 5f62111 Compare February 13, 2019 19:32

joostjager reviewed Feb 14, 2019

View reviewed changes

Comment thread contractcourt/htlc_success_resolver.go Outdated

Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated

Comment thread lnwallet/channel.go Outdated

Comment thread channeldb/witness_cache.go Outdated

cfromknecht commented Feb 16, 2019

View reviewed changes

Comment thread lnwallet/channel.go Outdated

cfromknecht force-pushed the batch-preimage-writes branch from 5f62111 to 981eaac Compare February 16, 2019 01:26

cfromknecht added this to the 0.6 milestone Feb 16, 2019

cfromknecht force-pushed the batch-preimage-writes branch 2 times, most recently from e8f02a9 to 73a041a Compare February 16, 2019 01:39

joostjager reviewed Feb 18, 2019

View reviewed changes

Comment thread contractcourt/htlc_outgoing_contest_resolver.go Outdated

Comment thread contractcourt/htlc_success_resolver.go Outdated

Comment thread htlcswitch/link_test.go Outdated

halseth suggested changes Feb 19, 2019

View reviewed changes

cfromknecht force-pushed the batch-preimage-writes branch from 73a041a to d062cbc Compare February 20, 2019 00:06

cfromknecht added 8 commits February 19, 2019 17:05

channeldb/witness_cache_test: test batch preimage insertion

56b6bec

channeldb/witness_cache: create AddSha256Witnesses helper + test

e8b7f1f

lntypes/preimage: add MakePreimage initializer

2d8bc99

cnct+lnwl+hswc: use lntypes.Preimage for witness beacon

29f07a5

htlcswitch/link_test: batch preimage write test

3428fde

channeldb+witness_beacon: use sha256 lookup+delete witness

0a3e1cf

cfromknecht force-pushed the batch-preimage-writes branch from d062cbc to 0a3e1cf Compare February 20, 2019 01:06

halseth approved these changes Feb 20, 2019

View reviewed changes

Comment thread channeldb/witness_cache.go

Comment thread contractcourt/channel_arbitrator.go Outdated

joostjager approved these changes Feb 21, 2019

View reviewed changes

Roasbeef merged commit cbe0bf6 into lightningnetwork:master Feb 22, 2019

cfromknecht deleted the batch-preimage-writes branch February 22, 2019 01:52

Conversation

cfromknecht commented Jan 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfromknecht commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfromknecht commented Feb 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfromknecht commented Feb 16, 2019

Uh oh!

cfromknecht commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfromknecht commented Feb 20, 2019

Uh oh!

halseth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cfromknecht commented Feb 13, 2019 •

edited

Loading

cfromknecht commented Feb 16, 2019 •

edited

Loading