swap,asset,db,etc: swap state and clean swap shutdown #406

chappjc · 2020-05-25T16:04:44Z

This addresses Swapper state persistence (save and load) and graceful Swapper shutdown. This resends of ack requests for which responses were pending, relaunches of active coin waiters, and rest restores matches.

[TODO: more on swap changes here]

Required changes to other packages:

asset

The asset backends needed support for general outputs, not just UTXOs, and for possibly spent swap contracts. New Output types in each asset backend are created as a base for most other types based on outputs, including UTXO and Contract. This changes many of the server/asset interfaces:

Add to Backend the Input, Output, and UnspentContract interfaces.
Move Value and FeeRate into the Coin interface.
Create the Output interface, adding the Addresses method to Coin, constructed by Backend.Output.
Rename Contract interface methods, and add RefundAddress.

Also, in server/asset/dcr, experiment with a more efficient blockChans structure using a map so that these channels can be easily removed when consumers go away (see (dcr *Backend).Run).

db

Add Fatal() <-chan struct{} method to DEXArchivist, which provides select semantics similar to Context.Done when there is a fatal backend error. Use LastErr to get the error.

market: Just a fix to collectPreimages where a down connection will never register a miss or a collected preimage.

auth,comms: Similar to the market fix, on send/request failure remove response handler, avoiding the wait for expiration, and make the caller handle the error, which may or may not be similar to the expire function.

chappjc

Needs a fair bit of cleanup and has a few little loose ends, but it's complete and the state load store is completely tested.

chappjc · 2020-06-02T17:21:34Z

