feat: Reject deceptive contracts #88
Conversation
benbierens
requested review from
dryajov and
markspanbroek
and removed request for
markspanbroek
markspanbroek
left a comment
markspanbroek
left a comment
There was a problem hiding this comment.
Thanks for the fix @benbierens! I left a few comments.
| type StoreSlotAsk* = object | ||
| cid*: Cid | ||
| slotIndex*: uint64 | ||
| slotSize*: uint64 | ||
| expiry*: SecondsSince1970 | ||
| repair*: bool | ||
|
|
||
| method available*(storage: StorageInterface): uint64 {.base, gcsafe, raises: [].} = | ||
| raiseAssert "not implemented" | ||
|
|
||
| method storeSlot*( | ||
| storage: StorageInterface, | ||
| cid: Cid, | ||
| slotIndex: uint64, | ||
| expiry: SecondsSince1970, | ||
| repair: bool, | ||
| storage: StorageInterface, storeAsk: StoreSlotAsk |
There was a problem hiding this comment.
I understand why you extracted a parameter object here, the amount of parameters is quite a lot. But for me, this doesn't make the code better to read. It immediately raises questions: what is a StoreSlotAsk, what's the difference with a StorageAsk, why is a repair boolean in the same object as slot properties?
There was a problem hiding this comment.
I am not too fond of the name 'StoreSlotAsk', but I think this object should exist. These parameters travel together through several signatures. They represent a concept... something like: "Everything you need to store this slot". But it's a node-type, independent of the smartcontract types.
It's also nice to know that this object is constructed in one place only, so you know where the data comes from.
There was a problem hiding this comment.
Yes, these are all good reasons to introduce a parameter object, however the fact that we can't think of a good name indicates to me that this is not a good abstraction. And then I'd rather choose no abstraction over having a bad one, especially in an important interface like this.
There was a problem hiding this comment.
Maybe the object isn't a good abstraction. Maybe it is and we can't see it yet. I have a feeling in the future some other changes might reveal that it makes sense, and we'll know what it's called at that time. In the meantime, just for the sake of code cleanliness, I vote in favor of this object. But, you're the boss. If you want I'll unpack it. Plz let me know, then this can get merged and I have another green release test.
There was a problem hiding this comment.
Yes, if we can come up with a good name for this in the future then we can introduce it. But for now, I'd like to keep the parameters as they were.
|
|
||
| type SalesData* = ref object | ||
| requestId*: RequestId | ||
| ask*: StorageAsk |
There was a problem hiding this comment.
I would like to ask you (pun intended) to keep this property in. I know it's currently not used, but it should be. One of the first things we do in the sales state machine is get the entire request, but we can already do a lot with just the ask, which we get for free with the StorageRequested event.
There was a problem hiding this comment.
This property is never assigned to, nor is it used. Its only purpose seems to be, to confuse and annoy me while I was implementing this fix. That purpose is now fulfilled, so let's remove it.
In the future, I think we should definitely clean up the optional-request field as you suggest. At that time, something like this can be re-introduced. Until that time, let's not accidentally waste anyone's time on fields that may some day be used.
| repair: false, | ||
| ) | ||
| let response = await storage.storeSlot(ask) | ||
| check response.isFailure |
There was a problem hiding this comment.
You might want to check the error message as well. I've seen tests deteriorate over time because they should be checking for a certain error, but accidentally keep succeeding because there's another error being returned.
From the Deceptive-contracts tests in the Archivist release test repo (https://github.com/durability-labs/cs-archivist-dist-tests/blob/main/Tests/ArchivistReleaseTests/MarketTests/DeceptiveContractTest.cs) we have the following behavior:
Storage requests posted on-chain contain a slotSize number (representing bytes in a single slot).
This is used by hosts to determine if they have capacity for the slot.
This is used by the marketplace SmartContract to determine the price payout to hosts.
The on-chain request also contains a CID (representing a verifiable-manifest).
A verifiable manifest also defines a slotSize in bytes.
In the current implementation of the node, the slotSize posted on-chain and the slotSize of the veriable manifest are allowed to mismatch. This can be exploited easily. Posting a contract on-chain with a small slotSize, but providing a CID with a large slotSize can trick a host into storing a large slot while receiving payment for a small one. Additionally, it can screw with a host's quota management.
This PR fixes the issue by:
As soon as a storage request is picked up and the manifest is fetched, we check that the manifest slotSize matches the storageRequest slotSize. If it does not, we do not proceed to download and fail out of the sales statemachine.
Sidenote:
It's possible to over-report your slotSize on-chain. It would be of benefit to hosts to pick up these contracts. You get paid more for doing less work! But, this PR expects the storage request to be honest: The numbers must match exactly.