Persist Winning Tickets #655

eladmallel · 2018-12-27T20:24:34Z

What does this pull request do? Explain your changes. (required)
Supports the storage of winning PM tickets into the node's local database.

Specific updates (required)

A new winningTickets table in the local database
db.go implements the TicketStore interface
A new testing dependency for succinct assertions (and more!)
Had to change SenderNonce to uint32 since the SQL library doesn't support very high values of uint64
Refactor to abstract away the AccountManager dependency such that the pm package no longer imports anything from the eth package - this was critical to resolve a cyclical dependency issue (since common now imports pm for the pm.Ticket type in the new function signatures)

How did you test each of these updates (required)

Unit tests that interact with an actual test DB instance

Does this pull request close any open issues?

Fixes pm: Store winning tickets on-disk with corresponding recipientRand value #621

Checklist:

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

Committing work in progress to get some quick feedback.

Got feedback that `UNSIGNED BIG INT` adds more confusion than clarity.

We want to be able to query all winning tickets received in the same PM session. In the current Streamflow iteration we will attempt to redeem winning tickets only once the video stream is over, which practically means we will keep the same PM session for the entire video stream, and it's likely O will receive multiple winning tickets in this model. Therefore, we need to allow O to redeem all winning tickets from a specific session (we can't just redeem all the tickets in the table because O might have other streams in progress). While in practice `sessionID` is `recipientRand`, we are choosing to store it as a separate field to decouple O's Livepeer-specific code from the internals of PM. This allows for a more generic and reusable PM implementation, and maintaining a more stable O should any internal PM changes happen.

This removes the dependency on our `eth` package which later allows us to reference the `pm` package in `db.go`.

These longer names allow us to implement this interface directly in our DB type, since their previous names were too short to fit as part of our large DB type with all the other functions that load and store multiple data types.

This is a refactor of the work previously started in DB to support writing and reading winning tickets, with the additional step forward of deciding that `DB` will directly implement `TicketStore` such that when we write code that integrates `pm` with the rest of the code, `DB` will be the concrete implementation behind the `TicketStore` we will inject into `Recipient` and other `pm` types that might depend on `TicketStore`.

Got some basic tests for storing and loading tickets. Wanted to push this up before tackling some edge cases. I already know this code doesn't work when `SenderNonce` is set to uint64 max value, but decided to include a fix for that in the next commit.

During testing of max values in storing winning tickets in the DB we learned that we get a SQL library error trying to store uint64 max values, so we chose the easy workaround of changing SenderNonce's range. With unsigned 32 bits we still have 4.3B nonces, which if you assume the case of one PM ticket per second still leaves us with a 136 years of video we can stream on a single `recipientRand` value, so I believe it should hold for now!

In order to fully reconstruct a ticket from DB without having the DB know the implementation of `recipientRandHash`.

eladmallel · 2018-12-27T20:41:43Z

@j0sh would love your eyes on this one in particular since we're trying out a new package for testing!

yondonfu

Looking good. Left a few minor comments. My main question is regarding the usage of the new testify package in tests - should we be consistent with our usage of assert and require over their vanilla Go test counterparts i.e. t.Error and t.Fatal?

common/db.go

yondonfu · 2018-12-27T20:54:11Z

common/db_test.go

