Finalize integration of Adjudicator to new Worker/Staker model#1093
Finalize integration of Adjudicator to new Worker/Staker model#1093KPrasch merged 27 commits intonucypher:alhambra-verdefrom
Conversation
Codecov Report
@@ Coverage Diff @@
## alhambra-verde #1093 +/- ##
=================================================
- Coverage 82.93% 80.44% -2.5%
=================================================
Files 69 69
Lines 9109 9179 +70
=================================================
- Hits 7555 7384 -171
- Misses 1554 1795 +241
Continue to review full report at Codecov.
|
|
Do I understand that we are having to roll out own EIP-191 logic in solidity? This seems counter-intuitive to me. For it not to have landed in python yet is to be expected, but why are we going down a path of maintaining a persistent script for an accepted EIP? |
|
@jMyles We currently are using the geth's The on-chain verification of EIP191 signatures is necessary since we have to verify these signatures in the Adjudicator contract. I must say that I don't necessarily think we should use version 0 signatures, since specifying the validator's address may prove to be complex in our setting, where contracts are upgradable. So perhaps version E signatures is enough. |
| emit CFragEvaluated(evaluationHash, msg.sender, cFragIsCorrect); | ||
| if (cFragIsCorrect) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If we remove this than may be such situation: investigator has correct CFrag but broken signatures and as a result - failed tx, and gas wasting (and investigator can do it again). From contract perspective we have two steps - CFrag correctness and evidence correctness. So if CFrag is correct -> there is no such thing like evedence of incorrect CFrag and next checking of signatures is not neccesery (from blockchain perspective)
There was a problem hiding this comment.
This check is not removed, just postponed until we check the signatures too (see line 166). I don't think that, in case of incorrect supporting evidence for the evaluation (e.g. bad signatures), we should (1) mark the CFrag as evaluated, and (2) emit an evaluation event, considering that the investigator already checks this off-chain before submitting (assuming she uses our nucypher code).
There was a problem hiding this comment.
Yeah, but if signatures are wrong than investigator will get just failed tx and CFrag evaluation will not be saved in storage. Checking this signatures don't change any step in algorithm if CFrag is correct: if cignatures are ok -> just check signatures, check balance of staker (require(stakerValue > 0, "Staker has no tokens");`) and do nothing - no any changes in storage state (except CFrag evaluation). if cignatures are broken -> same checks and failed tx. Why need fail tx in that situation?
Also from contract we can't be sure in anything outside of blockchain and can't be sure that investigator did everything properly.
There was a problem hiding this comment.
Before my changes, if CFrag was correct but signatures were wrong there was change in the state of the contract. This doesn't make sense, since it's not valid data. It may even be the case that the CFrag isn't coming from an Ursula: anyone can generate valid CFrags (e.g., using pyUmbral) with invalid signatures (since valid signatures involve participation of a real Ursula of the NuCypher network). Again, unless there's valid signatures, which imply real participation of an Ursula, we shouldn't change state.
There was a problem hiding this comment.
Can't agree that don't need to save correct CFrag when investigators sent wrong signatures: because investigator already paid and data are still valid for contract state. But I don't want be a blocker with that so I'm ok with your point
nucypher/blockchain/eth/sol/source/contracts/lib/SignatureVerifier.sol
Outdated
Show resolved
Hide resolved
nucypher/blockchain/eth/sol/source/contracts/lib/SignatureVerifier.sol
Outdated
Show resolved
Hide resolved
nucypher/blockchain/eth/actors.py
Outdated
| return wrapped | ||
|
|
||
|
|
||
| def cache_transaction(actor_method): |
There was a problem hiding this comment.
👍 Very cool - What about the name @save_receipt?
There was a problem hiding this comment.
Good idea. For consistency, then self._transaction_cache should change to self._saved_receipts. Is that OK?
There was a problem hiding this comment.
True, those are not "transactions" (which one could cache for a rebroadcast, for example), but indeed receipts. Where do we use those saved receipts and when do we clean them?
There was a problem hiding this comment.
Currently, they are not used, and they are not persisted anywhere, so they are cleaned once the actor object disappears. But I can imagine how they can be used to improve UX (e.g, in the CLI or some other GUI-based client).
| adjudicator_deployer.deploy(secret_hash=os.urandom(DispatcherDeployer.DISPATCHER_SECRET_LENGTH)) | ||
| adjudicator_agent = adjudicator_deployer.make_agent() # 4 | ||
|
|
||
| # TODO: Perhaps we should get rid of returning these agents here. |
There was a problem hiding this comment.
Totally agree here.
In my local tree I have started doing staking_agent = Agency.get_agent(StakingEscrowAgent).
What do you think?
There was a problem hiding this comment.
I'm just doing StakingEscrowAgent(). Shouldn't that return the singleton?
Not really an EIP191-compliant method, as it only supports version E, but this is what we currently use in nucypher
…-> self._saved_receipts
…le_staker fixture
| return period | ||
|
|
||
| @only_me | ||
| @save_receipt |
This PR completes the integration of Adjudicator to the worker/staker separation. It closes the loop initiated in #259 with the format for re-encryption metadata, by integrating the worker's identity evidence from the creation of WorkOrder to the slashing of the staker in the Adjudicator contract, and everything in between.
It also tackles #931, by creating the agent and actor for
Adjudicator, namely,AdjudicatorAgentandInvestigatorIn principle, it should be the last PR of
alhambra-verdebefore considering it for review.