Periodic winning ticket redemption transaction failure

[yondonfu]

Describe the bug
A clear and concise description of what the bug is.

I observed periodic winning ticket redemption transaction failure when doing e2e testing with a test-harness deployment. Some of the redemption transactions succeeded while others failed.

To Reproduce
Steps to reproduce the behavior:

1. Clone the latest master branch for the test-harness
2. In the go-livepeer codebase, change this line to 900000000.
3. Run make localdocker in the go-livepeer directory
4. In the test-harness directory, run node examples/local.js and wait for the setup to complete
5. Make sure you have a geth binary installed locally
6. Run geth attach http://localhost:8545
7. In the interactive console, run:

web3.eth.sendTransaction({from: web3.eth.accounts[0], to: web3.eth.accounts[0], value: 0, gasPrice: 400})

multiple times.
8. Watch the logs of the orchestrator nodes until you observe a message that indicates that the new gas price of 400 has been cached
9. Start a stream by running ./test-harness stream test123
10. Watch the logs of the orchestrators nodes and observe some winning ticket redemption transactions succeeding and some failing

Expected behavior
A clear and concise description of what you expected to happen.

Winning ticket redemption transactions should not fail.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: OSX
• Version: 0c1a6d0 (this is the starting HEAD commit, but see the reproduction steps for a note on the modification made to the code before building)

[kyriediculous]

After letting the transactions actually be submitted on-chain I was able to debug the transactions using truffle debug <tx_hash>

This yielded the following result (where the last stament just seems to be a big in the truffle debugger and can be ignored)

MixinTicketBrokerCore.sol:

308:
309:         require(
310:             isValidTicketSig(_ticket.sender, _sig, _ticketHash),
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

debug(development:0x507c11b6...)>

MixinTicketBrokerCore.sol:

307:         require(!usedTickets[_ticketHash], "ticket is used");
308:
309:         require(
             ^^^^^^^^

debug(development:0x507c11b6...)>

MixinTicketBrokerCore.sol:

39:     // Checks if sender's reserve is frozen
40:     modifier reserveNotFrozen(address _sender) {
41:         require(
            ^^^^^^^^

debug(development:0x507c11b6...)>

Transaction halted with a RUNTIME ERROR.

According to the debugger we are not sending a transaction with a valid signature.

Then it was established empirically through observing the orchestrator logs that when multiple tickets are received in a single payment, we end up with a race condition where we change the state (and thus pointer) of ticket in the next iteration of looping over TicketSenderParamsbefore the ticket redemption go-routine can fire needing the previous pointer which results in a ticket redemption being submitted with an already incremented nonce where it should not be. Therefore we are failing a signature verification over the ticket, not because it isn't actually signed by the sender but because we're checking the signature for the wrong ticket hash (if the sendernonce changes, so does the ticket hash).

func (orch *orchestrator) ProcessPayment(payment net.Payment, manifestID ManifestID) error {

	... 

	ticket := &pm.Ticket{
		Recipient:              ethcommon.BytesToAddress(payment.TicketParams.Recipient),
		Sender:                 ethcommon.BytesToAddress(payment.Sender),
		FaceValue:              new(big.Int).SetBytes(payment.TicketParams.FaceValue),
		WinProb:                new(big.Int).SetBytes(payment.TicketParams.WinProb),
		RecipientRandHash:      ethcommon.BytesToHash(payment.TicketParams.RecipientRandHash),
		CreationRound:          payment.ExpirationParams.CreationRound,
		CreationRoundBlockHash: ethcommon.BytesToHash(payment.ExpirationParams.CreationRoundBlockHash),
	}

	totalEV := big.NewRat(0, 1)
	totalTickets := 0
	totalWinningTickets := 0

	for _, tsp := range payment.TicketSenderParams {
		
		ticket.SenderNonce = tsp.SenderNonce

		if won {
			glog.V(common.DEBUG).Infof("Received winning ticket manifestID=%v recipientRandHash=%x senderNonce=%v", manifestID, ticket.RecipientRandHash, ticket.SenderNonce)

			totalWinningTickets++

			go func(ticket *pm.Ticket, sig []byte, seed *big.Int) {
				if err := orch.node.Recipient.RedeemWinningTicket(ticket, sig, seed); err != nil {
					glog.Errorf("error redeeming ticket manifestID=%v recipientRandHash=%x senderNonce=%v: %v", manifestID, ticket.RecipientRandHash, ticket.SenderNonce, err)
				}
			}(ticket, tsp.Sig, seed)
		}
	}
 
	...
}

As a quick fix it was proposed to move the allocation of pm.Ticket inside of the loop so that the correct pointer is preserved in memory (we're not changing the state but creating multiple instances). This works and no more ticket redemption errors are observed.