Random transaction generator #171

rawfalafel · 2018-06-12T05:57:38Z

This PR, which resolves #153, is ready for review. See my comments below for more details.

👋 ~~this PR isn't ready for review yet but I need help defining the requirements for this PR.~~

What kind of transactions should get sent through the feed? This branch just throws a random byte array into a types.Transaction object. Should the generator be smarter in order to target specific code paths on the other side of the event feed?

Also, should the generator be configurable? Would be nice if we could send transactions from geth console by having it read transactions from a JSON file. @rauljordan also mentioned making the intervals more random.

~~#125, #161, and possibly others could take advantage of a transaction generator but the requirements aren't clear to me, so I'm leaning towards keeping this simple and extensible for the next dev.~~

prestonvanloon · 2018-06-12T12:02:39Z

The purpose of this feature is to give the proposers something to propose while we flush out p2p.
It shouldn't be fancy because it's going to be deleted after we add transaction subscription from p2p.

rauljordan · 2018-06-12T16:27:05Z

Hi @rawfalafel the tx generator data isn't too important because for phase 1 of sharding, proposers will not care about the contents or "validity" of transactions and instead of just serialize them into blobs and package them into collation bodies.

If you are interested in extending past this simple PR into a more robust transaction generator, take a look at #17 which has been explored by @prestonvanloon a while back. The robust transaction generator would be more of a CLI tool or perhaps an additional service that can be registered to a sharding node that can be configurable and would be creating actual transactions from some sort of genesis state that we would configure in our own blockchain instance.

For this PR, however, feel free to keep it simple so we can get it merged soon. The item I outlined in the previous section is more of a bigger project that is outside of this scope.

rawfalafel · 2018-06-13T07:43:57Z

Thanks for the clarification @prestonvanloon @rauljordan!

This PR is ready to review. The logic is incredibly simple; send a random transaction through the event feed every 5 seconds, and instruct the proposer to subscribe to the feed and propose a collation using that transaction. I tested the goroutine by spinning up my own node.

rawfalafel · 2018-06-13T07:47:23Z

sharding/proposer/service.go

