p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check #18895

glozow · 2020-05-06T00:38:22Z

Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.

This PR addresses some of the outstanding TODOs building on top of it:

remove nLastResend logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038 comment)
expose unbroadcast info via RPCs, for more informative queries and testing (P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038 comment)
add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038 comment)

fanquake · 2020-05-06T00:42:03Z

My assumption is that those TODOs would be taken care of as part of #18807. Is that correct @amitiuttarwar ?

amitiuttarwar · 2020-05-06T02:02:18Z

@fanquake sorry for the confusion 😛
@gzhao408 offered to implement these follow ups, so I asked her to open a PR so I & others could review. I was thinking (now that this PR is open), I'd update #18807 to let reviewers know, & focus that PR on updates to release notes and tests.
does that seem reasonable?

fanquake · 2020-05-06T02:06:30Z

does that seem reasonable?

@amitiuttarwar Yea of course. Just wanted to clarify that you weren't both working on the same changes in parallel.

amitiuttarwar · 2020-05-06T02:11:07Z

@fanquake thanks!

Concept ACK :) will review soon

maflcko

Concept ACK, but it looks like the mempool_packages has a hardcoded mempool entry in the test, so that fails because the hardcoded entry is missing the unbraodcast key.

src/net_processing.cpp

src/rpc/blockchain.cpp

mzumsande

Concept ACK.

I think that mempool_packages.py just needs to wait a bit so that the unbroadcast status of txns does not change in between the getrawmempool and getmempoolentry calls because of delays in the p2p.

Also, it's confusing that #18807 has (almost) the same name as this PR, so maybe change the title?

naumenkogs · 2020-05-07T13:21:05Z

Concept ACK. Code looks good, will do ACK when @MarcoFalke comments are addressed.

glozow · 2020-05-09T22:04:45Z

Sorry for all the force pushes 😅. Addressed MarcoFalke's comments and the mempool_packages.py error. As mzumsande said, it was due to the unbroadcast value changing in between calls (i.e. a transaction completed initial broadcast while the test was running, so a subsequent call with verbose=True was inconsistent with before.

With unbroadcast as one of the mempool entry fields, it's probably not a good idea to compare mempool entries as a whole, ie assert_equal(mempool0[tx], mempool1[tx] because unbroadcast might be different. Most tests are more specific than this, so they are ok. I fixed mempool_packages.py by having the node wait for len(mempool) invs, so all unbroadcasts should be False.

mzumsande · 2020-05-13T00:03:29Z

I meant an earlier spot in my comment: The assert_equal(entry, mempool[x]) in L92 can fail because
mempool = self.nodes[0].getrawmempool(True) and
entry = self.nodes[0].getmempoolentry(x)
can have inconsistent unbroadcast flags if we haven't waited before calling getrawmempool the first time.

glozow · 2020-05-13T00:40:54Z

I meant an earlier spot in my comment: The assert_equal(entry, mempool[x]) in L92

@mzumsande OMG I am so sorry 🤦‍♀️ that should have been obvious. Just fixed it (I hope), waiting to see what travis says.

DrahtBot · 2020-05-13T08:38:26Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

test/functional/mempool_packages.py

src/rpc/blockchain.cpp

test/functional/mempool_packages.py

maflcko

Almost ACK

test/functional/test_framework/mininode.py

amitiuttarwar

overall, the code looks good! the approaches are simple & make sense. solid test coverage. I also appreciate the thoughtful documentation :) thank you for taking this on!

I left a bunch of nits and thoughts to consider. Other than removing the const to get rid of the build warning, feel free to take or leave whatever.

Just to note- this code adds unbroadcast info to 5 different RPC methods. Of these 5, the unbroadcast info of 2 are explicitly tested from the functional tests (getmempoolinfo, getrawmempool). But the others are implicitly tested because its a shared data structure that serves the data (getmempoolancestors, getmempooldescendants, getmempoolentry). That coverage seems sufficient to me.

I like the wait_for_broadcast method & how it encapsulates the synchronization required to ensure a txn is removed from the unbroadcast set. have you checked if there are other rpc call sites in the tests that might benefit (aka help prevent weird race conditions)? I’ll double check this for my next round of review.

thanks!

src/txmempool.h

test/functional/mempool_packages.py

src/rpc/blockchain.cpp

src/txmempool.h

test/functional/test_framework/mininode.py

amitiuttarwar · 2020-05-19T18:23:13Z

also I realized, since this updates some RPC endpoints, should there be release notes added? @MarcoFalke

maflcko · 2020-05-19T18:26:16Z

Sure, but I assumed this would be taken care of in the doc pr #18807

test/functional/test_framework/mininode.py

- expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast - makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set before and after originating node connects to peers (adds this in mempool_unbroadcast.py) - adds mempool method IsUnbroadcastTx to query for tx inclusion in mempool's unbroadcast set

- before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not - this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns - check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening

- mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata), so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete ('unbroadcast' = False) otherwise the state may change in between calls - update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list of txids to complete initial broadcast - make mempool_packages.py wait because it compares entries using getrawmempool and getmempoolentry

