claimWork and verify without transcoding verification #35

yondonfu · 2017-06-20T22:06:32Z

Addresses #23 and #24

Using pasted in ECVerify.sol and MerkleProof.sol libraries. The functionality of both these libraries are both being added to OpenZeppelin - once the relevant PRs are merged in we can remove these pasted in libraries and directly use the libraries in OpenZeppelin.
Pulled transcode job logic into a separate TranscodeJobs.sol library. This was needed to deploy the LivepeerProtocol.sol contract without exceeding gas limits. Should also be a +1 for code modularity

…Need to refactor to deploy due to gas costs

…Rate

yondonfu · 2017-06-20T22:09:32Z

contracts/LivepeerProtocol.sol

@@ -665,7 +682,7 @@ contract LivepeerProtocol {
     * for its time window
     * @param _transcoder Address of transcoder
     */
-    function validRewardTimeWindow(address _transcoder) internal returns (bool) {
+    function validRewardTimeWindow(address _transcoder) internal constant returns (bool) {


No state changes in this function so should be marked constant

yondonfu · 2017-06-20T22:11:58Z

test/LivepeerProtocol.js

+        });
+
+        it("should fail if the segment is not eligible for verification", async function() {
+


This is left blank for now because still figuring out the best way to test segment verification eligibility

yondonfu · 2017-06-20T22:15:35Z

test/LivepeerProtocol.js

+            const d3 = Buffer.from("4ce8765e720c576f6f5a34ca380b3de5f0912e6e3cc5355542c363891e54594b", "hex");
+
+            // Segment hashes (streamId, segmentSequenceNumber, dataHash)
+            const s0 = abi.soliditySHA3(["uint256", "uint256", "bytes"], [streamId, 0, d0]);


Solidity's built-in sha3 function accepts an arbitrary of number of arguments and tightly packs them together meaning they are concatenated without padding. The soliditySHA3 function from ethereumjs-abi tightly packs arguments before hashing thereby following the behavior of Solidity's sha3 in Javascript.

yondonfu · 2017-06-20T22:28:51Z

contracts/TranscodeJobs.sol

+        // Check if sender is assigned transcoder
+        if (self.jobs[_jobId].transcoderAddress != msg.sender) throw;
+        // Check if previous verification period over
+        if (block.number < self.jobs[_jobId].endVerificationBlock) throw;


For now we force transcoders to call verify a call to claimWork() before they can make a subsequent call to claimWork()

In the future, if verification finishes, this also should be able to be called even if endVerificationBLock hasn't been reached.

yondonfu · 2017-06-20T22:30:04Z

contracts/TranscodeJobs.sol

+        // Check if job is active
+        if (jobStatus(self, _jobId) != JobStatus.Active) throw;
+        // Check if sender is assigned transcoder
+        if (self.jobs[_jobId].transcoderAddress != msg.sender) throw;


Only the assigned transcoder for a job can submit transcode claims for a job. Therefore, the transcode claims used to form the submitted Merkle root do not need to be signed by the transcoder

yondonfu · 2017-06-20T23:23:41Z

contracts/TranscodeJobs.sol

+     * @param _segmentSequenceNumber Sequence number of segment in stream
+     * @param _verificationRate Rate at which a particular segment should be verified
+     */
+    function shouldVerifySegment(uint256 _segmentSequenceNumber,


Rather than access state (i.e. a Jobs struct), all dependencies are passed in as parameters in order to make this helper function easier to test.

dob

Awesome work. A lot to dig into here. I think we should err on the side of overcommenting for now because it's a lot to wrap your head around if you're trying to understand the protocol and tests from the outside.

dob · 2017-06-21T21:23:11Z

contracts/TranscodeJobs.sol

+        }
+    }
+
+    function newJob(Jobs storage self, uint256 _streamId, bytes32 _transcodingOptions, uint256 _maxPricePerSegment, address _electedTranscoder) returns (bool) {


might need to do validation on _transcodingOptions, but we'll leave that for the future.

dob · 2017-06-21T21:24:27Z

contracts/TranscodeJobs.sol

+                );
+    }
+
+    function getJobWorkDetails(Jobs storage self, uint256 _jobId) constant returns (uint256, uint256, bytes32) {


At the minimum let's add a comment on what this function is used for and how it's different than getJob()

or perhaps rename to getJobTranscodeClaimDetails()

dob · 2017-06-21T21:28:16Z

contracts/TranscodeJobs.sol

+        // Check if sender is assigned transcoder
+        if (self.jobs[_jobId].transcoderAddress != msg.sender) throw;
+        // Check if previous verification period over
+        if (block.number < self.jobs[_jobId].endVerificationBlock) throw;


In the future, if verification finishes, this also should be able to be called even if endVerificationBLock hasn't been reached.

dob · 2017-06-21T21:31:32Z

contracts/TranscodeJobs.sol

+        return true;
+    }
+
+    function verify(Jobs storage self, uint256 _jobId, uint256 _segmentSequenceNumber, bytes32 _dataHash, bytes32 _transcodedDataHash, bytes _broadcasterSig, bytes _proof, uint64 _verificationRate) constant returns (bool) {


Should we label this (and other functions) "internal", or whatever the proper scoping is to prevent it from being called externally by users?

Yondon mentioned that marking it internal possibly causes issues with deployment of the protocol contract. We can double check.

I would also consider changing the name of this function since "Verification" is a term that means verifying the offchain work, whereas this is just validating that the claim was correct.

dob · 2017-06-21T21:37:24Z

contracts/TranscodeJobs.sol

+                                 _verificationRate
+                                 )) return false;
+        // Check if segment was signed by broadcaster
+        if (!ECVerify.ecverify(sha3(self.jobs[_jobId].streamId, _segmentSequenceNumber, _dataHash), _broadcasterSig, self.jobs[_jobId].broadcasterAddress)) return false;


