From ec0ecfa065a1066baa02a77bcfbb706718fe0f99 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 6 May 2020 19:13:25 -0700 Subject: [PATCH] account: add support for withdrawals This commit introduces support for trader account withdrawals to arbitrary outputs. Traders are able to withdraw their accounts through either spending paths, multi-sig or expiry, with the latter requiring a new expiration (to be done as a follow-up). When spending through the multi-sig path, traders submit their outputs to the auctioneer, excluding the new account output as that can be reconstructed by the auctioneer. The auctioneer then creates a transaction adhering to the trader's constraints and provides a signature back. --- account/manager.go | 159 +++++++++++++++++++++------ account/manager_test.go | 230 ++++++++++++++++++++++++++++++++++------ 2 files changed, 323 insertions(+), 66 deletions(-) diff --git a/account/manager.go b/account/manager.go index 11b946b..8481940 100644 --- a/account/manager.go +++ b/account/manager.go @@ -387,6 +387,10 @@ func (m *Manager) resumeAccount(ctx context.Context, account *Account, // either a matched order or trader modification, so we'll need to wait // for its confirmation. Once it confirms, handleAccountConf will take // care of the rest of the flow. + // + // TODO(wilmer): Handle restart case where the client shuts down after + // the modification has been reflected on-disk, but the auctioneer's + // signature hasn't been received. case StatePendingUpdate: numConfs := numConfsForValue(account.Value) log.Infof("Waiting for %v confirmation(s) of account %x", @@ -555,12 +559,6 @@ func (m *Manager) handleAccountConf(traderKey *btcec.PublicKey, return err } - // Ensure we don't transition an account that's been closed back to open - // if the account was closed before it was open. - if account.State != StatePendingOpen { - return nil - } - log.Infof("Account %x is now confirmed at height %v!", traderKey.SerializeCompressed(), confDetails.BlockHeight) @@ -695,6 +693,47 @@ func (m *Manager) handleAccountExpiry(traderKey *btcec.PublicKey) error { return nil } +// WithdrawAccount attempts to withdraw funds from the account associated with +// the given trader key into the provided outputs. +func (m *Manager) WithdrawAccount(ctx context.Context, + traderKey *btcec.PublicKey, outputs []*wire.TxOut, + feeRate chainfee.SatPerKWeight, + bestHeight uint32) (*Account, *wire.MsgTx, error) { + + account, err := m.cfg.Store.Account(traderKey) + if err != nil { + return nil, nil, err + } + + if account.State != StateOpen { + return nil, nil, fmt.Errorf("account must be in %v to be"+ + "modified", StateOpen) + } + + // TODO(wilmer): Reject if account has pending orders. + + witnessType := determineWitnessType(account, bestHeight) + newAccountOutput, modifiers, err := createNewAccountOutput( + account, outputs, witnessType, feeRate, + ) + if err != nil { + return nil, nil, err + } + + outputs = append(outputs, newAccountOutput) + modifiers = append(modifiers, StateModifier(StatePendingUpdate)) + + modifiedAccount, spendPkg, err := m.spendAccount( + ctx, account, outputs, witnessType, modifiers, false, + bestHeight, + ) + if err != nil { + return nil, nil, err + } + + return modifiedAccount, spendPkg.tx, nil +} + // CloseAccount attempts to close the account associated with the given trader // key. Closing the account requires a signature of the auctioneer since the // account is composed of a 2-of-2 multi-sig. The account is closed to a P2WPKH @@ -731,8 +770,10 @@ func (m *Manager) CloseAccount(ctx context.Context, traderKey *btcec.PublicKey, closeOutputs = append(closeOutputs, output) } + modifiers := []Modifier{StateModifier(StatePendingClosed)} _, spendPkg, err := m.spendAccount( - ctx, account, closeOutputs, witnessType, bestHeight, + ctx, account, closeOutputs, witnessType, modifiers, true, + bestHeight, ) if err != nil { return nil, err @@ -743,13 +784,14 @@ func (m *Manager) CloseAccount(ctx context.Context, traderKey *btcec.PublicKey, // spendAccount houses most of the logic required to properly spend an account // by creating the spending transaction, updating persisted account states, -// requesting a signature from the auctioneer if necessary, and finally -// broadcasting the spending transaction. These operations are performed in this -// order to ensure trader are able to resume the spend of an account upon -// restarts if they happen to shutdown mid-process. +// requesting a signature from the auctioneer if necessary, broadcasting the +// spending transaction, and finally watching for the new account state +// on-chain. These operations are performed in this order to ensure trader are +// able to resume the spend of an account upon restarts if they happen to +// shutdown mid-process. func (m *Manager) spendAccount(ctx context.Context, account *Account, - outputs []*wire.TxOut, witnessType witnessType, - bestHeight uint32) (*Account, *spendPackage, error) { + outputs []*wire.TxOut, witnessType witnessType, modifiers []Modifier, + isClose bool, bestHeight uint32) (*Account, *spendPackage, error) { // Create the spending transaction of an account based on the provided // witness type. @@ -759,6 +801,13 @@ func (m *Manager) spendAccount(ctx context.Context, account *Account, ) switch witnessType { case expiryWitness: + // TODO(wilmer): Support modifications through the expiry path. + // This will require a new account expiration. + if !isClose { + return nil, nil, errors.New("modifications for expired " + + "accounts are not currently supported") + } + spendPkg, err = m.spendAccountExpiry( ctx, account, outputs, bestHeight, ) @@ -771,19 +820,40 @@ func (m *Manager) spendAccount(ctx context.Context, account *Account, } // With the transaction crafted, update our on-disk state and broadcast - // the transaction. - modifiers := []Modifier{ - StateModifier(StatePendingClosed), CloseTxModifier(spendPkg.tx), + // the transaction. We'll need some additional modifiers based on + // whether the account is being closed or not. + if isClose { + modifiers = append(modifiers, CloseTxModifier(spendPkg.tx)) + } else { + // The account output should be recreated, so we need to locate + // the new account outpoint. + newAccountOutput, err := account.Copy(modifiers...).Output() + if err != nil { + return nil, nil, err + } + idx, ok := clmscript.LocateOutputScript( + spendPkg.tx, newAccountOutput.PkScript, + ) + if !ok { + return nil, nil, fmt.Errorf("new account output "+ + "script %x not found in spending transaction", + newAccountOutput.PkScript) + } + modifiers = append(modifiers, OutPointModifier(wire.OutPoint{ + Hash: spendPkg.tx.TxHash(), + Index: idx, + })) } - err = m.cfg.Store.UpdateAccount(account, modifiers...) - if err != nil { + + prevAccountState := account.Copy() + if err := m.cfg.Store.UpdateAccount(account, modifiers...); err != nil { return nil, nil, err } // If we require the auctioneer's signature, request it now. if witnessType == multiSigWitness { witness, err := m.constructMultiSigWitness( - ctx, account, spendPkg, + ctx, prevAccountState, spendPkg, modifiers, isClose, ) if err != nil { return nil, nil, err @@ -831,11 +901,33 @@ func (m *Manager) spendAccountExpiry(ctx context.Context, account *Account, // given spending transaction of an account and returns the fully constructed // witness to spend the account input. func (m *Manager) constructMultiSigWitness(ctx context.Context, - account *Account, spendPkg *spendPackage) (wire.TxWitness, error) { + account *Account, spendPkg *spendPackage, modifiers []Modifier, + isClose bool) (wire.TxWitness, error) { - auctioneerSig, err := m.cfg.Auctioneer.ModifyAccount( - ctx, account, nil, spendPkg.tx.TxOut, nil, + var ( + auctioneerSig []byte + err error ) + + if isClose { + // If the account is being closed, we shouldn't provide any + // modifiers. + auctioneerSig, err = m.cfg.Auctioneer.ModifyAccount( + ctx, account, nil, spendPkg.tx.TxOut, nil, + ) + } else { + // Otherwise, the account output is being recreated due to a + // modification, so we need to filter it out from the spending + // transaction as the auctioneer can reconstruct it themselves. + idx := account.Copy(modifiers...).OutPoint.Index + outputs := make([]*wire.TxOut, len(spendPkg.tx.TxOut)-1) + copy(outputs, spendPkg.tx.TxOut[:idx]) + copy(outputs, spendPkg.tx.TxOut[idx+1:]) + + auctioneerSig, err = m.cfg.Auctioneer.ModifyAccount( + ctx, account, nil, outputs, modifiers, + ) + } if err != nil { return nil, err } @@ -896,7 +988,7 @@ func (m *Manager) createSpendTx(ctx context.Context, account *Account, // given fee rate. func createNewAccountOutput(account *Account, outputs []*wire.TxOut, witnessType witnessType, feeRate chainfee.SatPerKWeight) (*wire.TxOut, - error) { + []Modifier, error) { // To determine the new value of the account, we'll need to subtract the // values of all additional outputs and the resulting fee of the @@ -912,7 +1004,8 @@ func createNewAccountOutput(account *Account, outputs []*wire.TxOut, case multiSigWitness: accountInputWitnessSize = clmscript.MultiSigWitnessSize default: - return nil, fmt.Errorf("unknown witness type %v", witnessType) + return nil, nil, fmt.Errorf("unknown witness type %v", + witnessType) } var weightEstimator input.TxWeightEstimator @@ -929,7 +1022,7 @@ func createNewAccountOutput(account *Account, outputs []*wire.TxOut, // know its type. pkScript, err := txscript.ParsePkScript(out.PkScript) if err != nil { - return nil, fmt.Errorf("unable to parse output "+ + return nil, nil, fmt.Errorf("unable to parse output "+ "script %x: %v", out.PkScript, err) } @@ -941,8 +1034,8 @@ func createNewAccountOutput(account *Account, outputs []*wire.TxOut, case txscript.WitnessV0ScriptHashTy: weightEstimator.AddP2WSHOutput() default: - return nil, fmt.Errorf("unsupported output script %x", - out.PkScript) + return nil, nil, fmt.Errorf("unsupported output "+ + "script %x", out.PkScript) } outputTotal += btcutil.Amount(out.Value) @@ -954,20 +1047,22 @@ func createNewAccountOutput(account *Account, outputs []*wire.TxOut, fee := feeRate.FeeForWeight(int64(weightEstimator.Weight())) newAmount := inputTotal - outputTotal - fee if newAmount < minAccountValue { - return nil, fmt.Errorf("new account value is below accepted "+ - "minimum of %v", minAccountValue) + return nil, nil, fmt.Errorf("new account value is below "+ + "accepted minimum of %v", minAccountValue) } // Use the next output script in the sequence to avoid script reuse. newPkScript, err := account.NextOutputScript() if err != nil { - return nil, err + return nil, nil, err } - return &wire.TxOut{ + newAccountOutput := &wire.TxOut{ Value: int64(newAmount), PkScript: newPkScript, - }, nil + } + modifiers := []Modifier{ValueModifier(newAmount), IncrementBatchKey()} + return newAccountOutput, modifiers, nil } // sanityCheckAccountSpendTx ensures that the spending transaction of an account diff --git a/account/manager_test.go b/account/manager_test.go index 871c3ab..f10f4f2 100644 --- a/account/manager_test.go +++ b/account/manager_test.go @@ -3,6 +3,7 @@ package account import ( "bytes" "context" + "encoding/hex" "errors" "fmt" "reflect" @@ -104,6 +105,15 @@ func (h *testHarness) assertAccountExists(expected *Account) { } if !reflect.DeepEqual(found, expected) { + // Nil the public key curves before spew to prevent + // extraneous output. + found.TraderKey.PubKey.Curve = nil + expected.TraderKey.PubKey.Curve = nil + found.AuctioneerKey.Curve = nil + expected.AuctioneerKey.Curve = nil + found.BatchKey.Curve = nil + expected.BatchKey.Curve = nil + return fmt.Errorf("expected account: %v\ngot: %v", spew.Sdump(expected), spew.Sdump(found)) } @@ -115,8 +125,8 @@ func (h *testHarness) assertAccountExists(expected *Account) { } } -func (h *testHarness) openAccount(value btcutil.Amount, expiry uint32, - bestHeight uint32) *Account { +func (h *testHarness) openAccount(value btcutil.Amount, expiry uint32, // nolint:unparam + bestHeight uint32) *Account { // nolint:unparam h.t.Helper() @@ -175,14 +185,7 @@ func (h *testHarness) closeAccount(account *Account, outputs []*wire.TxOut, // This should prompt the account's closing transaction to be broadcast // and its state transitioned to StatePendingClosed. - var closeTx *wire.MsgTx - select { - case closeTx = <-h.wallet.publishChan: - case <-time.After(timeout): - h.t.Fatal("expected close transaction to be broadcast") - } - - checkCloseTx(h.t, closeTx, account) + closeTx := h.assertSpendTxBroadcast(account, nil, nil) account.State = StatePendingClosed account.CloseTx = closeTx @@ -198,37 +201,99 @@ func (h *testHarness) closeAccount(account *Account, outputs []*wire.TxOut, return closeTx } -func checkCloseTx(t *testing.T, closeTx *wire.MsgTx, account *Account) { - t.Helper() +func (h *testHarness) assertSpendTxBroadcast(accountBeforeSpend *Account, + outputs []*wire.TxOut, newValue *btcutil.Amount) *wire.MsgTx { - // The closing transaction should only include one output, which should - // be a P2WPKH output of the account's trader key. - if len(closeTx.TxOut) != 1 { - t.Fatalf("expected 1 output in close transaction, found %d", - len(closeTx.TxOut)) + h.t.Helper() + + var spendTx *wire.MsgTx + select { + case spendTx = <-h.wallet.publishChan: + case <-time.After(timeout): + h.t.Fatal("expected spend transaction to be broadcast") } - _, addrs, _, err := txscript.ExtractPkScriptAddrs( - closeTx.TxOut[0].PkScript, &chaincfg.MainNetParams, - ) - if err != nil { - t.Fatalf("unable to extract address: %v", err) + // The spending transaction should spend the account. + foundAccountInput := false + for _, txIn := range spendTx.TxIn { + if txIn.PreviousOutPoint == accountBeforeSpend.OutPoint { + foundAccountInput = true + } } - if len(addrs) != 1 { - t.Fatalf("expected 1 address, found %d", len(addrs)) - } - addr, ok := addrs[0].(*btcutil.AddressWitnessPubKeyHash) - if !ok { - t.Fatalf("expected P2WPKH address, found %T", addr) + if !foundAccountInput { + h.t.Fatalf("did not find account input %v in spend transaction", + accountBeforeSpend.OutPoint) } - witnessProgram := btcutil.Hash160( - account.TraderKey.PubKey.SerializeCompressed(), - ) - if !bytes.Equal(addr.WitnessProgram(), witnessProgram) { - t.Fatalf("expected witness program %x, got %x", witnessProgram, - addr.WitnessProgram()) + // If no outputs were provided, we should expect to see a single wallet + // output. + if len(outputs) == 0 { + if len(spendTx.TxOut) != 1 { + h.t.Fatalf("expected 1 output in spend transaction, "+ + "found %d", len(spendTx.TxOut)) + } + _, addrs, _, err := txscript.ExtractPkScriptAddrs( + spendTx.TxOut[0].PkScript, &chaincfg.MainNetParams, + ) + if err != nil { + h.t.Fatalf("unable to extract address: %v", err) + } + if len(addrs) != 1 { + h.t.Fatalf("expected 1 address, found %d", len(addrs)) + } + addr, ok := addrs[0].(*btcutil.AddressWitnessPubKeyHash) + if !ok { + h.t.Fatalf("expected P2WPKH address, found %T", addr) + } + // Witness program of address returned by the mock + // implementation of NextAddr. + witnessProgram := btcutil.Hash160(testRawTraderKey) + if !bytes.Equal(addr.WitnessProgram(), witnessProgram) { + h.t.Fatalf("expected witness program %x, got %x", + witnessProgram, addr.WitnessProgram()) + } + + return spendTx } + + // Otherwise, the spending transaction should include the expected + // outputs. If it recreates the account output, we should also attempt + // to locate it. + if newValue != nil { + nextPkScript, err := accountBeforeSpend.NextOutputScript() + if err != nil { + h.t.Fatalf("unable to generate next output script: %v", + err) + } + outputs = append(outputs, &wire.TxOut{ + Value: int64(*newValue), + PkScript: nextPkScript, + }) + } + if len(spendTx.TxOut) != len(outputs) { + h.t.Fatalf("expected %d output(s) in spend transaction, found %d", + len(outputs), len(spendTx.TxOut)) + } + + // The output indices may not match due to BIP-69 sorting. +nextOutput: + for _, output := range outputs { + for _, txOut := range spendTx.TxOut { + if !bytes.Equal(txOut.PkScript, output.PkScript) { + continue + } + if txOut.Value != output.Value { + h.t.Fatalf("expected value %v for output %x, "+ + "got %v", output.Value, output.PkScript, + txOut.Value) + } + continue nextOutput + } + h.t.Fatalf("expected output script %x in spend transaction", + output.PkScript) + } + + return spendTx } func (h *testHarness) restartManager() { @@ -464,3 +529,100 @@ func TestAccountSpendBatchNotFinalized(t *testing.T) { // updateAccount call above does so implicitly. h.assertAccountExists(account) } + +// TestAccountWithdrawal ensures that we can process an account withdrawal +// through the happy flow. +func TestAccountWithdrawal(t *testing.T) { + t.Parallel() + + h := newTestHarness(t) + h.start() + defer h.stop() + + const bestHeight = 100 + account := h.openAccount( + maxAccountValue, bestHeight+maxAccountExpiry, bestHeight, + ) + + // With our account created, we'll start our withdrawal by creating the + // outputs we'll withdraw our funds to. We'll create three outputs, one + // of each supported output type. Each output will have 1/4 of the + // account's value. + valuePerOutput := account.Value / 4 + p2wsh, _ := hex.DecodeString("00208c2865c87ffd33fc5d698c7df9cf2d0fb39d93103c637a06dea32c848ebc3e1d") + p2wpkh, _ := hex.DecodeString("0014ccdeffed4f9c91d5bf45c34e4b8f03a5025ec062") + np2wpkh, _ := hex.DecodeString("a91458c11505b54582ab04e96d36908f85a8b689459787") + outputs := []*wire.TxOut{ + { + Value: int64(valuePerOutput), + PkScript: p2wsh, + }, + { + Value: int64(valuePerOutput), + PkScript: p2wpkh, + }, + { + Value: int64(valuePerOutput), + PkScript: np2wpkh, + }, + } + + // We'll use the lowest fee rate possible, which should yield a + // transaction fee of 260 satoshis when taking into account the outputs + // we'll be withdrawing to. + const feeRate = chainfee.FeePerKwFloor + const expectedFee btcutil.Amount = 260 + + // Attempt the withdrawal. + // + // If successful, we'll follow with a series of assertions to ensure it + // was performed correctly. + _, _, err := h.manager.WithdrawAccount( + context.Background(), account.TraderKey.PubKey, outputs, + feeRate, bestHeight, + ) + if err != nil { + t.Fatalf("unable to process account withdrawal: %v", err) + } + + // We'll start by ensuring a proper spend transaction was broadcast that + // contains the expected outputs from above, and the recreated account + // output. + withdrawOutputSum := valuePerOutput * btcutil.Amount(len(outputs)) + valueAfterWithdrawal := account.Value - withdrawOutputSum - expectedFee + withdrawalTx := h.assertSpendTxBroadcast( + account, outputs, &valueAfterWithdrawal, + ) + + // The account should be found within the store with the following + // modifiers. + mods := []Modifier{ + ValueModifier(valueAfterWithdrawal), + StateModifier(StatePendingUpdate), + OutPointModifier(wire.OutPoint{ + Hash: withdrawalTx.TxHash(), + Index: 0, + }), + IncrementBatchKey(), + } + for _, mod := range mods { + mod(account) + } + h.assertAccountExists(account) + + // Notify the transaction as a spend of the account. The account should + // remain in StatePendingUpdate until it reaches the appropriate number + // of confirmations. + h.notifier.spendChan <- &chainntnfs.SpendDetail{SpendingTx: withdrawalTx} + h.assertAccountExists(account) + + // Notify the confirmation, causing the account to transition back to + // StateOpen. + h.notifier.confChan <- &chainntnfs.TxConfirmation{Tx: withdrawalTx} + StateModifier(StateOpen)(account) + h.assertAccountExists(account) + + // Finally, close the account to ensure we can process another spend + // after the withdrawal. + _ = h.closeAccount(account, nil, bestHeight) +}