maflcko · 2020-05-20T00:07:00Z

Review ACK 651f1d8

naumenkogs

Code review ACK 651f1d8

amitiuttarwar · 2020-05-21T19:27:39Z

ACK 651f1d8 🎉

double checked functional tests for possible races around returning unbroadcast from the various RPCs. looks like mempool_packages is the only one comparing all the fields of the returned object, so your changes have them covered.

…empool sanity check 651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408) 9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408) a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408) d160069 [wallet] remove nLastResend logic (gzhao408) Pull request description: Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours. This PR addresses some of the outstanding TODOs building on top of it: - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment))) - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment))) - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment))) ACKs for top commit: naumenkogs: Code review ACK 651f1d8 amitiuttarwar: ACK 651f1d8 🎉 MarcoFalke: Review ACK 651f1d8 Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845

jonatack

Posthumous code review ACK 651f1d8

src/txmempool.h

jonatack · 2020-05-22T07:47:04Z

src/txmempool.h

@@ -716,6 +719,12 @@ class CTxMemPool
        return m_unbroadcast_txids;
    }

+    // Returns if a txid is in the unbroadcast set


This can be doxygen like the neighbouring functions. Perhaps touch up in #18807.

harding · 2020-05-23T10:52:07Z

src/rpc/blockchain.cpp

@@ -399,6 +399,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
    RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
        {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
+    RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet confirmed)"},


Suggested rephrasing: s/confirmed/acknowledged/. I think "confirmed" might confuse the user into thinking we're referring to the transaction being included in a block. Perhaps "acknowledged" would be less confusing, or if a few more words are ok, "(initial broadcast not yet acknowledged by any peers)". Maybe @amitiuttarwar can squeeze this in #18807 (if not, I can open a separate PR).

great suggestion. adding to #18807

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a 👁 gzhao408: ACK [`9e1cb1a`](bitcoin@9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

…empool sanity check 651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408) 9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408) a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408) d160069 [wallet] remove nLastResend logic (gzhao408) Pull request description: Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours. This PR addresses some of the outstanding TODOs building on top of it: - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment))) - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment))) - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment))) ACKs for top commit: naumenkogs: Code review ACK 651f1d8 amitiuttarwar: ACK 651f1d8 🎉 MarcoFalke: Review ACK 651f1d8 Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a eye gzhao408: ACK [`9e1cb1a`](9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

…empool sanity check 651f1d8 [test] wait for inital broadcast before comparing mempool entries (gzhao408) 9d3f7eb [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408) a7ebe48 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408) d160069 [wallet] remove nLastResend logic (gzhao408) Pull request description: Followup to bitcoin#18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours. This PR addresses some of the outstanding TODOs building on top of it: - remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (bitcoin#18038 [comment](bitcoin#18038 (comment))) - expose unbroadcast info via RPCs, for more informative queries and testing (bitcoin#18038 [comment](bitcoin#18038 (comment))) - add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (bitcoin#18038 [comment](bitcoin#18038 (comment))) ACKs for top commit: naumenkogs: Code review ACK 651f1d8 amitiuttarwar: ACK 651f1d8 🎉 MarcoFalke: Review ACK 651f1d8 Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845

9e1cb1a [trivial/doc] Fix comment type (Amiti Uttarwar) 8f30260 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar) 750456d [trivial] Remove misleading 'const' (Amiti Uttarwar) fa32e67 [test] Manage node connections better in mempool persist test (Amiti Uttarwar) 1f94bb0 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar) 9c8a55d [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar) ba54983 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar) 00d44a5 [test] P2P connection behavior should meet expectations (Amiti Uttarwar) bd093ca [test] updates to unbroadcast test (Amiti Uttarwar) dab298d [docs] add release notes (Amiti Uttarwar) Pull request description: This PR is a follow up to bitcoin#18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions. bitcoin#18895 is another follow up to that addresses other functionality updates. Background context: The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool. ACKs for top commit: MarcoFalke: ACK 9e1cb1a eye gzhao408: ACK [`9e1cb1a`](9e1cb1a) Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6

