-
Notifications
You must be signed in to change notification settings - Fork 165
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
core, pm: validate payment sender #1210
Conversation
pm/recipient.go
Outdated
@@ -134,6 +136,11 @@ func (r *recipient) Stop() { | |||
close(r.quit) | |||
} | |||
|
|||
// ValidateSender checks whether sender is a valid ticket sender | |||
func (r *recipient) ValidateSender(sender ethcommon.Address) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding an additional method to the Recipient
public API we could validate the sender in ReceiveTicket()
:
func (r *recipient) ReceiveTicket(...) (string, bool, error) {
...
if err := r.sm.ValidateSender(ticket.Sender); err != nil {
return err
}
if err := r.val.ValidateTicket(...); err != nil {
return err
}
...
}
I see the benefits of this approach as:
- Reduces the # of public API changes in this PR from 2 to 1 since we would only have to change the
SenderMonitor
public API - Eliminates the need to modify the
core.Orchestrator
impl and its tests
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I don't like about that is that we would be validating the sender for each ticket in the case where a payment constitutes multiple tickets. This seems like an inefficiency to me as we want to validate the sender of a payment as a whole rather than separate tickets.
We could return an unacceptable payment error from ReceiveTicket
but this will still try to redeem winning tickets, which I don't see as fair towards B as these errors can be ephemeral and we don't add credit but are redeeming tickets.
Then again we could do if won && !unacceptableReceiveErr {...}
or break the loop upon an unacceptableReceiveError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d4b2a96, but I force pushed since the implementation is substantially different and the diffs wouldn't really matter here I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I don't like about that is that we would be validating the sender for each ticket in the case where a payment constitutes multiple tickets.
The first ticket should fail validation and we would not validate the rest of the tickets in the batch which seems ok.
We could return an unacceptable payment error from ReceiveTicket but this will still try to redeem winning tickets
The error returned would already be considered an unacceptable payment error since we are not returning an AcceptableError
from ReceiveTicket()
when the sender validation fails.
which I don't see as fair towards B as these errors can be ephemeral and we don't add credit but are redeeming tickets.
O should use the strategy that best serves its interest - in this case, it should always redeem a valid winning ticket if it receives one. We're treating a sender validation error the same way that we would treat an unacceptable payment error that is triggered due to a sender exceeding its max error count - we will not credit the sender and will not transcode the segment, but if the ticket wins we will still redeem it.
3521846
to
d4b2a96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
What does this pull request do? Explain your changes. (required)
This PR adds functionality that allows an orchestrator to check whether a broadcaster is coming near to the end of its unlock period. If this is the case the orchestrator will not accept segments nor payments from that broadcaster.
This is accomplished by a
recipient.ValidateSender(sender ethcommon.Address) error
method that is called inOrchestrator.ProcessPayment()
. This method currently does a single check against an unlock period but can be extended with other functionality if needed.This structure allows us to leverage a
RoundsWatcher
andSenderWatcher
instance that is already active onSenderMonitor
. The methods forSenderMonitor
are (for good reason) not directly accessible on theRecipient
interface thatOrchestrator
uses, therefore the call graph iscore.Orchestrator.ProcessPayment -> Recipient.ValidateSender -> SenderMonitor.IsLocked
.Alternatively, we could have added a method to
Orchestrator
that accomplishes the same thing, but we would have had to addSenderWatcher
andRoundsWatcher
instances toLivepeerNode
specifically for this. It thus seems a cleaner tradeoff to have a "wrapper/forwarding" function inRecipient
.Specific updates (required)
Recipient.ValidateSender(sender ethcommon.Address) error
SenderMonitor.IsLocked(sender ethcommon.Address) error
Recipient.ValidateSender
inOrchestrator.ProcessPayment
, short circuit if the sender is nearing its unlock periodHow did you test each of these updates (required)
Ran unit tests
Does this pull request close any open issues?
Fixes #1183
Checklist:
./test.sh
pass