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

RPCCost (RHPv2) #123

Merged
merged 3 commits into from
Oct 4, 2023
Merged

RPCCost (RHPv2) #123

merged 3 commits into from
Oct 4, 2023

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented Aug 28, 2023

This PR adds the rpcCost type from hostd. We can move it over to core and get rid of some of the very rough estimates for proof sizes and replace them by exact values we get from DiffProofSize.

@peterjan peterjan self-assigned this Aug 28, 2023
@peterjan peterjan closed this Aug 28, 2023
@peterjan peterjan deleted the pj/diff-proof-size branch August 28, 2023 13:16
@peterjan peterjan restored the pj/diff-proof-size branch August 28, 2023 13:19
@peterjan peterjan reopened this Aug 28, 2023
@lukechampine
Copy link
Member

I'm not entirely sure why the RHP stuff in v2 is different. In any case, the whole v2 package is gone in its-happening -- I decided to take a different approach.

I think my preference is:

  • Implement exact cost calculations in core now (for use in RHPv2/RHPv3)
  • Use fixed costs in RHPv4

@peterjan peterjan force-pushed the pj/diff-proof-size branch from 58fffd9 to 5cad92d Compare August 29, 2023 12:20
@peterjan
Copy link
Member Author

Yeah I saw you removed v2 in your branch, that is why I thought you no longer wanted to use DiffProofSize and pass in the exact RPCWriteActions to the functions calculating the cost of the RPC. I pushed a small commit that reverts my changes to the v2 package and changes the function signatures a bit.

However, since you mention we ideally go for exact cost calculations. Would you prefer changing some of those functions entirely to accept []RPCWriteAction and implement DiffProofSize as you did in the v2 package? I might go ahead and try to implement that already to see the difference.

@peterjan peterjan changed the title DiffProofSize RPC Cost Calculators Aug 29, 2023
@peterjan peterjan force-pushed the pj/diff-proof-size branch from b7b4c57 to 21b5bcd Compare August 30, 2023 10:19
@peterjan peterjan marked this pull request as ready for review August 30, 2023 10:21
@peterjan
Copy link
Member Author

For rhpv3 we could/should update the cost methods on the HostPriceTable to take an argument that indicates whether a proof is required and then also all necessary information to calculate the proof size. Is that something we want to add as well?

@peterjan peterjan changed the title RPC Cost Calculators RPC Cost Calculators (RHPv2) Aug 30, 2023
@peterjan peterjan force-pushed the pj/diff-proof-size branch from 21b5bcd to 205a0ff Compare October 3, 2023 08:00
@peterjan peterjan changed the title RPC Cost Calculators (RHPv2) RPC Cost (RHPv2) Oct 3, 2023
@peterjan peterjan changed the title RPC Cost (RHPv2) RPCCost (RHPv2) Oct 3, 2023
@peterjan peterjan force-pushed the pj/diff-proof-size branch from 205a0ff to 66c5800 Compare October 3, 2023 08:25
Add(settings.UploadBandwidthPrice.Mul64(SectorSize)).
Add(settings.DownloadBandwidthPrice.Mul64(128 * 32)) // proof
collateral = settings.Collateral.Mul64(SectorSize).Mul64(storageDuration)
// add some leeway to reduce chance of host rejecting
Copy link
Member Author

Choose a reason for hiding this comment

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

@n8maninger hostd stopped doing it so I copied that behaviour, just wanted to draw attention to it to ensure it's safe to do so

return settings.BaseRPCPrice.
Add(settings.DownloadBandwidthPrice.Mul64(n * 32)). // roots
Add(settings.DownloadBandwidthPrice.Mul64(128 * 32)) // proof
type RPCCost struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Functionally this type matches the ResourceCost in RHPv3, we could move this into the types package maybe and use the one type. I didn't want to do that yet mostly because of the documentation in ResourceCost which is tailored to the MDM... Maybe it's fine to just have both, for the same reason hostd had its own internal rpcCost type.

@peterjan peterjan requested review from n8maninger and removed request for ChrisSchinnerl October 3, 2023 08:38
rhp/v2/rhp.go Outdated Show resolved Hide resolved
rhp/v2/rhp.go Outdated
case sec.Length == 0:
return RPCCost{}, errors.New("length cannot be zero")
case proof && (sec.Offset%LeafSize != 0 || sec.Length%LeafSize != 0):
return RPCCost{}, errors.New("offset and length must be multiples of SegmentSize when requesting a Merkle proof")
Copy link
Member

Choose a reason for hiding this comment

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

*LeafSize

@lukechampine lukechampine merged commit 69313ce into master Oct 4, 2023
6 checks passed
@peterjan peterjan deleted the pj/diff-proof-size branch October 9, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants