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

VODKA: Slashing for incorrect re-encryption #507

Merged
merged 96 commits into from Feb 26, 2019

Conversation

@szotov
Copy link
Member

commented Nov 1, 2018

The merge commits of the original PRs that constitute vodka was lost during some rebase.
To facilitate reviewing, this is the order of merged PRs:

  • #591: Slashing in the MinersEscrow
  • #509: On-chain ZKP verification for re-encryption correctness
  • #606: Improvement of the MiningAdjudicator contract
  • #653: Check KFrag signature as part of on-chain ZKP verification
  • #770: Updates version of solidity in vodka
@szotov szotov added this to Scope & Discussion in During Federated Testnet via automation Nov 1, 2018
@szotov szotov force-pushed the vodka branch from 9836ae5 to e58b324 Nov 1, 2018
@jMyles jMyles changed the title [WIP] Slashing for incorrect re-encryption [WIP-RELEASE] Slashing for incorrect re-encryption Nov 2, 2018
@cygnusv cygnusv changed the base branch from master to federated Nov 12, 2018
@cygnusv cygnusv changed the base branch from federated to master Nov 12, 2018
@szotov szotov force-pushed the vodka branch from e58b324 to 779b024 Nov 12, 2018
@cygnusv cygnusv force-pushed the vodka branch from 779b024 to 84896e5 Dec 3, 2018
@cygnusv cygnusv changed the title [WIP-RELEASE] Slashing for incorrect re-encryption [WIP-RELEASE][Vodka] Slashing for incorrect re-encryption Dec 3, 2018
@cygnusv

This comment was marked as outdated.

Copy link
Member

commented Dec 3, 2018

Rebased vodka on top of current master(3051b10) .

Proposed ramp order to merge:

  • #592 Adapt NuCypher to umbral==0.1.2a0. Brings back coincurve
  • #591 Slashing in the MinersEscrow
  • #509 On-chain ZKP verification
  • #606 Improvement of Mining Adjudicator (WIP)
@cygnusv cygnusv force-pushed the vodka branch from f5ac283 to 4085a95 Dec 17, 2018
@KPrasch KPrasch added this to WIP in Ramp order via automation Dec 17, 2018
@KPrasch KPrasch moved this from WIP to Waiting for review in Ramp order Dec 17, 2018
@KPrasch KPrasch moved this from Waiting for review to WIP in Ramp order Dec 17, 2018
@cygnusv cygnusv force-pushed the vodka branch 4 times, most recently from 241c5b7 to 74487d4 Jan 4, 2019
@cygnusv cygnusv force-pushed the vodka branch from 520c4cb to 54ed11a Jan 31, 2019
@cygnusv cygnusv force-pushed the vodka branch from f7429a6 to cb819b4 Feb 12, 2019
@szotov szotov force-pushed the vodka branch 3 times, most recently from 6784be0 to 097629d Feb 15, 2019
@szotov szotov force-pushed the vodka branch from 3e8e743 to 867925f Feb 23, 2019
// Check that CFrag is not evaluated yet
bytes32 evaluationHash = SignatureVerifier.hash(
abi.encodePacked(_capsuleBytes, _cFragBytes), hashAlgorithm);
require(!evaluatedCFrags[evaluationHash]);

This comment has been minimized.

Copy link
@jMyles

jMyles Feb 23, 2019

Member

Forgive my ignorance here. So, the evaluatedCFrags mapping will change any time a block includes a completed transaction against this contract, right?

So what happens if the same CFrags is challenged many times in the same block time, before the adjudication is finalized in a block? Will some of the adjudications end up in Uncles or thrown away entirely?

Sub-question: If an Uncle contains a proper adjudication but the block itself didn't include it, is there a way to use the adjudication result on the Uncle?

This comment has been minimized.

Copy link
@szotov

szotov Feb 24, 2019

Author Member

First transaction in block will be completed and others will fail because of require(!evaluatedCFrags[evaluationHash]); (state changes after each transaction in a block). So others can't make any changes on state. The same as we put them in different blocks.

I dont know if miner could use precalculated result from uncle, but for client it's useless. Because uncle does not change anything for client, and result from this transaction just could show is CFrag correct. But client can check it locally without transaction.

