Fixes locking issues when creating channel / adding funds #2640

dirkmc · 2020-07-28T23:23:43Z

Currently when creating a channel or adding funds to an existing channel, the payment channel manager locks up until the messages have been confirmed on chain, which can take several minutes.

This PR resolves the locking issue:

Operations on each channel are executed sequentially
Operations on different channels can run in parallel

This PR also manages the state of requests and restores them if lotus is shut down and restarted:

On startup checks for pending create channel / add funds messages that have been sent to chain
Writes state to the datastore each time there's a state change
Keeps track of channels by from / to instead of by channel address, so that state can be written even before channel has been created

This PR also

sets a flag on channels that are in the "settling" state
creates a new channel if there is a subsequent call to add funds to that From and To

api/apistruct/struct.go

dirkmc · 2020-08-04T17:35:49Z

node/builder.go

@@ -274,6 +275,7 @@ func Online() Option {

 			Override(new(*paychmgr.Store), paychmgr.NewStore),
 			Override(new(*paychmgr.Manager), paychmgr.NewManager),
+			Override(HandlePaymentChannelManagerKey, paychmgr.HandleManager),


On startup we need to restart tracking of payment channel add funds requests that have already been sent to chain

node/impl/paych/paych.go

paychmgr/manager.go

dirkmc · 2020-08-04T17:48:30Z

paychmgr/paych.go

 }

