golemfactory / concent

Repository for Concent Service sources
8 stars 7 forks source link

Concent: Accepting additional verification requests from provider #201

Closed cameel closed 6 years ago

cameel commented 6 years ago

Sequence of operation

Implement the following part of the #193 blueprint:

Detecting duplicate requests

Check subtask state: 1) Subtask state is VERIFICATION RESULT TRANSFER or ADDITIONAL VERIFICATION: this is a duplicate request. Respond with ServiceRefused. 2) Subtask state is ACCEPTED or FAILED: it might or might not be a duplicate request. Does not matter because we don't allow additional verification in that state. Respond the same way as in any other state that does not allow it.

In case 3) you need more information than just the state. It should be possible to infer it based on what we have in the Subtask table but if there are problems with that we'll consider adding extra info to the table.

Setup for local development

You can assume that Celery is already configured (#197).

For local development you'll need to run a RabbitMQ server. You can run it in a docker container with a single command. See https://github.com/golemfactory/concent/issues/197#issuecomment-375620295. It should work out of the box because settings/development.py is configured to connect to a RabbitMQ server on default port running on localhost.

You don't need a running worker for this part of the implementation.

cameel commented 6 years ago

Update: model definitions moved to #193.

dybi commented 6 years ago

@cameel, as agreed, please elaborate a bit more on

SubtaskResultsRejected is invalid or was not issued by the requestor, but not due to negative verification on his part (ServiceRefused)

to make it more precise. Thanks!

dybi commented 6 years ago

@cameel,

Requestor does not have enough funds to pay for the task in case of positive verification (InsufficientFunds)

what about Provider's funds (in case of negative verification)? Should this be checked as well?

cameel commented 6 years ago

what about Provider's funds (in case of negative verification)? Should this be checked as well?

No, because there is no forced payment from provider's deposit in this use case. Provider does not even have to have a deposit.

The only thing that the provider has to pay for is the communication fee, but fee payments will be implemented after the mainnet release. They're also going to use a different payment mechanism (some kind of a payment channel to make nanopayments economically feasible).

cameel commented 6 years ago

@dybi I have added more details about the synchonous responses.

Since SubtaskResultsRejected is nested in another message, most of the cases I listed won't fail in api_view and you can report ServiceRefused but I added a remark that it's fine to handle the errors that happen outside of your code by returning HTTP 400.

dybi commented 6 years ago

I had asked Lukasz on rocket about this InsufficientFunds vs TooSmallRequestorDeposit, but since he didn't answer and since it InsufficientFunds is not implemented - I will change description. Is it ok for you, @cameel ?

cameel commented 6 years ago

OK.

cameel commented 6 years ago

Update: name and content of verification_request has changed in the blueprint.

cameel commented 6 years ago

Update: added clarification about detecting duplicate requests.

dybi commented 6 years ago

@cameel ,

Respond the same way as in any other state that does not allow it.

and what that would be? ServiceRefused (InvalidRequest)? Http400?

cameel commented 6 years ago

Updated the part about duplicate requests. As pointed out by @dybi, it was wrong. That's because while writing it I have mistakenly assumed that requestor's rejection puts the task in the FAILED state, which is not true (it puts it in the REJECTED state).

Respond the same way as in any other state that does not allow it. and what that would be? ServiceRefused (InvalidRequest)? Http400?

Prefer ServiceRefused if there's an appropriate reason for the situation. Use Http400 for stuff that's not in the protocol or is specific to HTTP and would not have an equivalent in a P2P protocol.

Anyway, client should be able to handle both.