refactor: Replace chain relayTransactions/submitMemoryPool by higher method #15713

ariard · 2019-04-01T01:22:20Z

Remove CWalletTx::AcceptToMemoryPool

Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay

Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.

Obviously, working on implementing #14978 (comment) to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly

DrahtBot · 2019-04-01T02:38:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16521 (wallet/rpc: Use the default maxfeerate value as BTC/kB by Remagpie)
#16503 (Remove p2pEnabled from Chain interface by ariard)
#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#15169 (Parallelize CheckInputs() in AcceptToMemoryPool() by sdaftuar)
#14920 (Build: enable -Wdocumentation via isystem by Empact)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky · 2019-04-02T22:47:36Z

src/interfaces/chain.cpp

Thanks for consolidating the code from relayTransaction and submitToMemoryPool in interfaces/chain.cpp and from BroadcastTransaction in node/transaction.cpp.

I do think the right place for all this code to live is in node/transaction.cpp, though. interfaces/chain.cpp is really just meant to be a simple glue file with lines like:

bool broadcastTransaction(...) override { return BroadcastTransaction(...); }

Also, I didn't look closely yet, but it seems like this refactoring might be changing wallet behavior (this could explain travis test failures). It would probably be better if this PR avoided changing behavior to the extent possible. This might require adding some extra options to the existing BroadcastTransaction function in node/transaction.cpp, or adding a second broadcast function there that can hopefully share some code with the current one.

Okay was thinking than node/transaction.cpp was temporary and long term goal was to aggregate it to interfaces/node.cpp.
Didn't want to change wallet behavior, fixed it by adding options in the existing BroadcastTransaction (but still not sure of ReacceptWalletTransactions, should we broadcast txn there ?), functional tests should be good now. Also rebased

re: #15713 (comment)

Thanks! This looks good and now net removes code (+63/-83).

Okay was thinking than node/transaction.cpp was temporary

Yeah, that may have been the original thought. But I think it could be a reasonable long term location if more things are moved around it. In any case it's where the BroadcastTransaction function currently lives, so it's easier not to move in this PR, which is more about simplifying wallet code.

and long term goal was to aggregate it to interfaces/node.cpp

Everything in interfaces/, including interfaces/node.cpp is just supposed to be very short glue code (the files would be huge otherwise). The interfaces are a way of letting wallet, node, and gui code run in different processes, and a way of preventing them from accessing each others internal state.

jnewbery · 2019-04-03T20:32:36Z

Thanks for looking at this @ariard. My view is different from @ryanofsky's. I think rather than trying to maintain existing behaviour, we should use this interface tidyup to better define the relationship between the wallet and the node, ie to answer the question "What services should the node offer to the wallet?" Currently, the node exposes two methods to the wallet for sending a transaction:

submitToMemoryPool(const CTransactionRef& tx, CAmount absurd_fee, CValidationState& state): Add transaction to memory pool if the transaction fee is below the amount specified by absurd_fee. Returns false if the transaction could not be added due to the fee or for another reason. This is only called in CWalletTx::AcceptToMemoryPool()
relayTransaction(const uint256& txid): adds the txid to the INV queue for each peer. This is only called in CWalletTx::RelayWalletTransaction() (which also calls CWalletTx::AcceptToMemoryPool())

Note that to relay an INV for the transaction to peers (2), the transaction must already be in the mempool. See:

bitcoin/src/net_processing.cpp

Line 3804 in ba54342

auto txinfo = mempool.info(hash);

relayTransaction() is only ever called if InMempool() || AcceptToMemoryPool() is true.

The wallet currently calls CWalletTx::AcceptToMemoryPool() in the following places:

1a. In CWallet::ReacceptWalletTransactions(), called after the wallet has started up to make sure that its transactions are added to the mempool at start.
2a. In CWalletTx::RelayWalletTransaction(), mentioned above.
3a. In CWallet::CommitTransaction(), when a transaction is to be broadcast to the network for the first time.

The wallet currently calls CWalletTx::RelayWalletTransaction() in the following places:

2a. In CWallet::ResendWalletTransactions(), called on a timer to rebroadcast unconfirmed transactions.
2b. In CWallet::CommitTransaction(), but only if the transaction is successfully added to the mempool.

