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

job() function and Job struct #30

Merged
merged 5 commits into from Jun 14, 2017

Conversation

Projects
None yet
2 participants
@yondonfu
Member

yondonfu commented Jun 12, 2017

Addresses #22

  • job() function to create a new Job struct
  • electCurrentActiveTranscoder(_maxPricePerSegment) is used to pseudorandomly select a current active transcoder with an acceptable price per segment
  • transcoderOptions is currently a bytes32 with encoding semantics to be determined at a later point in time

@yondonfu yondonfu requested review from dob and ericxtang Jun 12, 2017

Show outdated Hide outdated contracts/LivepeerProtocol.sol
Show outdated Hide outdated contracts/LivepeerProtocol.sol
Show outdated Hide outdated contracts/LivepeerProtocol.sol
@@ -121,6 +121,22 @@ contract LivepeerProtocol {
// rewardMultiplier[1] -> transcoder's cumulative stake for round
mapping (address => mapping (uint256 => uint256[2])) public rewardMultiplierPerTranscoderAndRound;
// The various states a job can be in
enum JobStatus { Inactive, Active }

This comment has been minimized.

@yondonfu

yondonfu Jun 13, 2017

Member

As mentioned in Doug's previous comment, a transcoder might submit transcode claims for multiple ranges of segments. As a result, I don't think we can determine on-chain whether all the work for a job has been claimed without additional information. Furthermore, since multiple submissions of transcode claims will require multiple calls to verify(), I also don't think we can determine on-chain whether all the claims for a job have been verified without additional information.

It seems that as of right now we can only accurately say that a job is one of two states: Inactive or Active. A job would become Active when a broadcaster submits a job using job(). A job would become Inactive after either the broadcaster or transcoder for a job calls endJob() and a certain number of blocks has passed.

@yondonfu

yondonfu Jun 13, 2017

Member

As mentioned in Doug's previous comment, a transcoder might submit transcode claims for multiple ranges of segments. As a result, I don't think we can determine on-chain whether all the work for a job has been claimed without additional information. Furthermore, since multiple submissions of transcode claims will require multiple calls to verify(), I also don't think we can determine on-chain whether all the claims for a job have been verified without additional information.

It seems that as of right now we can only accurately say that a job is one of two states: Inactive or Active. A job would become Active when a broadcaster submits a job using job(). A job would become Inactive after either the broadcaster or transcoder for a job calls endJob() and a certain number of blocks has passed.

/*
* End a job. Can be called by either a broadcaster or transcoder of a job
*/
function endJob(uint256 _jobId) returns (bool) {

This comment has been minimized.

@yondonfu

yondonfu Jun 13, 2017

Member

In #23 endJob() is described as the function called by transcoders to submit transcode claims for a range of segments. This name does not seem too semantically accurate and I think endJob() would make more sense as a name for a function used by a broadcaster or transcoder to set a job as inactive. Perhaps the function for submitting transcode claims for a range of segments can instead be called claimWorkForJob().

@yondonfu

yondonfu Jun 13, 2017

Member

In #23 endJob() is described as the function called by transcoders to submit transcode claims for a range of segments. This name does not seem too semantically accurate and I think endJob() would make more sense as a name for a function used by a broadcaster or transcoder to set a job as inactive. Perhaps the function for submitting transcode claims for a range of segments can instead be called claimWorkForJob().

return address(0);
} else {
// Pseudorandomly select an available transcoder that charges an acceptable price per segment
return availableTranscoders[uint256(block.blockhash(block.number.sub(1))) % numAvailableTranscoders];

This comment has been minimized.

@yondonfu

yondonfu Jun 13, 2017

Member

We loop through all currentActiveTranscoders to find the set of transcoders charging an acceptable price per segment. Then we generate a pseudorandom index to select a transcoder from the set of available transcoders.

I'm wary of pseudorandomly selecting a transcoder and then checking if the transcoder is charging an acceptable price per segment because using modulo to generate a pseudorandom index in a range might bias smaller indices and there could be a case where transcoders with an acceptable price per segment are associated with higher indices leading to long loops. While the best case of pseudorandomly selecting a transcoder immediately that charges an acceptable price per segment is nice, I'm not sure if it's worth dealing with the worst case.

@yondonfu

yondonfu Jun 13, 2017

Member

We loop through all currentActiveTranscoders to find the set of transcoders charging an acceptable price per segment. Then we generate a pseudorandom index to select a transcoder from the set of available transcoders.

I'm wary of pseudorandomly selecting a transcoder and then checking if the transcoder is charging an acceptable price per segment because using modulo to generate a pseudorandom index in a range might bias smaller indices and there could be a case where transcoders with an acceptable price per segment are associated with higher indices leading to long loops. While the best case of pseudorandomly selecting a transcoder immediately that charges an acceptable price per segment is nice, I'm not sure if it's worth dealing with the worst case.

This comment has been minimized.

@dob

dob Jun 14, 2017

Member

This is fine for now. In reality I think we want to select in proportion to stake as well. I have seen techniques for generating a random # and making selection according to stake. Let's use this for now, but track as a separate issue.

@dob

dob Jun 14, 2017

Member

This is fine for now. In reality I think we want to select in proportion to stake as well. I have seen techniques for generating a random # and making selection according to stake. Let's use this for now, but track as a separate issue.

@dob

Just about there. Curious about the potential throw statement in the getJob() function and whether it's necessary. But after that I think we can 🚢

* @param _jobId Job identifier
*/
function getJob(uint256 _jobId) constant returns (uint256, uint256, bytes32, uint256, address, address, uint256) {
Job job = jobs[_jobId];

This comment has been minimized.

@dob

dob Jun 14, 2017

Member

should we throw if the jobID doesn't exist? It might be better to fail fast than to return an empty job that needs to be checked.

@dob

dob Jun 14, 2017

Member

should we throw if the jobID doesn't exist? It might be better to fail fast than to return an empty job that needs to be checked.

This comment has been minimized.

@yondonfu

yondonfu Jun 14, 2017

Member

I don't think we need to throw here because currently when endJob() is called, a job becomes inactive, but is not deleted from the jobs array. I lean toward keeping it this way because we retain a history of jobs, but curious to hear what you think. As a result, this function would never return an empty job and might also be used to inspect information about an inactive job.

I also realized that jobs[_jobId] will throw if _jobId >= jobs.length (handled by Solidity) and if _jobId < 0 (since _jobId is a uint256 any negative values will be rejected), so we don't need an additional check + throw for these cases.

@yondonfu

yondonfu Jun 14, 2017

Member

I don't think we need to throw here because currently when endJob() is called, a job becomes inactive, but is not deleted from the jobs array. I lean toward keeping it this way because we retain a history of jobs, but curious to hear what you think. As a result, this function would never return an empty job and might also be used to inspect information about an inactive job.

I also realized that jobs[_jobId] will throw if _jobId >= jobs.length (handled by Solidity) and if _jobId < 0 (since _jobId is a uint256 any negative values will be rejected), so we don't need an additional check + throw for these cases.

Show outdated Hide outdated contracts/LivepeerProtocol.sol
Show outdated Hide outdated contracts/LivepeerProtocol.sol
return address(0);
} else {
// Pseudorandomly select an available transcoder that charges an acceptable price per segment
return availableTranscoders[uint256(block.blockhash(block.number.sub(1))) % numAvailableTranscoders];

This comment has been minimized.

@dob

dob Jun 14, 2017

Member

This is fine for now. In reality I think we want to select in proportion to stake as well. I have seen techniques for generating a random # and making selection according to stake. Let's use this for now, but track as a separate issue.

@dob

dob Jun 14, 2017

Member

This is fine for now. In reality I think we want to select in proportion to stake as well. I have seen techniques for generating a random # and making selection according to stake. Let's use this for now, but track as a separate issue.

@@ -196,6 +215,9 @@ contract LivepeerProtocol {
// Fail no more than 1% of the time
verificationFailureThreshold = 1;
// A job becomes inactive 100 blocks after endJob() is called
jobEndingPeriod = 100;

This comment has been minimized.

@yondonfu

yondonfu Jun 14, 2017

Member

This is a test value rather than an actual default value. Although in reality, the default value would be larger, our tests use a helper function to fast forward a certain number of blocks by repeatedly using TestRPC's evm_mine request. A large number of evm_mine requests slows down our tests a lot, so I think it makes sense to use smaller values for time related variables just for testing purposes. I filed an issue as a reminder to come back and replace these test values with realistic default values: #33

@yondonfu

yondonfu Jun 14, 2017

Member

This is a test value rather than an actual default value. Although in reality, the default value would be larger, our tests use a helper function to fast forward a certain number of blocks by repeatedly using TestRPC's evm_mine request. A large number of evm_mine requests slows down our tests a lot, so I think it makes sense to use smaller values for time related variables just for testing purposes. I filed an issue as a reminder to come back and replace these test values with realistic default values: #33

@dob

dob approved these changes Jun 14, 2017

@yondonfu yondonfu merged commit 851ddeb into master Jun 14, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@yondonfu yondonfu deleted the yf/job branch Jun 14, 2017

@yondonfu yondonfu referenced this pull request Jun 14, 2017

Closed

Job() request transaction #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment