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.

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

feat: paychmgr: Support paych funding (a.k.a. fast paid retrieval) #7883

Merged
merged 30 commits into from
Mar 2, 2022

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jan 4, 2022

Closes #7876

@@ -492,6 +567,8 @@ func (ca *channelAccessor) addFunds(ctx context.Context, channelInfo *ChannelInf
return &mcid, nil
}

// TODO func (ca *channelAccessor) freeFunds(ctx context.Context, channelInfo *ChannelInfo, amt, avail types.BigInt) (*cid.Cid, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want before landing, needed for stuff to work well with how we fund partial retrievals (which we fund like a full retrieval)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((apparently we don't have good market hooks for this; can be done later))

@magik6k magik6k changed the title feat: paychmgr: Support paych funding feat: paychmgr: Support paych funding (a.k.a. fast paid retrieval) Jan 5, 2022
@magik6k magik6k marked this pull request as ready for review January 5, 2022 15:22
@magik6k magik6k requested a review from a team as a code owner January 5, 2022 15:22
@magik6k magik6k requested a review from dirkmc January 5, 2022 15:22
@magik6k magik6k added area/payment-channel Area: Payment Channel kind/enhancement Kind: Enhancement labels Jan 5, 2022
@magik6k magik6k force-pushed the feat/paych-avail-reuse branch 3 times, most recently from 28de5d1 to 103bc53 Compare January 10, 2022 13:25
@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #7883 (e9a6f5f) into master (a74c466) will increase coverage by 0.12%.
The diff coverage is 84.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7883      +/-   ##
==========================================
+ Coverage   40.28%   40.40%   +0.12%     
==========================================
  Files         666      666              
  Lines       72756    72887     +131     
==========================================
+ Hits        29309    29452     +143     
+ Misses      38310    38293      -17     
- Partials     5137     5142       +5     
Impacted Files Coverage Δ
api/api_full.go 57.89% <ø> (ø)
api/v0api/v1_wrapper.go 14.04% <0.00%> (-0.16%) ⬇️
paychmgr/store.go 70.35% <57.14%> (-2.42%) ⬇️
markets/retrievaladapter/client.go 25.45% <60.00%> (+4.62%) ⬆️
cli/paych.go 49.36% <70.58%> (-0.31%) ⬇️
node/impl/paych/paych.go 57.77% <82.35%> (+5.14%) ⬆️
node/modules/client.go 75.72% <88.23%> (+0.48%) ⬆️
paychmgr/simple.go 83.16% <95.78%> (+2.92%) ⬆️
node/builder_chain.go 100.00% <100.00%> (ø)
paychmgr/manager.go 69.87% <100.00%> (+0.74%) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a74c466...e9a6f5f. Read the comment docs.

Copy link
Contributor

@hannahhoward hannahhoward 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 awesome and much needed.

Various suggestions and changes, though I'm not sure whether any of them are truly blocking.

I do think it would be good possibly to incorporate the markets change.

markets/retrievaladapter/client.go Outdated Show resolved Hide resolved
api/api_full.go Show resolved Hide resolved
channelInfo.CreateMsg = nil
})

return nil
}

// completeAvailable fills reserving fund requests using already available funds, without interacting with the chain
func (ca *channelAccessor) completeAvailable(ctx context.Context, merged *mergedFundsReq, channelInfo *ChannelInfo, amt, av types.BigInt) (*paychFundsRes, types.BigInt) {
toReserve := types.BigSub(amt, av)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I read this code, if we have available funds already confirmed, but we also have a no-reserve request ready to go that could supply the available funds, we don't use the available funds. I'm curious about the rationale here. Why not just grab the available funds and then let the no-reserve request complete on its own time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to completeAmount below should be doing this.

return ps.findChan(ctx, func(ci *ChannelInfo) bool {
if ci.Direction != DirOutbound {
return false
}
if ci.Settling {
return false
}

if ci.Channel != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just an unrelated bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would break your paychmgr after the channel got settled on-chain

api/api_full.go Outdated Show resolved Hide resolved
documentation/en/default-lotus-config.toml Outdated Show resolved Hide resolved
node/config/types.go Outdated Show resolved Hide resolved
api/api_full.go Show resolved Hide resolved
failed = types.BigAdd(failed, r.amt)
r.onComplete(&paychFundsRes{
channel: *channelInfo.Channel,
err: xerrors.Errorf("not enough funds available in the payment channel %s; add funds with 'lotus paych add-funds %s %s %s'", channelInfo.Channel, channelInfo.from(), channelInfo.to(), types.FIL(r.amt).Unitless()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest simplifying this error message - the message only makes sense if people are calling the API from the command line

paychmgr/simple.go Outdated Show resolved Hide resolved
paychmgr/store.go Outdated Show resolved Hide resolved
api/api_full.go Outdated Show resolved Hide resolved
@Angelo-gh3990
Copy link

I get : ERROR: Remote API version didn't match (expected 1.5.0, remote 1.4.0) when I start lotus-miner when I have compiled lotus with this version as well. Even tried compiling Lotus with the master branch. Anyway to get this to work, testing paid retrieval with Elijah. Thanks

@magik6k
Copy link
Contributor Author

magik6k commented Feb 16, 2022

Anyway to get this to work, testing paid retrieval with Elijah

Note that this is client-side only change. You don't need to update anything on the miner side to get fast paid retrievals to work.

Copy link
Contributor

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- this hasn't changed much since i last looked but the changes look good

@magik6k magik6k enabled auto-merge March 2, 2022 12:57
@magik6k magik6k merged commit b8473ae into master Mar 2, 2022
@magik6k magik6k deleted the feat/paych-avail-reuse branch March 2, 2022 13:28
@jennijuju jennijuju mentioned this pull request Mar 7, 2022
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/payment-channel Area: Payment Channel kind/enhancement Kind: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retrieval should use available funds in existing payment channels
4 participants