I've been thinking about this in the context of the wallet rebroadcasting transactions (see discussion here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-03-29-19.10.log.html#l-9). My view now is that the wallet should not have any access to relay functionality. It should be able to submit transactions to the node's mempool, and it should then be the mempool's responsibility to broadcast/rebroadcast the transaction (see comment here: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2019/bitcoin-core-dev.2019-03-29-19.10.log.html#l-92). That seems like a much cleaner divide of responsibility.

This PR seems like a simplification in terms of combining submitToMemoryPool() and relayTransaction() into a single method, but I don't think that it moves us towards a point where the node<->wallet interface is simpler to understand.

ryanofsky · 2019-04-03T20:45:49Z

John are you just saying that this PR does not go far enough, or that it does something bad or makes things more complicated?

I haven't looked closely enough at this yet but if it exposes one node method node instead of two, and reduces the amount of code without changing behavior, it would seem like a strict improvement. Is your rebroadcast proposal harder to implement after this PR?

src/interfaces/chain.cpp

src/wallet/wallet.cpp

jnewbery · 2019-04-03T21:24:23Z

I haven't looked closely enough at this yet but if it exposes one node method node instead of two, and reduces the amount of code without changing behavior, it would seem like a strict improvement.

It does reduce the number of node methods exposed, but does so by adding a flag to one of them which toggles the behaviour. I'm not sure if that counts as a simplification, or it's just moving the complexity around. I'm also not sure if this maintains existing behaviour. For example, the GetDepthInMainChain(locked_chain) == 0 in RelayWalletTransaction() seems to have gone and not been replaced by anything.

ariard · 2019-04-03T23:55:15Z

I kinda agree with @jnewbery here, wallet shouldn't have access to relay, because if so it may add complexity of proper tx rebroadcasting tracking, notably in privacy-enhancing context in referenced discussions ?
Have taken GetDepthInMainChain and IsCoinbase out, because seems it's cleaner to let the mempool do the checks, in AcceptToMemoryPoolWorker if I understand it well. And not multiply spurious wallet accesses to chain state.
Sorry scope of PR wasn't clear, would happily change it to underline the real issue I guess.

ryanofsky · 2019-04-09T14:48:05Z

Ariard or John, what are next steps? Should this PR be closed and revisited in the future if rebroadcasting is improved? Should the current code comment about this be removed or clarified?

bitcoin/src/interfaces/chain.h

Lines 52 to 54 in f3ecf30

    
           //! * The relayTransactions() and submitToMemoryPool() methods could be replaced 
        
           //!   with a higher-level broadcastTransaction method 
        
           //!   (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984).

jnewbery · 2019-04-09T15:06:08Z

I don't want to discourage @ariard from making this change if you both think it's an improvement over the existing code. It certainly doesn't preclude us from makeing the changes that I proposed in #15713 (comment) at a later time. @ariard - if you think this should still go in, can you perhaps break up the PR into commits and justify any changes (eg why remove GetDepthInMainChain and IsCoinbase from RelayWalletTransaction().

Note that this conflicts with my #15728 which I think is a useful tidy-up in itself.

ariard · 2019-04-09T22:30:46Z

Okay will keep working on it by breaking up the PR and justify changes. Had a look on #15728, IMO conflicts should be solved smoothly once yours get into it.
@jnewbery I've taken notes of your proposed changes of preventing wallet access to relay functionality but as far as I understand it would need first implementation of relay responsibility in mempool, something not submit yet ? Once that done, removing relay from this code path seems really straightforward changes.

ariard · 2019-04-11T16:22:50Z

Rebased, splitted the commits, added justifications. Note that I removed relay flag in BroadcastTransaction, if I get John opinion submitting tx to memory pool should imply relay if it's a valid one.

ariard · 2019-04-11T17:28:15Z

src/wallet/wallet.cpp

I still have a doubt on BroadcastTransaction HaveChain check behavior. HaveChain = !existingCoin.IsSpent() doesn't mean if tx is in chain, but all of its outputs has been spent, we accept it again to mempool/relay ?

@ariard wrote:

