-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Payment settlement in PDP [DO NOT MERGE] #743
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
base: pdpv0
Are you sure you want to change the base?
Conversation
3f33e68 to
d36d961
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About halfway through. Settlement watcher looks pretty good. Next up I need to understand what exactly is going on with the terminate service watcher and how its distinct from dataset deletion watcher.
| ) | ||
|
|
||
| const PaymentContractMainnet = "" | ||
| const PaymentContractCalibnet = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x09a0fDc2723fAd1A7b8e3e00eE5DF73841df55a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI getting these directlty from FWSS contract:
cast call --rpc-url http://api.calibration.node.glif.io/rpc/v1 0x02925630df557F957f70E112bA06e50965417CA0 "paymentsContractAddress()"| "github.com/filecoin-project/curio/build" | ||
| ) | ||
|
|
||
| const PaymentContractMainnet = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x2321fB2A346E3dA71f9310FB477384fF727924ba
| "github.com/filecoin-project/curio/build" | ||
| ) | ||
|
|
||
| const MultiCallAddressMainnet = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if multicall is really necessary for whatever we're doing here?
It ends up useful in client facing applications where latency matters a lot but this background settlement process should not be latency sensitive.
What's the main motivation? Gas savings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gas saving + reducing the number of messages. Each settlement requires its own message. We don't have settlerails() func.
| transactionsToSend := make(map[*types.Transaction][]int64) | ||
|
|
||
| for _, id := range toSettle { | ||
| data, err := pabi.Pack("settleRail", id, big.NewInt(int64(current))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against it necessarily but I'd like to understand why we're doing this. I'd be surprised if this helps much with gas since multicall afaik iterates through a bunch of sends and sends them one by one to the contract. So we're not getting any batching locality in the payments contract. And we're not saving much on message overhead since we need to specify all call parameters. I don't think latency matters very much or that one big multicall makes mpool resource use all that better.
The cost is that we have yet another contract dependency. For example all devnet testing will need to deploy a new multicall. And if something goes wrong with our multicall contract we need to be on top of switching to a new one.
Something @rvagg noticed is that we could have used multicall to make our lives easier with the pdp verifier create and add call so this could have helped there. But that ship has sailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gas saving + reducing the number of messages. Each settlement requires its own message. We don't have settlerails() func. Multicall was suggested by @wjmelements as a workaround for now. IIRC, @rvagg also ran into gas issues with settling, I am not sure if Multicall with help or just make things worse for that.
My PoV - I want to send least amount of message to settle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The as issues @rvagg ran into with settling were essentially contract logic bugs which we know how to fix (and hopefully did?) These are orthogonal to this issue.
My understanding of the gas model as I outline above makes me suspect that multicall savings are very small. I won't hold up this change but just FYI we are adding medium complexity and a serious dependency for an amount of gas savings that I don't think we've measured and I don't think is very big. It feels like a mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah no argument on number of messages. If there are good reasons to reduce those then this is a no brainer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gas issue should be resolved by two optimizations that landed about two weeks ago.
This mostly comes down to some constraints in settle. When a service it terminated, we set endEpoch. We can get paid till endEpoch if we provide service. Below is the Gist. DataSet deletion: (SP side deletion)
I wanted to terminate the DataSet immediately after terminating the service but unfortunately, lockup settle seems weird enough to not allow me a clean exit.
This different task doing different things. I still need to figure out how to do 5 reliably. Dealing directly with payment contract bring in too many constraints and edge cases. This is the reason why I asked why doesn't operator do this cleanly? I am open to less convoluted ideas if you can think of any. |
Pending Decisions
rmads