Mempool concurrency: transaction validation #103

kirushanths · 2019-02-24T00:00:55Z

…pool-validate

kirushanths · 2019-02-24T00:06:51Z

pkg/service/mempool.go

+                err = m.VerifyDepositTransaction(&tx)
+        } else {
+                // 300-500us
+                err = m.VerifySpendTransaction(&tx)


Once you move out the mempool.poolSpends checks out of this method and it just becomes validation.ValidateSpendTransaction (which only checks the previous block in storage and does math only, it basically can be parallelized).

I know you said the storage.isDoubleSpent can not be parallelized but exactly why? it's checking the previous persisted block only, which won't change until mempool gets flushed. And Mempool basically contains transactions for the next block and having the ensurePoolSpends separated on a single thread (under the new case req := <-m.validatedTxReqs:) ensures no double-spend happens in mempool.

In summary: storage doubleSpend check can be parallelized cause it's past history and static but maintaining mempool double-spend integrity on a single thread keeps the correctness.

Thoughts?

Second thoughts on this: realized that if a block is about to be written with transaction X (via flush), but a double-spend transaction X' is in a go-routine just past this line, it's possible for a double-spend to sneak in.

Basically mempool validation can't occur while a block is about to be written.

So you're right - I will need to re-think a little bit more

Correct, mempool validation cannot occur while a block is about to be/is being written. That's handled by the flushReq case in the mempool loop - all transaction processing halts until the block is successfully written to disk.

When I said the double spend check can't be parallelized I was referring to how it requires that blocks be written to disk before new transactions can be processed by the mempool.

Yeah but I think there is a race-condition introduced here. As described in my last comment: https://github.com/kyokan/plasma/pull/103/files#r259601343.

The transaction validation can occur simultaneously as a package block write. Basically we should wrap the go-routine in a lock.

kirushanths · 2019-02-24T00:07:29Z

pkg/eth/signatures.go

 	sigCopy := make([]byte, len(signature))
 	copy(sigCopy, signature)
 	if len(sigCopy) == 65 && sigCopy[64] > 26 {
 		sigCopy[64] -= 27
 	}

+        // 220 Us
 	pubKey, err := crypto.SigToPub(ethHash, sigCopy)


Basically 70% of the work happens right here on this line.

If you do the rough math here: If this takes say on average 200 us. That means about 5 transactions processed a millisecond. And in a 100ms block with 16-cores (my setup) parallelizing the processing you get: 5 transactions/ms x 100 ms/block x 16cores = 8000 TPS. I'm basically this figure plus/minus overhead.

EDIT: Sorry my math was horrible. And this line doesn't average near 200us. It's much lower but it's still the main bottleneck during transaction validation and eating 70% of the time. This is pre-block-packaging.

kirushanths · 2019-02-24T00:08:44Z

pkg/service/mempool.go

-				}
-				if err != nil {
+                                go m.validateTransactionRequest(req)
+                        case req := <-m.validatedTxReqs:


We validate the incoming transaction request first against previous block history for double-spends and once that's cleared we do our mem-pool integrity checks synchronously below.

I like this approach.

kirushanths · 2019-02-24T00:21:41Z

Also if you can configure CircleCI to run all commits (including forked PRs) would be great.

kirushanths · 2019-02-24T00:25:29Z

pkg/validation/validation.go

 	prevTx0Output := prevTx0.Body.OutputAt(tx.Body.Input0.OutputIndex)
 	err = eth.ValidateSignature(sigHash0, tx.Sigs[0][:], prevTx0Output.Owner)
 	if err != nil {
 		return NewErrInvalidSignature(0)
 	}

+        // 450 ns
 	totalInput := big.NewInt(0)
 	totalInput = totalInput.Add(totalInput, prevTx0Output.Amount)



Another thing: if !tx.Body.Input1.IsZero() never gets tripped which will essentially could double the time because eth.valdiateSignature gets called twice.

What do you mean never gets tripped? You mean if Input1 is defined?

Yeah if input1 is never defined. which is the case in our benchmark setup

mslipper

This is fine - doesn't seem to bloat complexity too much. Could you post the benchmark differences here, and write a quick test? I know that this file was untested before, but it's on my list of services to write tests for. Specifically I'm looking for the output of a test that adds transactions to the pool concurrently with the --race option enabled, which will spit out race conditions.

mslipper · 2019-02-24T07:22:08Z

pkg/validation/validation.go

 	prevTx0Output := prevTx0.Body.OutputAt(tx.Body.Input0.OutputIndex)
 	err = eth.ValidateSignature(sigHash0, tx.Sigs[0][:], prevTx0Output.Owner)
 	if err != nil {
 		return NewErrInvalidSignature(0)
 	}

+        // 450 ns
 	totalInput := big.NewInt(0)
 	totalInput = totalInput.Add(totalInput, prevTx0Output.Amount)



What do you mean never gets tripped? You mean if Input1 is defined?

mslipper · 2019-02-24T07:24:58Z

pkg/service/mempool.go

+                err = m.VerifyDepositTransaction(&tx)
+        } else {
+                // 300-500us
+                err = m.VerifySpendTransaction(&tx)


Correct, mempool validation cannot occur while a block is about to be/is being written. That's handled by the flushReq case in the mempool loop - all transaction processing halts until the block is successfully written to disk.

mslipper · 2019-02-24T07:26:14Z

pkg/service/mempool.go

-				}
-				if err != nil {
+                                go m.validateTransactionRequest(req)
+                        case req := <-m.validatedTxReqs:


I like this approach.

mslipper · 2019-02-24T07:27:56Z

pkg/service/mempool.go

+                err = m.VerifyDepositTransaction(&tx)
+        } else {
+                // 300-500us
+                err = m.VerifySpendTransaction(&tx)


When I said the double spend check can't be parallelized I was referring to how it requires that blocks be written to disk before new transactions can be processed by the mempool.

mslipper · 2019-02-24T07:32:17Z

Going to try reopening to kick CI.

mslipper · 2019-02-24T07:32:27Z

There it goes.

kirushanths added 2 commits February 23, 2019 23:58

comment runtimes; parallelize transaction verification in mempool

4847990

Merge branch 'master' of https://github.com/kyokan/plasma into ks-mem…

c72f023

…pool-validate

kirushanths commented Feb 24, 2019

View reviewed changes

mslipper approved these changes Feb 24, 2019

View reviewed changes

mslipper closed this Feb 24, 2019

mslipper reopened this Feb 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mempool concurrency: transaction validation #103

Mempool concurrency: transaction validation #103

kirushanths commented Feb 24, 2019

kirushanths Feb 24, 2019

kirushanths Feb 24, 2019

mslipper Feb 24, 2019

mslipper Feb 24, 2019

kirushanths Feb 24, 2019

kirushanths Feb 24, 2019

kirushanths Feb 24, 2019 •

edited

Loading

kirushanths Feb 24, 2019

mslipper Feb 24, 2019

kirushanths commented Feb 24, 2019

kirushanths Feb 24, 2019

mslipper Feb 24, 2019

kirushanths Feb 24, 2019

mslipper left a comment

mslipper Feb 24, 2019

mslipper Feb 24, 2019

mslipper Feb 24, 2019

mslipper Feb 24, 2019

mslipper commented Feb 24, 2019

mslipper commented Feb 24, 2019

Mempool concurrency: transaction validation #103

Are you sure you want to change the base?

Mempool concurrency: transaction validation #103

Conversation

kirushanths commented Feb 24, 2019

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

kirushanths Feb 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirushanths commented Feb 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mslipper 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

mslipper commented Feb 24, 2019

mslipper commented Feb 24, 2019

kirushanths Feb 24, 2019 •

edited

Loading