server/market/market.go

 		}

 		// Send the preimage request to the order's owner.
 		err = m.auth.RequestWithTimeout(ord.User(), req, func(_ comms.Link, msg *msgjson.Message) {
 			m.handlePreimageResp(msg, reqData)
 		}, piTimeout, miss)
 		if err != nil {
+			miss() // only called by RequestWithTimeout on expire, not send errors


Was a bug where a request error (not expire or successful response) would prevent a result for this preimage request, preventing epoch processing.

chappjc · 2020-06-02T17:30:54Z

dex/ws/wslink.go

+// Send sends the passed Message to the websocket peer. The actual writing of
+// the message on the peer's link occurs asynchronously. As such, a nil error
+// only indicates that the link is believed to be up and the message was
+// successfully marshalled.
 func (c *WSLink) Send(msg *msgjson.Message) error {
+	return c.send(msg, nil)
+}
+
+// SendNow is like send, but it waits for the message to be written on the
+// peer's link, returning any error from the write.
+func (c *WSLink) SendNow(msg *msgjson.Message) error {
+	writeErrChan := make(chan error, 1)
+	if err := c.send(msg, writeErrChan); err != nil {
+		return err
+	}
+	return <-writeErrChan
+}
+
+// CONCEPT
+func (c *WSLink) send(msg *msgjson.Message, writeErr chan<- error) error {


There are paths where blocking is acceptable (the client's own websocket handler goroutines) and paths where it is not (requests from swapper, etc.).
I'm not using SendNow, but it's an idea that might improve things that presently have to wait for expire even though a write error is hit immediately (when pings haven't already flagged the link as off/down, and the outgoing request gets that honor).

chappjc · 2020-06-02T17:33:05Z

server/asset/common.go

+// Output is a transaction output.
+type Output interface {
+	Coin
+	Addresses() []string
 }


We discussed eliminating this interface, just using Coin for coins that represent tx outputs. similarly, the Output interface method can go.

chappjc · 2020-06-02T17:36:43Z

dex/msgjson/types.go

@@ -365,7 +366,7 @@ func (msg *Message) String() string {

 // Match is the params for a DEX-originating MatchRoute request.
 type Match struct {
-	signable
+	Signature


Otherwise can't gob encode/decode even though the Sig field was exported. Initially I had some pretty complex GobEncode/GobDecode methods, but it got overly complex.

chappjc · 2020-06-03T16:35:57Z

server/comms/comms_test.go

+		// Banish does Disconnect, but Send is async! Wait, or use SendNow.
+		time.Sleep(50 * time.Millisecond)
 		c.Banish()


@buck54321 Here's a use case for SendNow. I hit this bug while testing the send/outHandler refactoring commit, so I decided to put this in a separate commit.

chappjc · 2020-06-03T16:38:15Z

dex/ws/wslink.go

+	select {
+	case c.outChan <- &sendData{b, writeErr}:
+	case <-c.stopped:
+		return ErrPeerDisconnected
+	}


@buck54321 I squashed in a refactor of send and outHandler with the previous SendNow commit... breaking my own rules about squashing during review, but the SendNow thing really doesn't belong in this PR anyway. We can take this whole commit out into another PR if that would be better for review.

buck54321

First pass. Haven't reviewed the WSLink.Send changes.

I'd like to work towards a fully DB-based solution eventually. I understand that it will be a lot more work, but what do you think are the major barriers for that? Is it mostly just a matter of grinding out the new tables, methods, and data structures, or is there more to it?

server/asset/btc/btc.go

buck54321 · 2020-06-02T20:10:56Z

server/asset/btc/btc.go

-func (btc *Backend) input(txHash *chainhash.Hash, vin uint32) (*Input, error) {
+// newTXIO creates a TXIO for a transaction, spent or unspent. The caller must
+// set the maturity field.
+func (btc *Backend) newTXIO(txHash *chainhash.Hash) (*TXIO, int64, error) {


The caller must set the maturity field.

I'm not actually seeing a good reason for this. It looks like we can even drop the confs return value if we just set maturity in newTXIO.

Agreed. It's just one of several mostly unrelated behaviors I didn't want to change. It does look straightforward, so I'll set it in newTXIO (instead of output, a caller of newTXIO in this case).

Actually, maturity depends on the output not just the transaction. maturity needs to be moved out of TXIO and into the containing type like Output. Ticket change outputs (sstxchange tagged) are spendable with 1 confirm (SStxChangeMaturity) while the actual ticket output (stakesubmission tagged) are spendable in TicketMaturity, plus those stakesubmission outputs can only be spent by a revocation or vote and thus should be considered unspendable for DEX purposes. CoinbaseMaturity only refers to outputs of coinbase and stakebase/votes (stakegen outputs).

I think this needs to be addressed carefully. I suggest we fix this in a follow up PR.

Ah, for btc, addressed.

buck54321 · 2020-06-02T21:02:22Z

server/asset/btc/utxo.go

+// transaction. See also (*Output).Confirmations. This function differs from the
+// Output method in that it is necessary to relocate the utxo after a reorg, it
+// may error if the output is spent.


This function differs from the Output method in that it is necessary to relocate the utxo after a reorg, it may error if the output is spent.

I don't think we want that to to be the case, but it's probably a moot point if we get rid of the UTXO type altogether.

Yeah, that seemed problematic, which is why I documented it. I didn't want to make too many sweeping changes to asset, namely changing behavior of the previous methods so I didn't actually remove utxo.

server/asset/btc/utxo.go

buck54321 · 2020-06-03T15:55:52Z

server/swap/swap.go

+	Msg  *msgjson.Message // init or redeem inferred from Msg.
+}
+
+// client generated message IDs are not globally unique.


I wouldn't expect them to be unique across connects either.

I didn't think this was trouble as the goal of this was just to key uniquely.

But since you mention reconnects, I would expect client-originating requests would have handlers registered by ID (client side) that would not be be lost on disconnect (only dexc shutdown). If this association is lost in the client, the client might not know what to do with any responses from processInit and processRedeem (whether respondError or respondSuccess). Off hand I don't know what happens to the client's request handler map when the connection goes down, but I thought it was fine as long as the software didn't shutdown, staying intact for when a reconnect succeeds.

But otherwise, processInit and processRedeem don't need the client's msg ID provided by the original handleInit/handleRedeem to do their jobs, which consist of validating the contract/redeem via the asset backends, storing the validated data, and sending an ack request to the counter party with a fresh server-generated msg ID. The only possible issue is as I mentioned with the success/fail message sent to the user having an ID that the client no longer recognizes, or even associated with a different client-originating request.

With the server-originating requests (the live ackers vs the waiters under discussion here), I regenerate the msg ID when firing up the live ackers, but it's clearly more of an issue for live coin waiters with the client-originating requests. Do you think this can be handled client side?

Off hand I don't know what happens to the client's request handler map when the connection goes down, but I thought it was fine as long as the software didn't shutdown, staying intact for when a reconnect succeeds

Yeah, the response handlers map persists across reconnects in client/comms (if the request doesn't timeout), but of course this is lost on dexc shutdown or anything that makes a fresh wsConn.

So if the client shutsdown with an outstanding request, it's not going to know what to do with a response it might get when it starts up again.

Looking at client/core, it seems that if these coin waiters are live on the server, then either (*trackedTrade).swapMatches or (*trackedTrade).redeemMatches are sitting at signAndRequest waiting to receive and unmarshal a msgjson.Acknowledgement. And it seems signAndRequest would not be erroring as long as the request Send completed without err (and it would have if the server has has a coin waiter running from it). The client just gives the connection time.Minute*5 according to client/comms.(*wsConn).logReq to respond.

buck54321 · 2020-06-03T16:45:37Z

server/swap/swap_test.go

-	ensureNilErr(rig.ackMatch_maker(true))
-	ensureNilErr(rig.ackMatch_taker(true))
+	if err := rig.ackMatch_maker(true); err != nil {
+		t.Fatal(err)
+	}
+	if err := rig.ackMatch_taker(true); err != nil {
+		t.Fatal(err)
+	}


I think these changes don't really do anything, but I don't have objections if you like this better.

I got frustrated by ensureNilErr among other helpers because the reported error/fatal line number was inside the helper function rather than the line where the helper was called, making debugging the test quite difficult.

I recently found the method off of testing t.Helper() which may allow helpers to be used while not messing up the line numbers.
https://golang.org/pkg/testing/#T.Helper

Wow, nice find @JoeGruffins

buck54321 · 2020-06-03T16:55:34Z

server/asset/btc/utxo.go

-// A UTXO is information regarding an unspent transaction output.
-type UTXO struct {
+// Output represents a transaction output.
+type Output struct {


We should drop UTXO and just use Output. We can just check for spendability in the appropriate methods. I think FundingCoin is now the only place we actually care anymore, and we don't check spendability again after that initial order processing.

I can do this later too.

The assets are now cleaned up quite a bit but there are a couple TODOs for another PR:

address maturity being output specific for decred stake transactions

remove the UTXO type completely

An update for the btc.Backend.blockChans field is incoming though.

buck54321 · 2020-06-03T16:57:31Z

server/cmd/dcrdex/main.go

+		fmt.Printf("Load swapper state from file %q with time stamp %v? (y, n, or enter to abort) ",
+			stateFile.Name, encode.UnixTimeMilli(stateFile.Stamp))
+		scanner := bufio.NewScanner(os.Stdin)
+		scanner.Scan()
+		promptResp := scanner.Text()
+		err = scanner.Err()
+		if err != nil {
+			return fmt.Errorf("input failed: %v", err)
+		}


I'm a little confused on why loading the swap state would ever be optional. Doesn't it depend on the manner of shutdown?

One TODO I had was various dcrdex command line flags to make these decisions, even specifying a certain file if needed. I'd like if there was a way to start fresh, and that's where something like --noswapstaterestore might be helpful. I felt like a prompt was appropriate since it will show you the file name with full path and a formatted time stamp so you can sanity check what you are about to do.

buck54321 · 2020-06-03T16:59:04Z

server/swap/state.go

+// time stamp (in the file name) from the given directory. The *BackupFile
+// return will be nil if the directory was read successfully but no matching
+// files were found.
+func LatestStateFile(dir string) (*BackupFile, error) {


What's the case for allowing multiple swapper states to be stored? It seems like if it's not from the moment of the last shutdown, it's doesn't do much for us.

Mostly for historical and debugging reasons.

chappjc · 2020-06-03T18:36:15Z

Thanks for the fast turnaround with the review. I made some quick replies, but will get into the other comments shortly.

I'd like to work towards a fully DB-based solution eventually. I understand that it will be a lot more work, but what do you think are the major barriers for that? Is it mostly just a matter of grinding out the new tables, methods, and data structures, or is there more to it?

In my initial planning for this, a DB based solution was shaping up to be considerably more complex and error prone, with additional data needed only for state restore rather than normal operation. Reloading the matches would be fairly straightforward although still requiring new columns like swap confirm time for the swapStatus structures. For the most part, I think you could figure out which acks are still pending and resend those, but I see a lot of gotchas trying to make those inferences, and a ton of DB work to identify which matches need what.

On the other hand, restarting active coin waiters would prove to be quite difficult as that requires the client initiated message (either Init or Redeem) to queue up processInit or processRedeem in the fresh latency queue. We could just wait for any live coin waiters before shutting down, but a minute (txWaitExpiration) is longer than I'd like for shutdown.

Similarly, the completion/failure tracking data in orderMatches is just a single line in restore (s.orders.orderMatches = state.OrderMatches) that would be quite hairy otherwise and I'm not even certain we could regenerate it correctly from the DB.

Besides these challenges, it would be certain to be quite slow by comparison too since the tables will be large.

chappjc · 2020-06-03T18:36:26Z

oops, wrong button

JoeGruffins

I haven't gotten through everything yet. Sorry for all the comment nitpicking.

server/asset/btc/btc.go

server/asset/btc/utxo.go

server/asset/btc/btc.go

server/asset/btc/utxo.go

server/asset/btc/btc.go

server/swap/swap.go

JoeGruffins

It's a big diff, but afict everything is sane. I have not encountered any problems that aren't in master during testing.

buck54321

Tests well. I was able to stop and restart the server and complete matches at all reasonably accessible states of the negotiation.

I did see an error once, but was unable to reproduce. I stopped the server and restarted while the match was in TakerSwapCast

2020-06-10 11:57:15.331 [ERR] SWAP: Loading match d47f237f8456ee5cf080ecf566d331ee92b4f99f6c4f65c453f2e46095a2cf68 failed: unable to find redeem in coin f1ae509a72d6990db5b23e265f7d2b082762886fa437b4f9006d0bdc463ba1f800000000 for asset 42: f1ae509a72d6990db5b23e265f7d2b082762886fa437b4f9006d0bdc463ba1f800000000 does not spend d8a061e5fcfdf91423d616f30eec9d12b813b10d79929c2dcb1b8a879149e29700000000

I think this is a fine solution for a complicated situation, but I will again register my discomfort with having two sources of truth and multiple ways for them to get out of sync. I'll continue pushing on this in the coming months.

I wonder if we should store a hash of the gob file in the database, and then ensure that the file matches on startup.

dex/ws/wslink.go

server/comms/comms_test.go

buck54321 · 2020-06-09T23:25:16Z

server/comms/comms_test.go

+		// Banish does Disconnect, but Send is async! Wait, or use SendNow.
+		time.Sleep(50 * time.Millisecond)


This is why I'm not convinced about the new Send pattern. I think it's reasonable that if someone does

client.Send(msg) client.Disconnect()

that a) Send will not block, and b) the message will reach WriteMessage. With the Send/SendNow split, you have to choose one or the other, but never both.

Send is still async though. This was a problem already since Send was async before. The race changed slightly and it became a problem more frequently.

But you can't guarantee that the message will reach WriteMessage. We could before because of the sendWG sync.WaitGroup.

Thanks for spelling that out. I see the change in behavior now with Send. I tweaked the WSLink to ensure all queued messages get their chance at a WriteMessage, and I actually think it's cleaner now. Please see 7c86f8f

That seems to work well.

server/market/market.go

server/swap/swap.go

buck54321 · 2020-06-10T14:33:23Z

dex/ws/wslink.go

+	// Note that there is a brief window where c.on is true but quit and stopped
+	// are not set.


Using atomic to protect c.on was probably a pipe dream. Might go back to a Mutex at some point.

buck54321 · 2020-06-10T14:57:16Z

server/asset/btc/utxo.go

-	tx := utxo.tx
-	if len(tx.outs) <= int(utxo.vout) {
-		return fmt.Errorf("invalid index %d for transaction %s", utxo.vout, tx.hash)
+func (contract *Contract) auditContract() error {


Another follow-up TODO is that auditContract could be made into a constructor for a Contract.

func NewContract(op *Output) (*Contract, error)

buck54321 · 2020-06-10T16:31:15Z

server/swap/swap_test.go

+	if ts.fatal == nil {
+		ts.fatal = make(chan struct{})
+		close(ts.fatal)
+	}


Should just make the channel in tNewTestRig.

The reason for this is that you can only close the channel once so the nil check is the hacky way to ensure that. Checking fatalErr is a possibility too, but I wanted the ability to update the error value even if the backend is already fatal, not that I'm doing that in the tests though.

server/swap/swap_test.go

buck54321 · 2020-06-10T17:08:45Z

server/asset/common.go

+	// RefundAddres is the refund address of the swap contract.
+	RefundAddress() string


Gonna remove this from the interface?

chappjc · 2020-06-10T19:45:35Z

dex/ws/wslink.go

 		for _, sd := range outQueue {
-			relayError(sd.ret, ErrPeerDisconnected)
+			write(sd)


The queue made this pretty simple. Just stop the goroutines and process anything left in the queue. Callers of Send will get an error from Send if the link is stopped, but if they get no error the message is guaranteed to be in outHandler's outQueue at this point.

You are preserving the map now, but not in all cases. If a send error is encountered server-side before the client has reconnected, removeClient would already have been called, and we lose the respHandlers. Should we open an issue for this?

Is this comment meant to be placed on the diff in 0ddc52f? I'm trying to discern if there's a connection between the two spots.

Although I do see that between auth.connMtx.Unlock() in the branch of (*AuthManager).handleConnect and the subsequent auth.addClient call to replace the clientInfo with the fresh connection that the map could be modified because it's a shallow copy. Is that what you are referring to here? I can make a deep copy of the map instead to resolve this.

swap: add TestFatalStorageErr for Run's use of db.Fatal

In (*AuthManager).RequestWithTimeout if client.conn.Request returns with a non-nil error: - delete the response handler from the client's handler map - stop the expire func Timer so it does not fire The caller of Request/RequestWithTimeout should check the error and act accordingly, which may not be calling the expire func. In (*wsLink).Request, do the same handler and timer clean up if Send returns a non-nil error. The caller (often authMgr.RequestWithTimeout) will handle the error as appropriate.

The Contract interface now has a SwapAddress methoda. The Backend.Contract method does not enforcing the output to be unspent. In both dcr and btc backends, keep the UTXO type for FundingCoin, but we will likely change FundingCoin to work with an Output type and instead check spendability by the caller according to their requirements. asset/btc: set maturity in newTXIO asset/dcr: note that maturity cannot be set in newTXIO and requires a fix to check the tag of the output index in question (e.g. ticket change matures right away, ticket outputs are not spendable except by votes and revokes, etc.) This is a TODO for a follow-up fix. asset: remove Backend.Input method and impls

Swapper.matches map keys on MatchID instead of string Rewrite (*Swapper).Run with graceful shutdown and state saving. Use UnspentContract in processInit; Contract in setState. Use Redemption in processRedeem; Input in setState. auth: pendingRequest and RequestWhenConnected Penalize flags connected users as suspended, but still attempts to close accounts in the DB even if they are not found in the connected clients map. handleConnect permits closed accounts to connect for dealing with active swaps, but sets the clientInfo.suspended flag. Refine application of RequestWhenConnected to decrement the connect timeout on each successive reconnect so the user cannot keep reconnecting and disconnecting forever. Set DefaultConnectTimeout to 10 minutes. swap,market: Add RequestWithConnected to stubs. swap: implement coin waiter restore from State Use RequestWhenConnected instead of RequestWithTimeout in both processInit and processRedeem, which are called from the waiter when the coins are found. Implement a connection grace period on resume that increases penalty timeout to connect timeout instead of the usual response timeout. Deal with penalization in a separate AfterFunc (outside of the request's expire function) so that penalization may still be applied without cancelling the request. This is important to enfoce community conduct while still allowing the users to complete swaps since that is in everyone's interest. For this reason, also use RequestWhenConnected in Negotiate even though there is no grace period there on restore. TODO: figure out sensibility of penalization to revoke ack inaction. swap: fix processAck not recognizing revoke acks TODO: do something with the revoke ack sigs!!! msgjson: rename and export signable => Signature for gob auth,swap,market: pendingMessages and SendWhenConnected (*AuthManager).SendWhenConnected is useful to help any message to succeed in the event of transient connectivity issues, such as: new order responses (market), successful verification of contracts or redeems (swap), and any error responses. But it is essential when resending these messages on Swapper load with a saved state when users are unlikely to be connected immediately after startup. swap: remove (*Swapper).coinNotFound, calling respondError directly from processInit and processRedeem. This is cleaner and it allows the decoded coin ID to be included with the coin waiter's timout message. a bunch of cleanup and docs in the swap package. market: Use SendWhenConnected in (*Market).processOrder and respondError For now, just use the expire function to log that the message did not reach its intended recipient. Consider other actions in processOrder. Define a separate market.DefaultConnectTimeout.

Add (*AuthManager).Suspended to check the existence and penalization status of an authenticated user. Block trade orders (cancel is OK) from suspended users in (*OrderRouter).{handleLimit,handleMarket}, then again downstream in (*Market).processOrder.

…es previous persist any registered user response handlers on connect

chappjc mentioned this pull request May 25, 2020

multi: handle trade suspension messages. #269

Merged

chappjc force-pushed the swap-state-WORK branch 2 times, most recently from 76705e9 to d20a62c Compare May 26, 2020 16:08

chappjc mentioned this pull request May 26, 2020

register: timed out waiting for 'notifyfee' response #392

Closed

chappjc force-pushed the swap-state-WORK branch 3 times, most recently from 536c046 to 29ff43d Compare June 2, 2020 17:29

chappjc commented Jun 2, 2020

View reviewed changes

chappjc marked this pull request as ready for review June 2, 2020 17:40

chappjc force-pushed the swap-state-WORK branch from 29ff43d to 6f1fc6a Compare June 3, 2020 16:34

chappjc commented Jun 3, 2020

View reviewed changes

buck54321 requested changes Jun 3, 2020

View reviewed changes

chappjc closed this Jun 3, 2020

chappjc reopened this Jun 3, 2020

chappjc mentioned this pull request Jun 3, 2020

notify client of penalty / account status change #444

Closed

JoeGruffins reviewed Jun 5, 2020

View reviewed changes

JoeGruffins approved these changes Jun 8, 2020

View reviewed changes

chappjc force-pushed the swap-state-WORK branch from e65183c to 9bc64bb Compare June 10, 2020 03:11

buck54321 requested changes Jun 10, 2020

View reviewed changes

chappjc commented Jun 10, 2020

View reviewed changes

buck54321 approved these changes Jun 10, 2020

View reviewed changes

chappjc added 6 commits June 11, 2020 14:32

db: add Fatal to DEXArchivist, like Context.Done for DB error

98f0363

swap: add TestFatalStorageErr for Run's use of db.Fatal

market: miss without timeout on RequestWithTimeout error

5c3ba62

chappjc added 3 commits June 11, 2020 15:33

dcrdex,dex manager: load swap state file with prompts, set state

f10cd26

ws: refactor send and outHandler with queue

48a58a6

auth,comms: connect when user already connected disconnects and remov…

c9e6845

…es previous persist any registered user response handlers on connect

chappjc force-pushed the swap-state-WORK branch from 5c59a2a to c9e6845 Compare June 11, 2020 20:38

chappjc merged commit 20aa77c into decred:master Jun 11, 2020

chappjc deleted the swap-state-WORK branch June 11, 2020 21:11

This was referenced Jun 26, 2020

dcrdex: on startup server must start accounting for booked orders #339

Closed

Swapper shutdown/restart and suspend/resume #293

Closed

		// Banish does Disconnect, but Send is async! Wait, or use SendNow.
		time.Sleep(50 * time.Millisecond)

		// Note that there is a brief window where c.on is true but quit and stopped
		// are not set.

		// RefundAddres is the refund address of the swap contract.
		RefundAddress() string

swap,asset,db,etc: swap state and clean swap shutdown #406

swap,asset,db,etc: swap state and clean swap shutdown #406

Conversation

chappjc commented May 25, 2020 • edited Loading

chappjc left a comment

Choose a reason for hiding this comment

chappjc Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 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

chappjc Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

JoeGruffins Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Jun 3, 2020

chappjc commented Jun 3, 2020

JoeGruffins left a comment

Choose a reason for hiding this comment

JoeGruffins left a comment

Choose a reason for hiding this comment

buck54321 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc commented May 25, 2020 •

edited

Loading

chappjc Jun 2, 2020 •

edited

Loading

chappjc Jun 2, 2020 •

edited

Loading

chappjc Jun 2, 2020 •

edited

Loading

chappjc Jun 2, 2020 •

edited

Loading

chappjc Jun 3, 2020 •

edited

Loading

chappjc Jun 3, 2020 •

edited

Loading

chappjc Jun 3, 2020 •

edited

Loading

JoeGruffins Jun 8, 2020 •

edited

Loading

buck54321 Jun 3, 2020 •

edited

Loading

chappjc Jun 3, 2020 •

edited

Loading

chappjc Jun 11, 2020 •

edited

Loading