Implement local ritual verification #232
Conversation
Codecov Report
@@ Coverage Diff @@
## tdec-epic #232 +/- ##
=============================================
- Coverage 83.24% 82.17% -1.08%
=============================================
Files 37 37
Lines 979 1004 +25
Branches 123 126 +3
=============================================
+ Hits 815 825 +10
- Misses 158 173 +15
Partials 6 6
|
piotr-roslaniec
force-pushed
the
local-verification
branch
from
b5485fa
to
c982702
Compare
piotr-roslaniec
force-pushed
the
local-verification
branch
from
4581cec
to
c6b1070
Compare
piotr-roslaniec
requested review from
KPrasch,
theref,
derekpierre and
manumonti
piotr-roslaniec
changed the title
lynx
src/characters/cbd-recipient.ts
Outdated
| ); | ||
| } | ||
|
|
||
| const isLocallyVerified = await DkgClient.verifyRitual( |
There was a problem hiding this comment.
I'm not sure whether this needs to be done every time retrieve is called. Perhaps this should be optional i.e. provided as a parameter to retrieve(...). It's unclear to me whether the default value should be True or False (cc @cygnusv ).
There was a problem hiding this comment.
I think true is a sensible default since the security of the user depends on local verification.
There was a problem hiding this comment.
Maybe. Technically if the ritual made it to completion, then the validation was crowd-sourced to all of the nodes in the cohort and was fine..., no? This is for the unlikely case that the entire cohort was malicious - since any one non-malicious Ursula would have prevented the DKG ritual from completing.
There was a problem hiding this comment.
We can only know if the participants contributed the correct transcripts once we perform the verification. I don't think there is any other mechanism in place at the moment. For example, how would a non-malicious Ursula prevent the ritual from completing? I don't see this implemented in the Coordinator contract.
Notice how without verification it's also possible for malicious Ursulas to shut down any ritual without any accountability, etc.
I think in order to address this we need to discuss the scope of configuration sets ("packages") we want to ship and then backtrack how rituals are supposed to work on the client side from there. In principle, for every ritual (or for every kind of ritual) local verification has to be performed at least once, at any point in time, before consuming the ritual. That should apply to every usage scenario, so perhaps this is something we could start with.
There was a problem hiding this comment.
Would moving the ritual verification step to CbdTDecDecrypter.create would be a good temporary solution?
There was a problem hiding this comment.
For example, how would a non-malicious Ursula prevent the ritual from completing? I don't see this implemented in the Coordinator contract.
This is done by the honest Ursula providing a correct aggregated transcript that does match the others (from malicious nodes) - https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/coordination/Coordinator.sol#L228. This is why currently the protocol only needs one honest Ursula out of the n nodes in the cohort. Subsequently, if all aggregated transcripts match, then ritual state is then set to finalized. Knowing that the state is finalized, means that either all nodes were malicious (probabilistically unlikely for larger-sized cohorts though not 0%) or that everything was good/correct.
To be clear, local verification is good as an option 👍 , I don't know/am unsure that we want it to be done every time which is the current set-up; should someone be explicit (default parameter is False and they have to specifically set it to True) about checking it versus being implicit (default to True and they have to specifically set it to False). In the long term, local verification would not be used at all because the protocol would be even more robust in the future (slashing, proofs, on-chain verification etc.), so having the default be False and adopters having to change to True could make sense - it's just something we should think about.... what do others think @cygnusv , @KPrasch ?
Side question: how long does the local verification take?
Would moving the ritual verification step to CbdTDecDecrypter.create would be a good temporary solution?
Interesting. That would seem to be a better spot since the verification would only be done once per encrypter, and is more hidden from the user. That being said, I still have questions about doing local verification in general, but it would definitely be an improvement, and seemingly more hidden from the user. Nice suggestion.
There was a problem hiding this comment.
This is done by the honest Ursula providing a correct aggregated transcript
Thanks for explaining that, I just started working on Coordinator changes and didn't have a clear picture there.
Side question: how long does the local verification take?
The complexity ceiling is a single BLS12-381 pairing, so an order of tens of milliseconds.
src/dkg.ts
Outdated
| public static getParticipantPublicKey = (address: string) => { | ||
| // TODO: Without Validator public key in Coordinator, we cannot verify the | ||
| // transcript. We need to add it to the Coordinator (nucypher-contracts #77). | ||
| const participantPublicKeys: Record<string, FerveoPublicKey> = { | ||
| '0x210eeAC07542F815ebB6FD6689637D8cA2689392': FerveoPublicKey.fromBytes( | ||
| fromHexString( | ||
| '6000000000000000ace9d7567b26dafc512b2303cfdaa872850c62b100078ddeaabf8408c7308b3a43dfeb88375c21ef63230fb4008ce7e908764463c6765e556f9b03009eb1757d179eaa26bf875332807cc070d62a385ed2e66e09f4f4766451da12779a09036e' | ||
| ) | ||
| ), | ||
| '0xb15d5A4e2be34f4bE154A1b08a94Ab920FfD8A41': FerveoPublicKey.fromBytes( | ||
| fromHexString( | ||
| '60000000000000008b373fdb6b43e9dca028bd603c2bf90f0e008ec83ff217a8d7bc006b585570e6ab1ce761bad0e21c1aed1363286145f61134ed0ab53f4ebaa05036396c57f6e587f33d49667c1003cd03b71ad651b09dd4791bc631eaef93f1b313bbee7bd63a' | ||
| ) | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Can we do this in a test with mocks etc. instead of having hardcoded values in actual code and subsequently going stale?
There was a problem hiding this comment.
Until we fix nucypher/nucypher-contracts#77 we can:
- Expose this as a parameter
- Fetch it from Ursulas
- Hardcode it
Either way, this is going to be superseded by changes in the contract and shouldn't remain in the source code for too long. We can also put a pause on this PR and update it after deploying changes from nucypher/nucypher-contracts#77
There was a problem hiding this comment.
My vote would be the pause until nucypher/nucypher-contracts#77 is dealt with. This PR would then be a downstream change. Note that PR 77 would also have downstream effects on nucypher.
I'd like to avoid hard-coding things that we know will change in actual code; tests are one thing, but in actual code it should be avoided if possible.
There was a problem hiding this comment.
I agree. Let's keep this PR reviewed & open, and reconsider merging after downstream changes are dealt with. I will document them where appropriate.
src/dkg.ts
Outdated
| shares: number; | ||
| threshold: number; | ||
| } | ||
| ): Promise<DkgRitual> { | ||
| const ritualId = 2; |
There was a problem hiding this comment.
I assume all of this is only for testing. Is this something that can be moved to tests / mocked in tests instead of including it in actual code?
There was a problem hiding this comment.
My assumption was that for the time being we're going to operate on a set of premade rituals, and right now we only have one
There was a problem hiding this comment.
On second thought I think it's reasonable to expose ritualId as a parameter at least within the SDK. I will add this change and changes related to ritual initialization (which should work on lynx now) in a separate PR
There was a problem hiding this comment.
Implemented in #233
|
Bundled size for the package is listed below: build/module/types/ethers-contracts/factories: 82.03 KB |
piotr-roslaniec
changed the title
lynx
piotr-roslaniec
force-pushed
the
local-verification
branch
from
e0dadba
to
46fb59a
Compare
|
So the original issue was caused by rebasing branches in the wrong order, i.e. |
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
lynx. Usage of keys from the coordinator is blocked by Add a public key field to the Participant struct nucypher-contracts#77lynxFerveoPublicKeyinCoordinatorcontract nucypher#3167Coordinatorcontract #234