-func (pm *Manager) OutboundChanTo(from, to address.Address) (address.Address, error) {


OutboundChanTo() wasn't being used anywhere so I removed it

dirkmc · 2020-08-04T17:49:40Z

paychmgr/paych.go

+
+// laneState gets the LaneStates from chain, then applies all vouchers in
+// the data store over the chain state
+func (ca *channelAccessor) laneState(state *paych.State, ch address.Address) (map[uint64]*paych.LaneState, error) {


laneState() and totalRedeemedWithVoucher() were just moved from state.go

dirkmc · 2020-08-04T17:50:01Z

paychmgr/state.go

-
-// laneState gets the LaneStates from chain, then applies all vouchers in
-// the data store over the chain state
-func (pm *Manager) laneState(state *paych.State, ch address.Address) (map[uint64]*paych.LaneState, error) {


laneState() and totalRedeemedWithVoucher() were just moved to paych.go

paychmgr/manager.go

paychmgr/simple.go

magik6k

This is so much better!

Generally looks good, just a couple of comments.

Running go test -count 100 ./paychmgr seems to produce some test failures on my PC -> https://gist.github.com/magik6k/2b09f76c40418a0c7553fc14ded0ae2f

magik6k · 2020-08-05T14:59:40Z

paychmgr/simple.go

-// TODO: wait outside the store lock!
-//  (tricky because we need to setup channel tracking before we know its address)


api/apistruct/struct.go

dirkmc · 2020-08-05T22:00:52Z

@magik6k I was able to repro the issues you were seeing when you ran 100 tests. There was a sync issue in the mocks for mempool / state wait that I believe was causing the issues, I've fixed it now and it works for me.
Separately I noticed a couple of missing locks so I added those as well.

Note that it's ok for the tests to output error logs - some of the tests are designed to trigger error cases and then test that the paych manager handles errors correctly.

Please take another look and let me know if it works for you now.

paychmgr/settle_test.go

paychmgr/simple.go

whyrusleeping

A couple comments, but overall this looks really good, excited to have this in soon!

hannahhoward

I wrote a lot of suggestions and ideas, but generally these are improvements / future tickets rather than blocking issues.

However, the one thing I need to at minimum confirm is your intention (and if so, understand why) is keying the channels on -from->to- -- it seems like we should be using the UUID, cause otherwise we're overwriting channels potentially. I guess if they're not active that's ok, maybe even preferable, but it still feels odd to have the uuid if we're not really using it that much (I guess we're using it for messages)

node/impl/paych/paych.go

paychmgr/manager.go

hannahhoward · 2020-08-05T23:51:45Z

paychmgr/store.go

 }

-func (ps *Store) VouchersForPaych(ch address.Address) ([]*VoucherInfo, error) {
-	ci, err := ps.getChannelInfo(ch)
+// TODO: This is a hack to get around not being able to CBOR marshall a nil


eeek how annoying!

paychmgr/store.go

hannahhoward · 2020-08-06T00:04:24Z

paychmgr/simple.go

+}
+
+// Change the state of the channel in the store
+func (ca *channelAccessor) mutateChannelInfo(from address.Address, to address.Address, mutate func(*ChannelInfo)) {


I think it may be worth looking at go-statestore even if you're not using go-statemachine cause it provides some of this functionality in a simple form. (for the future)

paychmgr/msglistener.go

hannahhoward · 2020-08-06T00:14:15Z

paychmgr/paych.go

 	if err != nil {
-		return types.NewInt(0), err
+		return types.BigInt{}, err


Does Lotus still have a BigInt type of its own? I feel like at this point all our money types should come from spec-actors abi package? Future refactor.

It's an alias of the spec type, we just never got around to getting rid of it

node/impl/paych/paych.go

paychmgr/settle_test.go

paychmgr/simple.go

paychmgr/manager.go

…tead of burning funds)

Update adapter to use wait methods provided by paych adapter

hannahhoward

LGTM all other comments non-blocking

dirkmc changed the base branch from master to next July 28, 2020 23:24

dirkmc force-pushed the fix/store-locking branch 2 times, most recently from a2c541e to 4a6362b Compare July 29, 2020 17:57

mishmosh mentioned this pull request Jul 31, 2020

Retrieval deals fail if running many in parallel #2743

Closed

dirkmc force-pushed the fix/store-locking branch 2 times, most recently from 1e86e83 to b41ace0 Compare August 4, 2020 16:22

dirkmc commented Aug 4, 2020

View reviewed changes

dirkmc mentioned this pull request Aug 4, 2020

PaychGet has poor usability #2295

Open

dirkmc marked this pull request as ready for review August 4, 2020 23:06

dirkmc requested review from hannahhoward and magik6k August 4, 2020 23:07

magik6k reviewed Aug 5, 2020

View reviewed changes

paychmgr/manager.go Outdated Show resolved Hide resolved

magik6k reviewed Aug 5, 2020

View reviewed changes

paychmgr/simple.go Show resolved Hide resolved

magik6k reviewed Aug 5, 2020

View reviewed changes

magik6k added area/payment-channel Area: Payment Channel goal/incentives labels Aug 5, 2020

dirkmc requested a review from magik6k August 5, 2020 22:03

magik6k approved these changes Aug 5, 2020

View reviewed changes

whyrusleeping reviewed Aug 5, 2020

View reviewed changes

paychmgr/settle_test.go Show resolved Hide resolved

whyrusleeping reviewed Aug 5, 2020

View reviewed changes

paychmgr/simple.go Show resolved Hide resolved

whyrusleeping approved these changes Aug 5, 2020

View reviewed changes

hannahhoward requested changes Aug 6, 2020

View reviewed changes

dirkmc commented Aug 6, 2020

View reviewed changes

paychmgr/manager.go Outdated Show resolved Hide resolved

dirkmc added 5 commits August 6, 2020 12:04

WIP: fix payment channel locking

fdfccf0

fix: make DeleteActor(beneficiary) transfer funds to beneficiary (ins…

4abc741

…tead of burning funds)

fix: payment channel integration test

6d70d91

docs: fix comments

c897cc0

fix: lint fixes

aea1b0e

dirkmc and others added 5 commits August 6, 2020 12:04

fix: paych manager dependency injection

45d9ddc

feat(markets): update for new paych logic

b048fcd

Update adapter to use wait methods provided by paych adapter

fix: tighten up testing and add a couple of missing locks

28985c8

fix: race in paych manager test

63bb095

fix: track payment channel by ID instead of from/to

1ef9113

dirkmc force-pushed the fix/store-locking branch from 03a258b to 1ef9113 Compare August 6, 2020 16:08

hannahhoward approved these changes Aug 6, 2020

View reviewed changes

magik6k approved these changes Aug 6, 2020

View reviewed changes

magik6k merged commit d2fc7c8 into next Aug 6, 2020

magik6k deleted the fix/store-locking branch August 6, 2020 16:52

This was referenced Aug 6, 2020

Fix paych sleepy tests #2869

Merged

Panic if there's a datastore err when writing state to datastore after chain msg #2870

Closed

This was referenced Aug 8, 2020

Payment Channel Tech Debt Clean-up #2911

Closed

Payment Channel Manager can combine funding operations #2912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes locking issues when creating channel / adding funds #2640

Fixes locking issues when creating channel / adding funds #2640

dirkmc commented Jul 28, 2020 •

edited

dirkmc Aug 4, 2020

dirkmc Aug 4, 2020

dirkmc Aug 4, 2020

dirkmc Aug 4, 2020

magik6k left a comment

magik6k Aug 5, 2020

dirkmc commented Aug 5, 2020

whyrusleeping left a comment

hannahhoward left a comment

hannahhoward Aug 5, 2020

hannahhoward Aug 6, 2020

hannahhoward Aug 6, 2020

magik6k Aug 6, 2020

hannahhoward left a comment

		}

		func (pm *Manager) OutboundChanTo(from, to address.Address) (address.Address, error) {

		// TODO: wait outside the store lock!
		// (tricky because we need to setup channel tracking before we know its address)

Fixes locking issues when creating channel / adding funds #2640

Fixes locking issues when creating channel / adding funds #2640

Conversation

dirkmc commented Jul 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc commented Aug 5, 2020

whyrusleeping left a comment

Choose a reason for hiding this comment

hannahhoward left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhoward left a comment

Choose a reason for hiding this comment

dirkmc commented Jul 28, 2020 •

edited