This comment has been minimized.

Copy link
@michwill

michwill Feb 24, 2019

Member

Yes, the client can check locally. There could be several competing transactions in the block though. If there are, the first will collect the reward, and others will pat small fee for checking this condition before fail.
That's not ideal, but probably ok for now

tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
tests/metrics/estimate_gas.py Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
pragma solidity ^0.5.3;

This comment has been minimized.

Copy link
@jMyles

jMyles Feb 24, 2019

Member

It looks like we're ultimately passing the important stuff into ecrecover. It's not immediately clear to me why this wrapper is necessary; maybe I'm just getting review fatigue. :-)

This comment has been minimized.

Copy link
@michwill

michwill Feb 24, 2019

Member

Reads more natural to me (e.g. verify returns a boolean instead of an Ethereum address which logically would make no sense when a signature is derived from a signing non-Ethereum key). Plus, hashing and parsing signatures is embedded.
IMO would be less readable when not wrapped and when r/s/v are parsed in Python

This comment has been minimized.

Copy link
@michwill

michwill Feb 24, 2019

Member

But I agree that it's a really, really dense read overall...

This comment has been minimized.

Copy link
@cygnusv

cygnusv Feb 24, 2019

Member

As @michwill points out, ecrecover expects the individual r, s and v components the signature, the message already prehashed, and outputs an ETH address corresponding to the public key. The SignatureVerifier wrapper is makes things easier by allowing to pass the original message, the signature as bytes (which is how we'll have it most of the time) and the verifying public key.

This comment has been minimized.

Copy link
@jMyles

jMyles Feb 24, 2019

Member

...but then aren't we paying gas for this small operations when we can just as easily do them in python?

This comment has been minimized.

Copy link
@szotov

szotov Feb 24, 2019

Author Member

we must have signatures verification in contract and of course better to just get r, s, v. But if we try to put it in method evaluateCFrag - then will be not enough variables in stack

This comment has been minimized.

Copy link
@jMyles

jMyles Feb 24, 2019

Member

Ahh, so this is more for scope hygiene than readability?

This comment has been minimized.

Copy link
@szotov

szotov Feb 24, 2019

Author Member

at first place yes
but also could be only 16 local variables at once, so we can't put everything in separate variables

This comment has been minimized.

Copy link
@cygnusv

cygnusv Feb 24, 2019

Member

Solidity (or more correctly, the EVM) sucks on this regard. The "Stack too deep" forces us to do weird and ugly things to fit everything in the stack space.

Co-Authored-By: szotov <zotov89@mail.ru>
# Travel to the start of the next period to prevent problems with unexpected overflow first period
testerchain.time_travel(hours=1)

escrow, escrow_dispatcher = escrow
policy_manager, policy_manager_dispatcher = policy_manager
adjudicator, adjudicator_dispatcher = adjudicator

This comment has been minimized.

Copy link
@jMyles

jMyles Feb 24, 2019

Member

Lines like this drive me crazy. What was adjudicator if the first member is adjudicator?

This comment has been minimized.

Copy link
@szotov

szotov Feb 24, 2019

Author Member

Agreed, not the best naming
first adjudicator, escrow, policy_manager are tuples: (contract_abi, dispatcher_abi)

@jMyles
jMyles approved these changes Feb 24, 2019
Copy link
Member

left a comment

I approve. Some notes:

  • I have not read this with the fullness and vigor that I think is required to have confidence that everything is as it appears, or even that everything comports with the abstractions and metaphors that I think they do. Instead, this approval is more of a certification that nothing looks wildly out-of-place and that I'm prepared to talk coherently about this material in the future by doing additional deep-dives during these conversation.
  • The delegatecall issue is something that is rightfully a topic of ongoing discussion.
  • Reading this PR, it is more clear to me than ever that Miner is a broken metaphor. I have never liked this name, but reading this, I am given pause that there is a number mismatch between the work types for a given node and the practices surrounding this work types. For example, the inflation economics apply to a "Staker" or "Worker" (half of what we currently call Miner). Since this is a work token, Worker makes sense to me. However, the work being performed pursuant to this work token, as it relates to the adjudication, is not of only one kind. Presently, we expect Ursula to perform PRE and understand that she'll be slashed if she fails to perform. Let's imagine a future in which we task a different character to perform SSS or NuFHE under the auspices of the same work token. Is this person still a Miner? It seems to me that Worker is a concept that might utilize Ursulas and SSS/NuFHE-characters and is not properly understood as a "mining mixin." Normally, I am not terribly concerned about these sorts of future-proofing exercises, but I think that we'll have crossed an epitaph rubicon if we deploy these smart contracts with the word "Miner" - it will be hard to educate our audience on what we really mean.
  • There are other naming conventions that are bad, but in name only (ie, not conceptually incorrect like Miner). The most visible among these is _testTarget. This is not a test concept at all, but a genuine production candidate for deployment. I think that the naming around contract targeting needs to be upgraded in seriousness to indicate this. My eyes tend to gloss over "test" things.
  • I am still concerned about #563 in terms of our overall gas strategy. This is only tangentially related, but worth considering in the wake of a merge on this PR.
  • The fast-paced but inquisitive reader will benefit from many of our invocations of require having a second argument explaining the situation.
  • Similarly, there are many lines which go by without comment which might be helped with a little nudge (and mind you, I'm not normally comment-heave myself). For example, I find this document to be quite readable: https://github.com/livepeer/protocol/blob/master/contracts/ManagerProxy.sol
  • Following this merge, there needs to be a reckoning such that all the mechanics herein are contextualized in some kind of whitepaper or documentation so that the economic intentions are more clear.

These concerns notwithstanding, I'm super happy to finally see this thing merged. What a big step. It is a particular pleasure to read the interplay between @szotov and @cygnusv play out in certain modules, such as MiningAdjudicator.sol.

Copy link
Member

left a comment

Perhaps, good to merge.
As soon as we do though, need to document, how the slashing currently works, probably adding explaining comments in the code at the same time.
Also, we will need to have discussions about data separation pattern vs proxy contracts; slashing for non-responsiveness; and difficulty of future upgrades which enable scaling (e.g. evm-capable sidechains, without talking through the details, just if it will be possible at all to do an upgrade of this sort once what we have the contracts deployed)

@KPrasch KPrasch dismissed their stale review Feb 26, 2019

reviewed with team

@KPrasch

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I have similar thoughts to @michwill and @jMyles - and not much more to add.

This is a massive step towards realizing the slashing protocol, but let's continue to question our approach to building upgradable contracts as we move into the next phases of deployment and testing.

Nice work @szotov and @cygnusv

@cygnusv cygnusv merged commit ec6b754 into master Feb 26, 2019
20 checks passed
20 checks passed
ci/circleci: actors Your tests passed on CircleCI!
Details
ci/circleci: agents Your tests passed on CircleCI!
Details
ci/circleci: blockchain Your tests passed on CircleCI!
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/circleci: character Your tests passed on CircleCI!
Details
ci/circleci: cli Your tests passed on CircleCI!
Details
ci/circleci: config Your tests passed on CircleCI!
Details
ci/circleci: contracts Your tests passed on CircleCI!
Details
ci/circleci: crypto Your tests passed on CircleCI!
Details
ci/circleci: deployers Your tests passed on CircleCI!
Details
ci/circleci: estimate_gas Your tests passed on CircleCI!
Details
ci/circleci: heartbeat_demo Your tests passed on CircleCI!
Details
ci/circleci: keystore Your tests passed on CircleCI!
Details
ci/circleci: mypy Your tests passed on CircleCI!
Details
ci/circleci: network Your tests passed on CircleCI!
Details
ci/circleci: pip_install_36 Your tests passed on CircleCI!
Details
ci/circleci: pip_install_37 Your tests passed on CircleCI!
Details
ci/circleci: pipenv_install_36 Your tests passed on CircleCI!
Details
ci/circleci: pipenv_install_37 Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
During Federated Testnet automation moved this from Scope & Discussion to Done Feb 26, 2019
@KPrasch KPrasch deleted the vodka branch Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.