I still have a doubt on BroadcastTransaction HaveChain check behavior. HaveChain = !existingCoin.IsSpent() doesn't mean if tx is in chain, but all of its outputs has been spent, we accept it again to mempool/relay ?

IsSpent() is "misleadingly" named. Because we use AccessCoin: Return a reference to Coin in the cache, or a pruned one if not found.. Pruned in this case means all its CTxOut are IsNull(), and IsSpent() just calls IsNull().

Thanks @Sjors . I agree with you that IsSpent() is a misleading name. A more accurate name would be DoesNotExist(). Negating the function here determines whether the coin exists in the current UTXO set.

The check in BroadcastTransaction() is an optimization: if any of the outputs from the tx still exist in the UTXO set then the tx must be confirmed. If the coin has been confirmed but it's outputs don't exist, then it'll fail later (because its inputs can't exist if it has already been spent).

This check in RelayWalletTransaction() is also an optimization. If the transaction has been confirmed (or a conflicting transaction has been confirmed), then the block hash will be recorded in the CMerkleTx, and we can check in the chain whether that block exists.

An alternative to removing this call to GetDepthInMainChain() would be to merge #15931 first, which removes GetDepthInMainChain's requirement on locking the chain.

Thanks @Sjors . I agree with you that IsSpent() is a misleading name. A more accurate name would be DoesNotExist(). Negating the function here determines whether the coin exists in the current UTXO set.

The check in BroadcastTransaction() is an optimization: if any of the outputs from the tx still exist in the UTXO set then the tx must be confirmed. If the coin has been confirmed but it's outputs don't exist, then it'll fail later (because its inputs can't exist if it has already been spent).

It might be useful to enhance the code documentation added in 976d1e485 with this information (if it isn't expected to change soon). I found it v helpful to understanding the !existingCoin.IsSpent()) optimisation.

jnewbery

opinion submitting tx to memory pool should imply relay if it's a valid one

No, this would cause the wallet to rebroadcast all of its unconfirmed transactions at startup. That's a change in behaviour and a potentially bad privacy leak.

src/interfaces/chain.cpp

src/node/transaction.cpp

src/interfaces/chain.h

jnewbery

A few comments inline. The intermediate commit 07d5787041461bcfb8b5777809d874869710aa1e does not build because locked_chain is not defined in SubmitMemoryPoolAndRelay().

src/interfaces/chain.cpp

src/interfaces/chain.h

src/node/transaction.cpp

src/node/transaction.h

jnewbery · 2019-04-12T15:24:40Z

src/wallet/wallet.cpp

Review note: This is a minor behaviour change. chain().broadcastTransaction() could return false if BroadcastTransaction() returned TransactionError::P2P_DISABLED (ie the transaction is added to the mempool but not relayed).

What's your take on returning a pair of boolean from chain().broadcastTransaction() to encode both mempool/relay results ?

In the existing code there's not a return code for chain::relayTransaction(). I think it's fine for the new broadcastTransaction interface method to only return false if the transaction failed to get into the mempool, and treat TransactionError::P2P_DISABLED as success. That way, fInMempool will get updated in exactly the same way as the existing code.

Good, added TransactionError::P2P_DISABLED == err check in broadcastTransaction interface

src/wallet/wallet.cpp

src/node/transaction.h

ariard · 2019-04-13T15:32:23Z

Thanks @jnewbery for the review and sorry for the ugly commit split. I think I've addressed all your points except this one : #15713 (comment) ?

src/interfaces/chain.h

…MemoryPool by higher method fb62f12 Tidy up BroadcastTransaction() (John Newbery) b8eecf8 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard) 8753f56 Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard) 611291c Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard) 8c8aa19 Add BroadcastTransaction utility usage in Chain interface (Antoine Riard) Pull request description: Remove CWalletTx::AcceptToMemoryPool Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic. Obviously, working on implementing bitcoin#14978 (comment) to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly ACKs for top commit: MarcoFalke: re-ACK fb62f12 Sjors: re-ACK fb62f12 Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f

jnewbery

post-merge ACK fb62f12.

A couple of nits inline that could be cleaned up in a subsequent PR.

jnewbery · 2019-08-02T13:42:21Z