Summary: remove nLastResend logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). 1 This is a backport of Core [[bitcoin/bitcoin#18895 | PR18895]] [1/3] bitcoin/bitcoin@d160069 Depends on D9027 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9023

Summary: a7ebe48b94c5a9195c8eabd193204c499cb4bfdb - expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast - makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set before and after originating node connects to peers (adds this in mempool_unbroadcast.py) - adds mempool method IsUnbroadcastTx to query for tx inclusion in mempool's unbroadcast set ------ 651f1d816f054cb9c637f8a99c9360bba381ef58 - mempool entry 'unbroadcast' field changes when tx passes initial broadcast (receive getdata), so anytime you compare mempool entries as a whole, you must wait for all broadcasts to complete ('unbroadcast' = False) otherwise the state may change in between calls - update P2PTxInvStore to send msg_getdata for invs and add functionality to wait for a list of txids to complete initial broadcast - make mempool_packages.py wait because it compares entries using getrawmempool and getmempoolentry ------ This is a backport of Core [[bitcoin/bitcoin#18895 | PR18895]] [2/3] bitcoin/bitcoin@a7ebe48 bitcoin/bitcoin@651f1d8 Depends on D9023 Test Plan: ninja && test/functional/test_runner.py mempool_* Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9024

Summary: - before reattempting broadcast for unbroadcast txns, check they are in mempool and remove if not - this protects from memory leaks and network spam just in case unbroadcast set (incorrectly) has extra txns - check that tx is in mempool before adding to unbroadcast set to try to prevent this from happening This is a backport of Core [[bitcoin/bitcoin#18895 | PR18895]] [3/3] bitcoin/bitcoin@9d3f7eb Depends on D9024 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9025

fanquake added the P2P label May 6, 2020

glozow marked this pull request as ready for review May 6, 2020 02:08

glozow changed the title ~~[wip] unbroadcast followups~~ unbroadcast followups May 6, 2020

glozow force-pushed the unbroadcast-followup branch from b47cd8b to 7abdadf Compare May 6, 2020 02:13

amitiuttarwar mentioned this pull request May 6, 2020

[doc / test / mempool] unbroadcast follow-ups #18807

Merged

maflcko approved these changes May 6, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

maflcko added this to the 0.21.0 milestone May 6, 2020

mzumsande reviewed May 6, 2020

View reviewed changes

glozow changed the title ~~unbroadcast followups~~ p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check May 7, 2020

glozow force-pushed the unbroadcast-followup branch 3 times, most recently from b320734 to 365defb Compare May 9, 2020 22:03

glozow force-pushed the unbroadcast-followup branch from 365defb to cd76381 Compare May 9, 2020 22:07

glozow force-pushed the unbroadcast-followup branch from cd76381 to 4ebe10f Compare May 13, 2020 00:35

maflcko reviewed May 13, 2020

View reviewed changes

test/functional/mempool_packages.py Outdated Show resolved Hide resolved

maflcko reviewed May 13, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request May 13, 2020

RPC: Add ancestor{count,size,fees} to listunspent output #12677

Merged

glozow force-pushed the unbroadcast-followup branch from 4ebe10f to c8ac437 Compare May 14, 2020 23:38

maflcko reviewed May 14, 2020

View reviewed changes

test/functional/mempool_packages.py Show resolved Hide resolved

glozow force-pushed the unbroadcast-followup branch from c8ac437 to bc2d7a9 Compare May 15, 2020 23:45

maflcko approved these changes May 16, 2020

View reviewed changes

test/functional/test_framework/mininode.py Outdated Show resolved Hide resolved

glozow force-pushed the unbroadcast-followup branch from bc2d7a9 to 827fc85 Compare May 18, 2020 17:29

amitiuttarwar reviewed May 19, 2020

View reviewed changes

maflcko reviewed May 19, 2020

View reviewed changes

test/functional/test_framework/mininode.py Outdated Show resolved Hide resolved

glozow added 3 commits May 19, 2020 14:23

glozow force-pushed the unbroadcast-followup branch from 827fc85 to 651f1d8 Compare May 19, 2020 23:31

fanquake requested review from naumenkogs and ajtowns May 20, 2020 00:29

naumenkogs approved these changes May 20, 2020

View reviewed changes

fanquake requested a review from amitiuttarwar May 21, 2020 01:35

fanquake merged commit ad3a61c into bitcoin:master May 21, 2020

jonatack reviewed May 22, 2020

View reviewed changes

harding reviewed May 23, 2020

View reviewed changes

glozow deleted the unbroadcast-followup branch May 25, 2020 20:31

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check #18895

p2p: unbroadcast followups: rpcs, nLastResend, mempool sanity check #18895

Uh oh!

Conversation

glozow commented May 6, 2020 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented May 6, 2020

Uh oh!

amitiuttarwar commented May 6, 2020

Uh oh!

fanquake commented May 6, 2020

Uh oh!

amitiuttarwar commented May 6, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented May 7, 2020

Uh oh!

glozow commented May 9, 2020

Uh oh!

mzumsande commented May 13, 2020

Uh oh!

glozow commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 13, 2020

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amitiuttarwar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amitiuttarwar commented May 19, 2020

Uh oh!

maflcko commented May 19, 2020

Uh oh!

Uh oh!

maflcko commented May 20, 2020

Uh oh!

naumenkogs left a comment • 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!

amitiuttarwar commented May 21, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack May 22, 2020

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar May 23, 2020

Choose a reason for hiding this comment

Uh oh!

harding May 23, 2020

Choose a reason for hiding this comment

Uh oh!

amitiuttarwar May 23, 2020

Choose a reason for hiding this comment

Uh oh!

glozow commented May 6, 2020 •

edited by fanquake

Loading

glozow commented May 13, 2020 •

edited

Loading

naumenkogs left a comment •

edited

Loading