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

core,net,pm,server: O sends expected TicketExpirationParams to B along with TicketParams #1428

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

kyriediculous
Copy link
Contributor

What does this pull request do? Explain your changes. (required)
The orchestrator sends along TicketExpirationParams as ExpectedExpirationParams with TicketParams. The broadcaster then uses these values for the ticket CreationRound and AuxData instead of relying on the TimeWatcher (view of the current chain for B). Both CreationRound and CreationRoundBlockHash are added to the HMAC scheme to validate these values were advertised by the Orchestrator.

Two considerable benefits:

  • It removes the synchronization requirement of the Ethereum network between O and B for payment creation. This was a dependency I was quite uncomfortable with. Instead it is now the responsability of the Orchestrator to provide correct values that result in redeemable tickets.
  • We no longer encounter unwanted "invalid ticket creation round" errors during round transitions that might interrupt transcoding

Downsides:

  • Increase the size of the OrchestratorInfo message with a couple of bytes
  • Likely due to increase of size of the message for the HMAC generation this will become marginally slower (not sure here, not that familiar with the scheme but seems like a logical assumption).

Specific updates (required)

  • Embedded TicketExpirationParams in TicketParams protobuf message
  • Add pm.TicketExpirationParams field to pm.TicketParams
  • Recipient reads last initialized round and its blockhash from memory when recipient.TicketParams() is called and the values are used for the ExpectedExpirationParams to send to B
  • Add CreationRound and CreationRoundBlockHash to recipientRand generation and thus ticket validation
  • Remove validateCreationRound() and its usage in ticket validation
  • B uses the ExpectedExpirationParams provided in the TicketParams from O to populate the values for CreationRound and CreationRoundBlockHash when creating tickets

How did you test each of these updates (required)
Added unit tests
Ran nodes with devtool

Does this pull request close any open issues?
Fixes #1187

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the pmTicketParams() helper in db_discovery.go doesn't need to populate the expiration params field since those fields are not accessed within the file at the moment, it might be a good idea to populate the field anyways for consistency.

pm/recipient.go Outdated Show resolved Hide resolved
pm/recipient.go Outdated Show resolved Hide resolved
pm/sender.go Outdated Show resolved Hide resolved
pm/ticket.go Outdated Show resolved Hide resolved
@yondonfu
Copy link
Member

Some thoughts on implications of this change below:

Since B always uses the creation round (and block hash - when I say creation round in this comment I also refer to block hash associated with the creation round) that it receives, O is able to set the creation round in the range [currentRound - 2, currentRound] where 2 is the current on-chain ticket validity period.

Why would O set the creation round to anything less than the current round? One possible reason is to control the fee pool that fees are sent to. Recall that an O has a fee pool for each round and fees sent to a fee pool for round N are distributed to delegators that were staked to O in round N. In this case, O can choose to send fees to the fee pool for any round in the range [currentRound - 2, currentRound]. O could benefit from this ability if it notices that delegators have already claimed their fees for one of rounds in this range - additional fees sent to the pool at this point would be distributed amongst delegators (including the O itself) that have not yet claimed from those pools.

I don't think this is a big problem in practice because:

  • Delegators typically do not need to claim earnings unless they have > 100 rounds of earnings to claim and they need to perform a bonding action. So, if delegators claim earnings infrequently and only when absolutely needed they will reduce the likelihood of "leaving fees on the table".
  • O controls the creation round, but does not control when tickets will win so it is difficult to opportunistically send fees to fee pools for past rounds based on observed delegator behavior.

@kyriediculous
Copy link
Contributor Author

Good points you raise, I didn't consider those.

But I think it would also require the O to not call reward itself then, otherwise it would have already called it for that EarningsPool, which would be visible in the explorer and might deter delegates as well. Plus there is always the possibility of an orchestrator trying such shenanigans coming under community scrutiny.

fixups in cb323b0

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the pmTicketParams() helper in db_discovery.go doesn't need to populate the expiration params field since those fields are not accessed within the file at the moment, it might be a good idea to populate the field anyways for consistency.

Bump on the above comment.

pm/recipient_test.go Outdated Show resolved Hide resolved
pm/sender.go Outdated Show resolved Hide resolved
pm/ticket.go Outdated Show resolved Hide resolved
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good - let's rebase.

@kyriediculous
Copy link
Contributor Author

rebased !

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid ticket creation round when starting a broadcaster after being offline for awhile
2 participants