allow orchestrator to set ticket maxfacevalue #2290
Conversation
There was a problem hiding this comment.
@0xB79 Thanks a lot for the PR. Great to see the community contribution!
Before we dig into the code review, could you please explain the use cases for the changes you made?
- I understand that one of the changes is to make
-ticketEVflag configurable at runtime. In which scenarios would the Broadcaster need it? - You added two configuration parameters
faceValueLimitandfaceValueFixed. When would you configure each of them? Maybe we will need to also at them to the flags, but first I need to understand the use cases.
Thanks!
|
Hello @leszko These updates are to allow the Orchestrator more control when setting the TicketParams sent to the Broadcasters.
These could be added as command line flags and be set at run time. Another addition that would be helpful is adding ability to set these options with Some examples of tickets received by orchestrator after updating running Orchestrator node: Set faceValueLimit to 0.02ETH: Set fixedFaceValue to 0.11 ETH, just above calculated faceValue: Set fixedFaceValue to 0.50 ETH: Set ticketEV to double default ticketEV to get less tickets with higher winProb: |
|
@0xB79 Thanks for the explanation! I add @yondonfu and @hthillman to decide if we want to add these Concerning I'll add some comments wrt the code itself after the comments from @yondonfu and/or @hthillman . |
|
I think giving Os more control over the faceValue used for tickets makes sense. However, I'm in favor of a configurable flag to adjust the maximum faceValue to use for a ticket, but not a configurable flag to set a fixed faceValue to use for a ticket right now because the security of the PM protocol right now depends on an O using a faceValue that does not exceed the reserve allocation that a B commits to the O (i.e. the B's reserve divided by the # of active Os). If an O uses a faceValue that exceeds this reserve allocation then the difference between the faceValue and the reserve allocation is the value that O risks losing out on in the even that a B's deposit is depleted before the O redeems a winning ticket. So, I'd advocate for the node to behave as safely as possible as a default to avoid scenarios where an O sets a fixed faceValue that causes it to lose out on value if a B's deposit is depleted. If O can set the maximum faceValue to use then in the scenario where the B's reserve allocation is less than the maximum faceValue the node will still automatically set the faceValue to the reserve allocation as the "safe" value to use. In the scenario where the B's reserve allocation is greater than the maximum faceValue the node will just use the maximum faceValue as the faceValue. I believe this is more or less the behavior of the |
ad-astra-video
force-pushed
the
dev
branch
2 times, most recently
from
2e19c89
to
5a7e860
Compare
|
I have updated to add command line flag and remove fixedFaceValue option. Tested by adding these commits to v0.5.29 code and building. Also, added livepeer_cli option to update while orchestrator node is running. |
There was a problem hiding this comment.
Thanks for the changes @0xB79 !
Added two comments. In short, the change about setting maxFaceValue is fine, however I'm not sure about the changes related to setting EV. I thought we only want to add the ability to set max face value in this PR.
Also, please rebase you branch to master since there are some conflicts.
cmd/livepeer/livepeer.go
Outdated
| glog.Errorf("-maxFaceValue must be a valid integer, but %v provided. Restart the node with a different valid value for -maxFaceValue", *maxFaceValue) | ||
| return | ||
| } else { | ||
| n.SetFaceValueLimit(mfv) |
There was a problem hiding this comment.
For the sake of consistency, maybe let's name it n.SetMaxFaceValue() instead of n.SetFaceValueLimit()?
cmd/livepeer/livepeer.go
Outdated
| @@ -633,10 +634,19 @@ func main() { | |||
| timeWatcher, | |||
| cfg, | |||
| ) | |||
| n.SetTicketEV(ev) | |||
There was a problem hiding this comment.
Isn't that line setting the fixed face value? I think we wanted to allow only setting max face value, why do we need this line then?
|
Hello, I applied the changes to dev branch using master branch code base. I am not sure I did this correctly...if I did not, commit [ I incorporated the changes requested where it only allows setting a max face value. I tested by building and running on working orchestrator node. There was one bug fix in starter.go where the |
|
@leszko I changed it to the livepeer:master branch and it shows 9 files changed now. |
cmd/livepeer/starter/starter.go
Outdated
| glog.Errorf("-maxFaceValue must be a valid integer, but %v provided. Restart the node with a different valid value for -maxFaceValue", *cfg.MaxFaceValue) | ||
| return |
There was a problem hiding this comment.
| glog.Errorf("-maxFaceValue must be a valid integer, but %v provided. Restart the node with a different valid value for -maxFaceValue", *cfg.MaxFaceValue) | |
| return | |
| panic(fmt.Errorf("-maxFaceValue must be a valid integer, but %v provided. Restart the node with a different valid value for -maxFaceValue", *cfg.MaxFaceValue)) |
Please use panic() instead.
pm/recipient.go
Outdated
| //if faceValue > maxfacevalue, set faceValue=maxfacevalue | ||
| if r.maxfacevalue.Cmp(faceValue) < 0 { |
There was a problem hiding this comment.
| //if faceValue > maxfacevalue, set faceValue=maxfacevalue | |
| if r.maxfacevalue.Cmp(faceValue) < 0 { | |
| if r.maxfacevalue.Cmp(faceValue) < 0 { |
Nit: There is no need for this comment.
There was a problem hiding this comment.
Agree, not used to Go with using .Cmp. Will remove comment.
| @@ -517,6 +517,25 @@ func (s *LivepeerServer) setServiceURI(client eth.LivepeerEthClient, serviceURI | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *LivepeerServer) setMaxFaceValueHandler() http.Handler { | |||
There was a problem hiding this comment.
Would be nice if you could add a unit test in handlers_test.go.
There was a problem hiding this comment.
Unit tests added for maxfacevalue handler. Took some time for me to get up to speed on how to make a mock recipient in unit tests for the stub livepeer server.
There was a problem hiding this comment.
Unit tests added.
…estrator node and conforming update to webserver.go for required form params
leszko
requested review from
thomshutt
and removed request for
yondonfu and
hthillman
| ) | ||
| if err != nil { | ||
| glog.Errorf("Error setting up PM recipient: %v", err) | ||
| return | ||
| } | ||
| mfv, _ := new(big.Int).SetString(*cfg.MaxFaceValue, 10) |
There was a problem hiding this comment.
Should we also check the returned bool that indicates success here?
There was a problem hiding this comment.
My understanding was SetString returns nil if it fails. Is this correct? This is same pattern used on lines 675 and 740. I can update if prefer to check bool rather than value or prefer to check both.
There was a problem hiding this comment.
From looking at the SetString code, it seems like the way you've done it probably is safe:
If SetString fails, the value of z is undefined but the returned value is nil
but I think to be safe and not depend on that behavior never changing, let's also check the success bool isn't false
There was a problem hiding this comment.
I think that for this one, for the sake of consistency, we can keep it as it is. And later convert all calls mfv, _ := new(big.Int).SetString(*cfg.MaxFaceValue, 10) to what Thom suggested.
|
@0xB79 Last comments from Thom and we can merge your PR 🥳 Also, please run |
ad-astra-video
changed the title
|
Updated title and pull request description to remove the items dropped from review comments. |
Codecov Report
@@ Coverage Diff @@
## master #2290 +/- ##
===================================================
- Coverage 55.02775% 54.96218% -0.06558%
===================================================
Files 94 94
Lines 19641 19699 +58
===================================================
+ Hits 10808 10827 +19
- Misses 8236 8274 +38
- Partials 597 598 +1
Continue to review full report at Codecov.
|
|
Thanks again for the PR @0xB79 |
What does this pull request do? Explain your changes. (required)
Allows Orchestrator nodes to specify ticket max face value while node is running. Default is no max face value.
Specific updates (required)
How did you test each of these updates (required)
After updates completed, ran orchestrator node on mainnet and monitored logs after updating settings with webserver endpoints.
Ticket was redeemed for 0.01 ETH by remote redeemer node for orchestrator. https://discord.com/channels/423160867534929930/808142181679235083/945862545527439370.
How to use webserver endpoints:
Does this pull request close any open issues?
No
Checklist:
makeruns successfully - with these changes applied to v0.5.29 source code./test.shpass