src/interfaces/chain.cpp

    {
-        RelayTransaction(txid, *g_connman);
+        const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
+        // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures.


I think this line can be removed now. The only failures that BroadcastTransaction() can return are mempool-related failures.

jnewbery · 2019-08-02T13:43:25Z

src/node/transaction.cpp

-TransactionError BroadcastTransaction(const CTransactionRef tx, uint256& hashTx, std::string& err_string, const CAmount& highfee)
+TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
 {
+    assert(g_connman);


nit: I'd prefer to see a comment next to this assert to explain why g_connman must always be assigned here.

Should be part of #16503, maybe

jnewbery · 2019-08-06T17:09:53Z

I was wrong about this: #15713 (comment). Apparently removing that check causes log spam, particularly for test nodes.

@mzumsande

b7b9f6e Remove p2pEnabled from Chain interface (Antoine Riard) Pull request description: RPC server starts in warmup mode, it can't process yet calls, then follows connection manager initialization and finally RPC server get out of warmup mode. RPC calls shouldn't be able to get P2P disabled errors because once we initialize g_connman it's not unset until shutdown, after RPC server has been stopped. @mzumsande comment in bitcoin#15713 let me thought that `p2pEnabled` was maybe useless, `g_connman` is always initialized before RPC server is getting out of warmup. These checks against P2P state were introduced in bitcoin@5b446dd. ACKs for top commit: promag: ACK b7b9f6e jnewbery: ACK b7b9f6e Tree-SHA512: 4de2b9fc496bf8347ff5cc645848a5a44c8ca7596cd134f17f3088f5f8262d1d88b8e2a052df93e309ec9a81956a808df17a9eb9f10d4f4d693c95d607fe3561

…rface Summary: Access through a broadcastTransaction method. Add a wait_callback flag to turn off race protection when wallet already track its tx being in mempool Standardise highfee, absurdfee variable name to max_tx_fee We drop the P2P check in BroadcastTransaction as g_connman is only called by RPCs and the wallet scheduler, both of which are initialized after g_connman is assigned and stopped before g_connman is reset. bitcoin/bitcoin@8c8aa19 --- Depends on D6272 Partial backport of Core [[bitcoin/bitcoin#15713 | PR15713]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6273

Summary: Higher wallet-tx method combining RelayWalletTransactions and AcceptToMemoryPool, using new Chain::broadcastTransaction bitcoin/bitcoin@611291c --- Partial backport of Core [[bitcoin/bitcoin#15713 | PR15713]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6420

Summary: IsCoinBase check is already performed early by AcceptToMemoryPoolWorker GetDepthInMainChain check is already perfomed by BroadcastTransaction To avoid deadlock we MUST keep lock order in ResendWalletTransactions and CommitTransaction, even if we lock cs_main again further. in BroadcastTransaction. Lock order will need to be clean at once in a future refactoring bitcoin/bitcoin@8753f56 --- Depends on D6420 Partial backport of Core [[bitcoin/bitcoin#15713 | PR15713]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6421

…ctions Chain interfaces Summary: bitcoin/bitcoin@b8eecf8 --- Depends on D6421 Partial backport of Core [[bitcoin/bitcoin#15713 | PR15713]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6422

Summary: bitcoin/bitcoin@fb62f12 --- Concludes backport of Core [[bitcoin/bitcoin#15713 | PR15713]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6427

@amitiuttarwar

…utility 3fef7d3 refactor : use RelayTransaction in BroadcastTransaction utility (Antoine Riard) Pull request description: Implementing suggestion in bitcoin/bitcoin#15713 (comment). Seems a reason of these node utilities is to glue with already there functions, so we should reuse them. ACKs for top commit: MarcoFalke: ACK 3fef7d3 promag: ACK 3fef7d3, verified there are no more `PushInventory(CInv(MSG_TX, ...`, nice refactor, 👍 @amitiuttarwar. jnewbery: ACK 3fef7d3 jonatack: ACK 3fef7d3, second @jnewbery's suggestions, my guess is they could be added without risking delaying this PR. Tree-SHA512: 841c65d5f0d9ead5380814bb2260d7ebf03f2a9bfa58a1025785d353bdb42f9122cc257993e6a7bd2bd3f2c74db19c5978cc14be0d83258124ca22e33d6d0164

@amitiuttarwar

…utility 9bc8b28 refactor : use RelayTransaction in BroadcastTransaction utility (Antoine Riard) Pull request description: Implementing suggestion in bitcoin/bitcoin#15713 (comment). Seems a reason of these node utilities is to glue with already there functions, so we should reuse them. ACKs for top commit: MarcoFalke: ACK 9bc8b28 promag: ACK 9bc8b28, verified there are no more `PushInventory(CInv(MSG_TX, ...`, nice refactor, 👍 @amitiuttarwar. jnewbery: ACK 9bc8b28 jonatack: ACK 9bc8b28, second @jnewbery's suggestions, my guess is they could be added without risking delaying this PR. Tree-SHA512: 841c65d5f0d9ead5380814bb2260d7ebf03f2a9bfa58a1025785d353bdb42f9122cc257993e6a7bd2bd3f2c74db19c5978cc14be0d83258124ca22e33d6d0164

… by higher method

fanquake added Refactoring Wallet labels Apr 1, 2019

DrahtBot added the Needs rebase label Apr 1, 2019

ryanofsky reviewed Apr 2, 2019

View reviewed changes

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch from abae703 to 322638c Compare April 3, 2019 03:01

DrahtBot removed the Needs rebase label Apr 3, 2019

ryanofsky reviewed Apr 3, 2019

View reviewed changes

src/interfaces/chain.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

jnewbery mentioned this pull request Apr 3, 2019

Remove wallet relayTransaction #15734

Closed

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch from 322638c to 56e3997 Compare April 3, 2019 23:07

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch from 56e3997 to 468116b Compare April 4, 2019 00:24

DrahtBot added the Needs rebase label Apr 9, 2019

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch from 468116b to 5628623 Compare April 11, 2019 16:20

DrahtBot removed the Needs rebase label Apr 11, 2019

ariard commented Apr 11, 2019

View reviewed changes

jnewbery reviewed Apr 11, 2019

View reviewed changes

src/interfaces/chain.cpp Outdated Show resolved Hide resolved

src/interfaces/chain.cpp Outdated Show resolved Hide resolved

src/node/transaction.cpp Outdated Show resolved Hide resolved

src/interfaces/chain.h Outdated Show resolved Hide resolved

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch 2 times, most recently from bcbc316 to 94623bd Compare April 12, 2019 16:27

jnewbery reviewed Apr 12, 2019

View reviewed changes

ariard force-pushed the 2019-03-refactor-relay-tx-submit-pool branch from 94623bd to 602e38d Compare April 13, 2019 15:25

practicalswift reviewed Apr 15, 2019

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

maflcko merged commit fb62f12 into bitcoin:master Aug 2, 2019

jnewbery reviewed Aug 2, 2019

View reviewed changes

ariard mentioned this pull request Aug 7, 2019

[wallet] restore coinbase and confirmed/conflicted checks in SubmitMemoryPoolAndRelay() #16557

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15713: Replace chain relayTransactions/submitMemoryPool…

0187d2e

… by higher method

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

refactor: Replace chain relayTransactions/submitMemoryPool by higher method #15713

refactor: Replace chain relayTransactions/submitMemoryPool by higher method #15713

Uh oh!

Conversation

ariard commented Apr 1, 2019

Uh oh!

DrahtBot commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 3, 2019

Uh oh!

ryanofsky commented Apr 3, 2019

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Apr 3, 2019

Uh oh!

ariard commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 9, 2019

Uh oh!

jnewbery commented Apr 9, 2019

Uh oh!

ariard commented Apr 9, 2019

Uh oh!

ariard commented Apr 11, 2019

Uh oh!

ariard Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard commented Apr 13, 2019

Uh oh!

Uh oh!

jnewbery left a comment

DrahtBot commented Apr 1, 2019 •

edited

Loading

ariard Apr 3, 2019 •

edited

Loading

ariard commented Apr 3, 2019 •

edited

Loading

ariard Apr 11, 2019 •

edited

Loading

Sjors Jul 26, 2019 •

edited

Loading