Let's consider using the keccak256 version of the alias since the hash is actually not sha3 according to spec. This is minor. Don't need to do it, but we could consider being trailblazers of truth.

dob · 2017-06-21T21:56:41Z

contracts/LivepeerProtocol.sol

+     * @param _broadcasterSig Broadcaster's signature over segment
+     * @param _proof Merkle proof for the signed transcode claim
+     */
+    function verify(uint256 _jobId, uint256 _segmentSequenceNumber, bytes32 _dataHash, bytes32 _transcodedDataHash, bytes _broadcasterSig, bytes _proof) returns (bool) {


Will possibly be payable since someone needs to pay Oraclize or Truebit. TBD whether we can use LPT for this or need to use Ether.

dob · 2017-06-21T22:03:18Z

test/LivepeerProtocol.js

+            const proof = merkleTree.getHexProof(tClaim0);
+
+            // Account 1 calls verify
+            await instance.verify(0, 0, utils.bufferToHex(d0), utils.bufferToHex(tD0), utils.bufferToHex(bSig0), proof, {from: accounts[1]});


Should check that this does not throw.

dob · 2017-06-21T22:07:07Z

test/LivepeerProtocol.js

+
+            try {
+                // Account 1 calls verify
+                await instance.verify(0, 2, utils.bufferToHex(d2), utils.bufferToHex(tD2), utils.bufferToHex(bSig3), proof, {from: accounts[1]});


Fails because of bSig3 when it should be bSig2.

dob · 2017-06-21T22:08:09Z

test/TranscodeJobs.js

+            let hashNum = new BigNumber(hash, 16);
+            let res = hashNum.mod(verificationRate);
+
+            while (res != 0) {


Let's add a comment about why we're iterating here to find a suitable candidate segment number.

dob · 2017-06-21T22:09:36Z

test/TranscodeJobs.js

+            let hashNum = new BigNumber(hash, 16);
+            let res = hashNum.mod(verificationRate);
+
+            while (res == 0) {


Same comment as above about why we're looping here.

yondonfu · 2017-06-22T15:58:35Z

In response to the comment on marking library functions in TranscodeJobs.sol as internal by @dob: if an internal library function is called by a contract, the Solidity compiler will actually pull in the internal library function code and inline it into the caller contract function. This is in contrast to when a public library function (public by default) is called by a contract, the Solidity compiler will use delegatecall to execute the library function code in the context of the current contract without pulling in additional code.

If many internal library functions are called by a contract, the bytecode size of the contract will increase along with the gas costs of deploying the contract. I tested this out locally and marking all the relevant functions in TranscodeJobs.sol as internal led to a failed deployment of the LivepeerProtocol.sol contract due to insufficient gas.

I don't think its too big of an issue to have the functions in TranscodeJobs.sol be public because they only have access to the state variables of LivepeerProtocol if they are explicitly supplied. The only way to modify state of LivepeerProtocol using a library function from TranscodeJobs.sol is by using a contract function that calls the library function.

dob

Looks good. 🚢

yondonfu added 6 commits June 15, 2017 15:46

claimWork() and verify() using pasted ECVerify and MerkleProof libs. …

f8fed06

…Need to refactor to deploy due to gas costs

Job logic moved to TranscodeJobs lib

1c27ab9

Tests for claimWork()

75a2a3c

Some initial tests, working on testing ecverify

523da51

Working tests for verify()

b620932

should verify segment check passing with dummy value for verification…

f5300ca

…Rate

yondonfu requested review from dob and ericxtang June 20, 2017 22:06

Remove trailing comma in package.json

4e00467

yondonfu commented Jun 20, 2017

View reviewed changes

Decouple TranscodeJobs.shouldVerifySegment from state and add tests

68c6798

yondonfu commented Jun 20, 2017

View reviewed changes

dob requested changes Jun 21, 2017

View reviewed changes

Naming changes and additional comments for clarity

34b5597

dob approved these changes Jun 22, 2017

View reviewed changes

yondonfu merged commit 3ddcaee into master Jun 22, 2017

yondonfu deleted the yf/claimWorkAndVerify branch June 22, 2017 20:29

This was referenced Jun 22, 2017

EndJob() and transcoding claim #23

Closed

Verify() and proof of transcoding #24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

claimWork and verify without transcoding verification #35

claimWork and verify without transcoding verification #35

yondonfu commented Jun 20, 2017

yondonfu Jun 20, 2017

yondonfu Jun 20, 2017

yondonfu Jun 20, 2017

yondonfu Jun 20, 2017

dob Jun 21, 2017

yondonfu Jun 20, 2017

yondonfu Jun 20, 2017 •

edited

Loading

dob left a comment

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

dob Jun 21, 2017

yondonfu commented Jun 22, 2017 •

edited

Loading

dob left a comment

		});

		it("should fail if the segment is not eligible for verification", async function() {

claimWork and verify without transcoding verification #35

claimWork and verify without transcoding verification #35

Conversation

yondonfu commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yondonfu Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

dob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yondonfu commented Jun 22, 2017 • edited Loading

dob left a comment

Choose a reason for hiding this comment

yondonfu Jun 20, 2017 •

edited

Loading

yondonfu commented Jun 22, 2017 •

edited

Loading