+	defer dbraw.Close()
+	if err != nil {
+		t.Error(err)
+		return


Why do we use an explicit return here? Does t.Fatal() not return cleanly for this function thereby messing with the defer statements closing the DB handler (although I do see that we are still using t.Fatal() elsewhere in this test file)?

I will change this to use the new require package - hope that will do the trick!

yondonfu · 2018-12-27T20:58:50Z

common/db_test.go

+	sessionID, ticket, sig, recipientRand := defaultWinningTicket(t)
+	err = dbh.StoreWinningTicket(sessionID, ticket, sig, recipientRand)
+	if err != nil {
+		t.Fatalf("unexpected errro storing ticket: %v", err)


"unexpected errro" -> "unexpected error"?

Just went ahead and deleted the error messages since the behavior of require and assert is to print out the failure line, and I think that's good enough for debugging!

yondonfu · 2018-12-27T21:01:03Z

common/db_test.go

+	return count
+}
+
+func randAddressOrFatal(t *testing.T) ethcommon.Address {


Should we move some of these test helpers i.e. randBytesOrFatal, randBytes, etc. into a single place to avoid duplication since pm/helpers.go already contains these functions?

yondonfu · 2018-12-27T21:14:59Z

common/db_test.go

+
+	err = dbh.StoreWinningTicket(sessionID, ticket, sig, recipientRand)
+
+	if err != nil {


Could we do the following:

if assert.Nilf(err, "failed to insert winning ticket with error: %v", err) { ... assertions assuming that winning ticket insertion succeeded .... }

I'm just switching to require.Nil(err) for all the instances where we would use t.Fatalf

common/db_test.go

yondonfu · 2018-12-27T21:32:23Z

common/db_test.go

+	assert.Len(recipientRands, 0)
+}
+
+func TestLoadtWinningTicket_GivenEmptySessionID_ReturnsEmptySlicesNoError(t *testing.T) {


If we ignore that fact that a pm recipient will use recipientRandHash as the sessionID, technically the DB could accept an empty string as the sessionID and return non-empty slices right?

You're right. In this case I was aiming at making sure that we just get an empty result set rather than an error. I didn't find it interesting to add a test where there would be tickets stored with sessionID = "", since I believe the combination of the tests we already have covers it. But please let me know if you think we need this additional test case and I'll add it :)

👍 I don't think an additional test case is necessary - I just wanted to clarify that an empty sessionID should return an empty slice in this test case, but does not necessarily always return an empty slice

common/db_test.go

yondonfu · 2018-12-27T21:47:23Z

pm/stub.go

-
-func (am *stubAccountManager) SignTx(signer types.Signer, tx *types.Transaction) (*types.Transaction, error) {
+// TODO remove this function
+// NOTE: Keeping this function for now because removing it causes the tests to fail when run with the


yondonfu · 2018-12-28T00:12:41Z

pm/helpers.go

@@ -7,7 +7,7 @@ import (
 	ethcommon "github.com/ethereum/go-ethereum/common"
 )

-func randHashOrFatal(t *testing.T) ethcommon.Hash {
+func RandHashOrFatal(t *testing.T) ethcommon.Hash {


Feels like these functions should actually live in a common test utils file since they aren't really specific to pm. Unfortunately, I think moving these functions to the common package might introduce a cycle since the DB code is also in the common package? Perhaps if we move the DB code into its own package then we can move these functions into the common package - I don't think we have to do this in this PR though

Yes, that was exactly my thinking - that all these test helpers should go under common but to do that we must solve the cyclical dependency issue.

Another approach we could take is have a package dedicated to test helpers we could name testutils or something similar.

Wdyt?

I've also addressed this issue in the past by wrapping types into DB-specific structs, with external helpers to convert between the DB struct and the original. eg, pm.Ticket -> common.DBTicket and so forth.

This has certain benefits; we can incorporate "extra" information that exists in the DB that may not necessarily belong in the PM ticket struct, such as its submission timestamp, tx hash and other transaction status info, block confirmation number, any other fields that may have a slightly different representation, etc.

@j0sh how does a DBTicket relate to the conversation about refactoring these helper functions to a more common package? - I think I get the future benefits of having such a type, just trying to connect the dots to this specific thread

Rather than have common import pm, we can have common export a DBTicket which is converted into a pm.Ticket somewhere else, eg in the PM code. That would allow us to keep common helpers in common and avoid an import cycle.

In the case of tickets, there is an additional annoyance in that pm.Recipient.ReceiveTicket() will use the method StoreWinningTicket(string, *Ticket, []byte, *big.Int) defined in the TicketStore interface to persist winning tickets. common.DB currently implements TicketStore. We can avoid importing pm in common by using common.DBTicket in the TicketStore interface: StoreWinningTicket(string, *common.DBTicket, []byte, *big.Int). But, now we're importing code specific to common.DB into pm when it would be preferable to keep pm agnostic to how persistence is handled.

One solution is to destructure the ticket fields in the TicketStore method call:

StoreWinningTicket(string, ethcommon.Address, ethcommon.Address ... rest of ticket fields ..., []byte, *big.Int)

Neither package would have to import the either. But this leads to a very unwieldy method call with many parameters - not great.

Alternatively, we could use an anonymous struct - the method signature would look ugly, but the method calls would be cleaner.

Another solution is to move the Ticket struct into the common package (or whatever the name is for the package we use to store common types used throughout go-livepeer). But this creates more external dependencies for the pm package when it would preferable to keep it self contained.

Extracting DB into its own package that imports pm and having the common package specifically focused on just helper functions seems cleaner than the above solutions.

But, now we're importing code specific to common.DB into pm when it would be preferable to keep pm agnostic to how persistence is handled.

The PM code right now is quite nicely cleaved with minimal glue needed between the DB and PM itself. However, I do wonder how hard we'll need to work to maintain this clean separation in the future.

One difficulty with the current approach is that nearly all modules could have a legitimate reason to use the DB [1], but if the DB module imports everything, none of those modules can actually invoke the DB itself. That means we need a third module to glue together the DB and the module, or to implement more of the "glue code" within the DB module itself, which bleeds over the concerns.

We could define a generic Ticket as an interface to avoid embedding any DB-specific constructs within the TicketStore interface (perhaps with a single PMTicketToGenericTicket function for the Ticket interface). However, I don't think there is any harm in putting DB-aware code within the PM implementation itself, as long as the interface remains storage-agnostic. We may eventually find it difficult to keep the concrete implementation completely agnostic to the DB.

[1] That most modules could make use of the DB is one reason I like to consider the DB common code, and by keeping it narrowly scoped to processing queries (eg, minimal glue), there is less reason to move the DB into its own module.

Another solution is to move the Ticket struct into the common package (or whatever the name is for the package we use to store common types used throughout go-livepeer). But this creates more external dependencies for the pm package when it would preferable to keep it self contained.

Presumably pm would still be importing common helpers so it wouldn't be a technical obstacle to defining the Ticket struct within common. However, I understand the incongruity of separating the definition from the actual use.

yondonfu

LGTM 🚢 Loving the concise tests using testify

j0sh · 2019-01-03T01:45:17Z

common/db.go

+		recipientRand BLOB,
+		recipientRandHash STRING,
+		sig BLOB,
+		sessionID STRING


Generally I like to incorporate a creation timestamp here as well. Useful for forensic purposes and doesn't affect any queries with a DEFAULT CURRENT_TIMESTAMP. Also, some constraints might have been nice such as a composite key consisting of(recipientRand, senderNonce) which would have ensured uniqueness of the pair.

Haven't fully read through the approach here yet, but I've used DB constraints as one form of checking to ensure subsequent code-paths are only executed once (eg, preventing double-submits to the blockchain).

I'm more than happy to add a creation timestamp! Created this issue #661.

As for the constraints - the reason I did not add them is because they weren't necessary as of yet, and I don't like adding "future code" that isn't being used when it's added. I really can't see in what scenario we would benefit from this constraint since I don't see a code path that would lead to our saving the same winning ticket twice (since in-memory protections are in place to reject a repeat incoming ticket). So on this one I'm still leaning more towards not adding anything that isn't really needed, but would love to hear your further thoughts to learn more about why you think it's good to add?

As for double-submit prevention - I agree it's a good idea to defend against that, and think we can do that well by adding some ETH-related columns when we get to spending more time on a robust redemption mechanism. I think we will add these DB columns as we solve for this issue #632. wdyt?

why you think it's good to add?

The ability to declare constraints among fields and key relationships is probably my favorite feature of relational DBs. It's a type system that helps ensure the "shape" (consistency and integrity) of the data during storage and retrieval.

I really can't see in what scenario we would benefit from this constraint since I don't see a code path that would lead to our saving the same winning ticket twice (since in-memory protections are in place to reject a repeat incoming ticket)

We may have in-memory protections now, but any constraints act as a safety net in case these are missed somehow. Actually, I have been even lazier in the past: I've used the DB in lieu of in-memory protection, because why write additional code when your logic can be enforced declaratively?

Anyway, just my two cents! This is still fine as-is.

eladmallel added 13 commits December 26, 2018 14:33

PM: WIP saving winning tickets to DB

85bbdd8

Committing work in progress to get some quick feedback.

PM: cleanup in DB test

ea02ecb

PM: winningTickets schema cleanup

6283485

Got feedback that `UNSIGNED BIG INT` adds more confusion than clarity.

PM: Sender refactor away from AccountManager

f610f57

This removes the dependency on our `eth` package which later allows us to reference the `pm` package in `db.go`.

PM: TicketStore functions rename

987d0db

These longer names allow us to implement this interface directly in our DB type, since their previous names were too short to fit as part of our large DB type with all the other functions that load and store multiple data types.

PM: save recipientRandHash to winningTickets table

ed4626d

In order to fully reconstruct a ticket from DB without having the DB know the implementation of `recipientRandHash`.

PM: Wrapping DB errors for better debugging UX

304faf3

PM: using assert lib in DB tests

a8d58a3

PM: DB winningTickets edge case handling

ddb25be

eladmallel requested review from yondonfu and j0sh December 27, 2018 20:27

eladmallel added 2 commits December 27, 2018 15:33

Merge branch 'master' into em/persist-winning-tickets-2

dd95902

PM: build fix

317e4fc

yondonfu requested changes Dec 27, 2018

View reviewed changes

PM: PR comments cleanup

b51db07

yondonfu reviewed Dec 28, 2018

View reviewed changes

yondonfu approved these changes Dec 28, 2018

View reviewed changes

eladmallel merged commit 05a497c into master Dec 28, 2018

eladmallel deleted the em/persist-winning-tickets-2 branch December 28, 2018 14:47

j0sh reviewed Jan 3, 2019

View reviewed changes

j0sh mentioned this pull request Jan 3, 2019

PM Sender #651

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist Winning Tickets #655

Persist Winning Tickets #655

eladmallel commented Dec 27, 2018 •

edited

Loading

eladmallel commented Dec 27, 2018

yondonfu left a comment

yondonfu Dec 27, 2018

eladmallel Dec 27, 2018

yondonfu Dec 27, 2018

eladmallel Dec 27, 2018

yondonfu Dec 27, 2018

yondonfu Dec 27, 2018

eladmallel Dec 27, 2018

yondonfu Dec 27, 2018

eladmallel Dec 27, 2018

yondonfu Dec 27, 2018

yondonfu Dec 27, 2018

yondonfu Dec 28, 2018 •

edited

Loading

eladmallel Dec 28, 2018

j0sh Jan 3, 2019

eladmallel Jan 3, 2019

j0sh Jan 3, 2019

yondonfu Jan 3, 2019

j0sh Jan 3, 2019

yondonfu left a comment

j0sh Jan 3, 2019

eladmallel Jan 3, 2019

j0sh Jan 3, 2019


		err = dbh.StoreWinningTicket(sessionID, ticket, sig, recipientRand)

		if err != nil {

Persist Winning Tickets #655

Persist Winning Tickets #655

Conversation

eladmallel commented Dec 27, 2018 • edited Loading

eladmallel commented Dec 27, 2018

yondonfu 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

yondonfu Dec 28, 2018 • edited Loading

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 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

eladmallel commented Dec 27, 2018 •

edited

Loading

yondonfu Dec 28, 2018 •

edited

Loading