-		rand.Read(data)
-		txs = append(txs, types.NewTransaction(0, common.HexToAddress("0x0"),
-			nil, 0, nil, data))
+	for {


This function body here needs test coverage but I didn't see any examples for testing side effects in the sharding package. Any ideas?

Can you elaborate more? What makes this difficult to test?

I haven't looked at every test in the sharding package but most of them assert the return value of a function. For this method I'd like to stub createCollation and check that it gets called when a transaction gets sent to the transaction feed but I don't see any mocking libraries being used in any of the test files.

There's still no test coverage for this PR. Open to ideas on how to add test coverage? Although having no test coverage (for now) is fine if you consider how much the behavior here is going to change in the near future.

Yeah, I'm not sure. We are facing the same issue in other places.

I suppose you could call p.cancel after a short timeout, but that feels like a hacky solution.

Since this is temporary code, I'm ok without testing this for loop, but this is a pattern we are already having issues within non-temporary code. I will spend some more time thinking about how to test this today.

Oh wait this is the non-temporary code.

prestonvanloon · 2018-06-13T12:19:52Z

sharding/proposer/service.go

-		txs = append(txs, types.NewTransaction(0, common.HexToAddress("0x0"),
-			nil, 0, nil, data))
+	for {
+		timeout := time.NewTimer(10 * time.Second)


Please use context for timeouts
https://golang.org/pkg/context/#WithTimeout

Ah, okay. Bear with me because I'm still new to Go. Are you saying call context.WithTimeout here and pass the new context down to createCollation, or to simply replace time.NewTimer with this context method and discard the context.

Something like this:

for { ctx, cancel := context.WithTimeout(context.Background(), 10 * time.Second) defer cancel() select { case tx := <-requests: log.Info(fmt.Sprintf("Received transaction: %x", tx.Hash())) if err := p.createCollation(ctx, []*types.Transaction{tx}); err != nil { log.Error(fmt.Sprintf("Create collation failed: %v", err)) } case <-ctx.Done(): log.Error("Subscriber timed out") case err := <-p.txpoolSub.Err(): log.Error(fmt.Sprintf("Subscriber failed: %v", err)) cancel() return } }

prestonvanloon · 2018-06-13T12:48:57Z

sharding/proposer/service.go

-		rand.Read(data)
-		txs = append(txs, types.NewTransaction(0, common.HexToAddress("0x0"),
-			nil, 0, nil, data))
+	for {


Can you elaborate more? What makes this difficult to test?

rauljordan · 2018-06-13T14:14:21Z

sharding/txpool/service.go

+	for range p.ticker.C {
+		tx := createTestTransaction()
+		nsent := p.transactionsFeed.Send(tx)
+		log.Info(fmt.Sprintf("Sent transaction %x to %d subscribers", tx.Hash(), nsent))


tx.Hash().Hex()

Using the %x formatter seems more logical to me, although I'd be splitting hairs.

rauljordan · 2018-06-13T14:15:00Z

sharding/txpool/service.go

 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/sharding"
 )

 // ShardTXPool handles a transaction pool for a sharded system.
 type ShardTXPool struct {
-	p2p sharding.ShardP2P
+	p2p              sharding.ShardP2P


We decided on not using interfaces for things we most likely won't have a second implementation for. See #177 - just use *p2p.Server.

rauljordan · 2018-06-13T14:15:34Z

sharding/proposer/service.go

 func (p *Proposer) proposeCollations() {
+	requests := make(chan *types.Transaction)


Make sure you also defer close(requests)

rauljordan · 2018-06-13T14:16:28Z

sharding/proposer/service.go

@@ -23,6 +23,7 @@ type Proposer struct {
 	client       *mainchain.SMCClient
 	shardp2p     sharding.ShardP2P


See 2 comments below about discontinuing interfaces for things with a single implementation. Just use *p2p.Server and *txpool.ShardTXPool - #177.

terencechain

Just a few feedbacks. Thanks for doing this @rawfalafel!

terencechain · 2018-06-13T14:21:13Z

sharding/proposer/service.go

 	return nil
 }

+// proposeCollations listens to the transaction feed submits a collation when one is found
 func (p *Proposer) proposeCollations() {


Since this is similar to simulateNotaryRequests() for Notary. Would it be better to place proposeCollations() under /proposer/utils.go? And maybe name it to simulateProposeCollation?

Not against it, but I guess this method doesn't know that the transactions it's being fed are simulated -- ShardTxPool does.

terencechain · 2018-06-13T14:22:22Z

sharding/proposer/service.go

-		txs = append(txs, types.NewTransaction(0, common.HexToAddress("0x0"),
-			nil, 0, nil, data))
+	for {
+		timeout := time.NewTimer(10 * time.Second)


terencechain · 2018-06-13T14:24:42Z

sharding/proposer/service.go

+		select {
+		case tx := <-requests:
+			if err := p.createCollation([]*types.Transaction{tx}); err != nil {
+				log.Error(fmt.Sprintf("Create collation failed: %v", err))


I don't think we should be capitalizing first letter of error messages

These error logs (not error objects) seem to be capitalized consistently throughout the entire package.

Ah. I meant to not capitalize for nested error. But this is fine

terencechain · 2018-06-13T14:25:45Z

sharding/proposer/service.go

+			if err := p.createCollation([]*types.Transaction{tx}); err != nil {
+				log.Error(fmt.Sprintf("Create collation failed: %v", err))
+			}
+			log.Info(fmt.Sprintf("Received transaction: %x", tx.Hash()))


Maybe this line should be before createCollation?

terencechain · 2018-06-13T14:29:47Z

sharding/txpool/service.go

-	p2p sharding.ShardP2P
+	p2p              sharding.ShardP2P
+	transactionsFeed *event.Feed
+	ticker           *time.Ticker
 }

 // NewShardTXPool creates a new observer instance.


This comment is not right

terencechain · 2018-06-13T14:31:09Z

sharding/txpool/service.go

+	return p.transactionsFeed
+}
+
+// sendTestTransaction sends a transaction with random bytes over a 5 second interval


Period end function comments

nisdas · 2018-06-13T14:44:42Z

sharding/proposer/proposer.go

@@ -15,6 +15,7 @@ import (
 // and body. Header consists of shardID, ChunkRoot, period,
 // proposer addr and signatures. Body contains serialized blob
 // of a collations transactions.
+


Remove this newline

rawfalafel · 2018-06-17T05:20:44Z

This PR is ready for another look!

rauljordan · 2018-06-17T14:33:54Z

sharding/proposer/service.go

+	p.txpoolSub = p.txpool.TransactionsFeed().Subscribe(requests)
+
+	for {
+		ctx, cancel := context.WithTimeout(p.ctx, 10*time.Second)


Why do you need to define a new context here? Let's just use p.ctx.Done() in the select block. Also, this will cancel the goroutine right after the select block...we instead want this to run forever.

I derived a new context here to mimic the behavior here. If a single loop takes more than 10 seconds, whatever process is running gets canceled and the loop restarts. On the other hand if the parent context gets canceled we want the goroutine to be exit entirely.

This looks good to me.

rauljordan · 2018-06-17T14:34:07Z

sharding/proposer/service.go

+				log.Error(fmt.Sprintf("Create collation failed: %v", err))
+			}
+		case <-ctx.Done():
+			log.Info("Context canceled")


add a return statement after this line to stop the goroutine

+1 to return statement.
Remove the logging, it's not necessary (Or make it a debug level)

rauljordan · 2018-06-17T14:34:28Z

sharding/proposer/service.go

-		txs = append(txs, types.NewTransaction(0, common.HexToAddress("0x0"),
-			nil, 0, nil, data))
+		cancel()
+		return


Eliminate these two lines. Cancel will be called only if the service stops in the Stop() function.

I called the derived context's cancel to let any goroutine that might have spawned within the for loop to abandon their work. The return doesn't belong here though.

rawfalafel · 2018-06-17T15:27:01Z

I'm going to take another look at how contexts work tomorrow morning before making changes.

terencechain · 2018-06-17T16:36:46Z

sharding/proposer/service.go

 	// Get current block number.
-	blockNumber, err := p.client.ChainReader().BlockByNumber(context.Background(), nil)
+	blockNumber, err := p.client.ChainReader().BlockByNumber(ctx, nil)


why not use p.ctx instead of passing ctx down?

So that BlockByNumber could be given the opportunity to exit immediately if it hit ctx's 10 second timeout. But I removed the derived context now so this is passing down p.ctx again.

terencechain · 2018-06-17T16:37:31Z

sharding/proposer/service.go

+				log.Error(fmt.Sprintf("Create collation failed: %v", err))
+			}
+		case <-ctx.Done():
+			log.Info("Context canceled")


This log.Info can be more descriptive

rawfalafel · 2018-06-18T05:34:22Z

I poked around at the behavior of the context derived by context.WithTimeout and decided to remove it because there were some unintended race conditions that caused createCollation to sometimes fail on startup. Since the timeout in the for loop isn't required behavior (as far as I know), it'd be simpler to just leave it out and add it back in if it's necessary.

This is ready for another look!

Sharding: proof of custody performance improvements

…more testable and add more unit tests

rawfalafel · 2018-06-19T14:50:21Z

I pushed up a commit that adds tests to the proposer service by refactoring methods in the package. I found that because of the limitations to mocking smcClient, I passed in the mainchain interfaces to the goroutine function directly in order to test it. I also found that passing in the channels that the goroutine switched on made testing those cases easier as well.

Since adding tests led to unwanted scope creep, I'm down with reverting the last commit and merging this in without test coverage.

rauljordan · 2018-06-19T20:06:26Z

Hey @rawfalafel let's revert and do the test coverage in a separate PR

rawfalafel · 2018-06-19T23:45:14Z

reverted

…nctions more testable and add more unit tests" This reverts commit e7608d4.

terencechain

LGTM

terencechain · 2018-06-19T13:06:56Z

sharding/proposer/proposer.go

 )

+func getPeriod(reader mainchain.Reader, periodLength *big.Int) (*big.Int, error) {


should we move it on the SMC client level so getPeriod can be used for notary as well? if not, please add a TODO to make it a good first issue

pointing out, just in case, but this is from the reverted code

terencechain · 2018-06-19T13:18:23Z

sharding/proposer/proposer.go

+	return new(big.Int).Div(blockNumber.Number(), periodLength), nil
+}
+
+func submitCollationHeader(txs []*types.Transaction, period *big.Int, shardID *big.Int, account *accounts.Account,


can you add a quick function comment here for submitCollationHeader?

sharding: random transaction generator

sharding: random transaction generator Former-commit-id: 69b7da1

sharding: random transaction generator Former-commit-id: b41891c474372ab54cbf1e3fa34dc7399f42c6d4 [formerly 69b7da1] Former-commit-id: b642be5

rauljordan and others added 9 commits June 11, 2018 19:48

sharding: subscribe transactions

08fcd42

Refactoring of subscription channels

8050214

Add error handling case for feed subscription

6729e29

Add simple logic to send transactions to txPool feed

c5fd161

Merge remote-tracking branch 'origin/master' into tx-generator

45cd809

small fixes

0df1e00

gofmt fixes

7c2fab9

Add transactionFeed back into txpool interface

5622ebd

linting

d71c434

nisdas added In Progress labels Jun 12, 2018

Yutaro Mori added 6 commits June 13, 2018 14:51

merge origin/master

473a771

Cleanup proposer service

77abdac

fix tests

d7ea292

fmt fixes

3276a6a

more gofmt tweaks

e5c3758

goimport fixes

01c19be

rawfalafel commented Jun 13, 2018

View reviewed changes

prestonvanloon added this to the Ruby milestone Jun 13, 2018

prestonvanloon reviewed Jun 13, 2018

View reviewed changes

rauljordan requested changes Jun 13, 2018

View reviewed changes

terencechain requested changes Jun 13, 2018

View reviewed changes

nisdas reviewed Jun 13, 2018

View reviewed changes

Yutaro Mori added 2 commits June 14, 2018 09:50

PR comments

a891f24

linting

894ee4d

Yutaro Mori added 2 commits June 17, 2018 13:55

gofmt fixes

f192e3b

Derive a new context within the proposer service's goroutine

a32a240

rauljordan requested changes Jun 17, 2018

View reviewed changes

terencechain reviewed Jun 17, 2018

View reviewed changes

Yutaro Mori added 2 commits June 18, 2018 14:21

Remove derived WithTimeout context in proposer service

a9312b3

Merge remote-tracking branch 'origin/master' into tx-generator

6b7ea3a

Yutaro Mori and others added 2 commits June 19, 2018 20:18

perf improvements to CalculatePOC (prysmaticlabs#182)

0c6a1af

Sharding: proof of custody performance improvements

Refactor proposer package to make goroutine and individual functions …

e7608d4

…more testable and add more unit tests

rawfalafel force-pushed the tx-generator branch from 23d8558 to 5fde6ed Compare June 19, 2018 14:40

rauljordan mentioned this pull request Jun 19, 2018

Implement Sharded TxPool #161

Closed

Yutaro Mori added 2 commits June 20, 2018 10:38

Revert "Refactor proposer package to make goroutine and individual fu…

d9c0e80

…nctions more testable and add more unit tests" This reverts commit e7608d4.

merge origin/master

9eaa6e9

rawfalafel force-pushed the tx-generator branch from 61454b5 to 9eaa6e9 Compare June 20, 2018 01:45

rauljordan approved these changes Jun 20, 2018

View reviewed changes

empty commit

21a22da

terencechain approved these changes Jun 20, 2018

View reviewed changes

Merge branch 'master' into tx-generator

45111b5

terencechain merged commit 55e9cc4 into prysmaticlabs:master Jun 20, 2018

rauljordan mentioned this pull request Jun 20, 2018

Turn Simulator Package Into an Actor #195

Closed

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018

Random transaction generator (#171)

69b7da1

sharding: random transaction generator

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018

Random transaction generator (#171)

b642be5

sharding: random transaction generator Former-commit-id: 69b7da1

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018

Random transaction generator (#171)

346b9ae

sharding: random transaction generator Former-commit-id: b41891c474372ab54cbf1e3fa34dc7399f42c6d4 [formerly 69b7da1] Former-commit-id: b642be5

		func (p *Proposer) proposeCollations() {
		requests := make(chan *types.Transaction)

		@@ -23,6 +23,7 @@ type Proposer struct {
		client *mainchain.SMCClient
		shardp2p sharding.ShardP2P

		)

		func getPeriod(reader mainchain.Reader, periodLength big.Int) (big.Int, error) {

Random transaction generator #171

Random transaction generator #171

Conversation

rawfalafel commented Jun 12, 2018 • edited Loading

prestonvanloon commented Jun 12, 2018

rauljordan commented Jun 12, 2018

rawfalafel commented Jun 13, 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

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

terencechain 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

rawfalafel commented Jun 17, 2018

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

rawfalafel commented Jun 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawfalafel commented Jun 18, 2018

rawfalafel commented Jun 19, 2018

rauljordan commented Jun 19, 2018

rawfalafel commented Jun 19, 2018

terencechain 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

rawfalafel commented Jun 12, 2018 •

edited

Loading

rawfalafel commented Jun 13, 2018 •

edited

Loading