Overhaul transaction request logic #19988

sipa · 2020-09-21T07:21:31Z

This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests).

The major changes are:

Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first.
No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available).
The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions).

This replaces #19184, rebased on #18044 and with many small changes.

jonatack · 2020-09-21T11:00:49Z

Initial light Concept ACK based on first reading of the code, the documentation in txrequest.h, and thinking about the differences with respect to the current tx request logic. Debug build clean and local tests green at each commit. The new txrequest fuzzer is running so far without issues.

DrahtBot · 2020-09-21T11:34:35Z

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

Conflicts

No conflicts as of last run.

Coverage

Coverage	Change (pull 19988, `0ad8af2`)	Reference (master, `380705e`)
Lines	+0.1109 %	90.7277 %
Functions	+0.1378 %	86.3344 %
Branches	+0.0613 %	52.0367 %

^{Updated at: 2020-10-09T08:51:32.622660.}

instagibbs · 2020-09-21T14:59:43Z

committing to review soon(TM)

jonatack · 2020-09-21T18:11:54Z

Reviewed the first commit, "Add txrequest module". Overall looks good. Various minor suggestions in https://github.com/jonatack/bitcoin/commits/pr-19988-review-suggestions to not add noise here; feel free to pick and choose.

sipa · 2020-09-22T00:58:59Z

Pushed an update, incorporating @jonatack's nits above, and addressing a number of @ariard's comments on #19184. I also moved the entire implementation to txrequest.cpp, hidden using a TxRequestTracker::Impl object. I hope this makes it easier to distinguish the comments in the .h (now entirely about the observable behavior without implementation details) vs the .cpp (which works quite differently). This also fixed an AppVeyer issue with MSVC failing to compile.

src/txrequest.h

sipa · 2020-09-23T03:06:40Z

@sr-gi You may be interested in this.

ajtowns

Here's a whole bunch of nits from a first pass at the PR, prior to looking at any of the implementation details. As at 2cac006

src/txrequest.h

src/net_processing.cpp

test/functional/p2p_tx_download.py

src/txrequest.cpp

jnewbery

Just a couple of minor style comments so far.

src/txrequest.h

src/txrequest.cpp

instagibbs

Need to review the big commit "Change transaction request logic to use txrequest" but dropping some general comments now

Edit: glanced over that commit, it's really in the weeds for me to say anything sensible without a lot more review time

approach ACK at least

src/txrequest.h

src/net_processing.cpp

ajtowns

Sorry, more nits, still working on getting to the meat. git branch with patches for some of them at https://github.com/ajtowns/bitcoin/tree/202009-txrequest-ideas which might hopefully make it easier to evaluate them.

src/net_processing.cpp

src/txrequest.h

src/txrequest.cpp

sipa · 2020-09-27T16:56:54Z

I think using .CountInFlight() isn't actually the right way to determine being overloaded. If a peer dumps 5000 INVs on us at once, they'll all be inserted at a time when inflight==0, and thus all be eligible to get the "first" marker, and won't get any delay penalty.

On the other hand, I think using CountTracked is suboptimal as well, as it includes entries in COMPLETED state as well, which have no bearing on how loaded the peer currently is.

I think we should use inflight + (entries in CANDIDATE_* state). There is something to be said about excluding CANDIDATE_READY, as those are requests that will most likely go to other peers, but that would complicate specification significantly (as CANDIDATE_READY isn't observable right now). Also, CANDIDATE_DELAY is sometimes also subject to this, but it's too early to tell.

ariard · 2020-10-14T01:18:53Z

Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic.

Diff with last ACK is dropping "Remove support for time going backwards", new units/functional tests comments.

maflcko · 2020-10-14T07:46:06Z

Approach ACK fd9a006 🏹

Show signature and timestamp

Signature:

Timestamp of file with hash f487101a5eb46060cd0a56b0091e376a7a553c1643ccb2255f894b27be16adfa -

naumenkogs · 2020-10-14T09:10:12Z

Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events.

Comparison to the current master code is still hard, but maybe we shouldn't even try: the only advantage current code has is standing test-of-time. This PR, on the other hand, has much better test coverage, and many people looked at it already.

laanwj · 2020-10-14T16:37:41Z

code review ACK fd9a006

ajtowns

Post merge ACK fd9a006

ajtowns · 2020-10-14T04:07:19Z

src/txrequest.cpp

+    {
+        auto& index = m_index.get<ByPeer>();
+        auto it = index.lower_bound(ByPeerView{peer, false, uint256::ZERO});
+        while (it != index.end() && it->m_peer == peer) {


if (it->m_peer != peer) return; // no tracked announcements // invariant from this point: it == end or it->m_peer == peer while (it != index.end()) { auto it_next = std::next(it); if (it_next != index.end() && it_next->m_peer != peer) it_next = index.end(); // do things with it, but don't affect any other announcement from peer, so it_next remains valid it = it_next; }

would be slightly clearer to me for what it's worth -- it's what I'm translating the code to in my head to be convinced it's correct.

…cific MN p2p logic overhault fd9a006 Report and verify expirations (Pieter Wuille) 86f50ed Delete limitedmap as it is unused now (Pieter Wuille) cc16fff Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille) 173a1d2 Expedite removal of tx requests that are no longer needed (Pieter Wuille) de11b0a Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille) 242d164 Change transaction request logic to use txrequest (Pieter Wuille) 5b03121 Add txrequest fuzz tests (Pieter Wuille) 3c7fe0e Add txrequest unit tests (Pieter Wuille) da3b8fd Add txrequest module (Pieter Wuille) Pull request description: This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests). The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). * The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions). This replaces bitcoin#19184, rebased on bitcoin#18044 and with many small changes. ACKs for top commit: ariard: Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic. MarcoFalke: Approach ACK fd9a006 🏹 naumenkogs: Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events. jnewbery: utACK fd9a006 jonatack: WIP light ACK fd9a006 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers. ryanofsky: Light code review ACK fd9a006, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on: Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7

New functional test coverage of tx download was added by bitcoin#19988, but `with p2p_lock` is redundant for some tests with `wait_until` test helper, already guaranteeing test lock tacking.

bc4a230 Remove redundant p2p lock tacking for tx download functional tests (Antoine Riard) d3b5eac Add mutation for functional test test_preferred_inv (Antoine Riard) 06efb31 Add functional test test_txid_inv_delay (Antoine Riard) a07910a test: Makes wtxidrelay support a generic P2PInterface option (Antoine Riard) Pull request description: This is a simple functional test to increase coverage of #19988, checking that txid announcements from txid-relay peers are delayed by TXID_RELAY_DELAY, assuming we have at least another wtxid-relay peer. You can verify new test with the following diff : ``` diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f14db37..2a2805df5 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -773,7 +773,7 @@ void PeerManager::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std auto delay = std::chrono::microseconds{0}; const bool preferred = state->fPreferredDownload; if (!preferred) delay += NONPREF_PEER_TX_DELAY; - if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY; + //if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY; const bool overloaded = !node.HasPermission(PF_RELAY) && m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; ``` ACKs for top commit: laanwj: ACK bc4a230 Tree-SHA512: 150e806bc5289feda94738756ab375c7fdd23c80c12bd417d3112043e26a91a717dc325a01079ebd02a88b90975ead5bd397ec86eb745c7870ebec379a8aa711

bc4a230 Remove redundant p2p lock tacking for tx download functional tests (Antoine Riard) d3b5eac Add mutation for functional test test_preferred_inv (Antoine Riard) 06efb31 Add functional test test_txid_inv_delay (Antoine Riard) a07910a test: Makes wtxidrelay support a generic P2PInterface option (Antoine Riard) Pull request description: This is a simple functional test to increase coverage of bitcoin#19988, checking that txid announcements from txid-relay peers are delayed by TXID_RELAY_DELAY, assuming we have at least another wtxid-relay peer. You can verify new test with the following diff : ``` diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f14db37..2a2805df5 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -773,7 +773,7 @@ void PeerManager::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std auto delay = std::chrono::microseconds{0}; const bool preferred = state->fPreferredDownload; if (!preferred) delay += NONPREF_PEER_TX_DELAY; - if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY; + //if (!gtxid.IsWtxid() && g_wtxid_relay_peers > 0) delay += TXID_RELAY_DELAY; const bool overloaded = !node.HasPermission(PF_RELAY) && m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; ``` ACKs for top commit: laanwj: ACK bc4a230 Tree-SHA512: 150e806bc5289feda94738756ab375c7fdd23c80c12bd417d3112043e26a91a717dc325a01079ebd02a88b90975ead5bd397ec86eb745c7870ebec379a8aa711

New functional test coverage of tx download was added by bitcoin#19988, but `with p2p_lock` is redundant for some tests with `wait_until` test helper, already guaranteeing test lock tacking.

Summary: ``` This adds a new module (unused for now) which defines TxRequestTracker, a data structure that maintains all information about transaction requests, and coordinates requests. ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@da3b8fd#diff-e3e574cb6dabe01b6149f9d121fc7e286abb49c04442a0a816977ef2a4103ed8 This also includes a GCC workaround that would cause the build to fail: Partial backport of [[bitcoin/bitcoin#20162 | core#20162]]. This backport will be completed as PR19988 is ported because it impacts the code from other commits. Since this is a lot of changes the tests are done in separate diffs (as per the PR commits). For now the code is not used so this is safe. Test Plan: ninja Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9547

Summary: ``` Add unit tests for TxRequestTracker. Several scenarios are tested, randomly interleaved with eachother. Includes a test by Antoine Riard (ariard). ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@3c7fe0e Partial backport of [[bitcoin/bitcoin#20162 | core#20162]]. Depends on D9547. Test Plan: ninja check Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9548

Summary: ``` This adds a fuzz test that reimplements a naive reimplementation of TxRequestTracker (with up to 16 fixed peers and 16 fixed txhashes), and compares the real implementation against it. ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@5b03121 Depends on D9548. Test Plan: ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9549

Summary: ``` This removes most transaction request logic from net_processing, and replaces it with calls to a global TxRequestTracker object. The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight and requestable already, we still want to request it from them. The cap is replaced with an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). Includes functional tests written by Marco Falke and Antoine Riard. ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@242d164 Depends on D9549. Test Plan: ninja all check-all With and without the sanitizers: ./test/functional/test_runner.py p2p_tx_download Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9550

Summary: ``` Maintaining up to 100000 INVs per peer is excessive, as that is far more than fits in a typical mempool. Also disable the "overload" penalty for PF_RELAY peers. ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@de11b0a Depends on D9550. Test Plan: ninja all check-all ./test/functional/test_runner.py p2p_tx_download Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9551

Summary: ``` Whenever a transaction is added to the mempool or orphan pool, both its txid and wtxid are considered AlreadyHave, and thus will eventually be removed from m_txrequest. The same is true for hashes added to the reject filter, but note that sometimes only the wtxid is added (in which case only the wtxid can be removed from m_txrequest). ``` Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@173a1d2 Depends on D9551. Test Plan: ninja all check-all ./test/functional/test_runner.py p2p_tx_download Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9552

Summary: Partial backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@86f50ed Depends on D9552. Test Plan: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9553

Summary: Completes backport of [[bitcoin/bitcoin#19988 | core#19988]]: bitcoin/bitcoin@fd9a006 Depends on D9553. Test Plan: ninja all check-all ./test/functional/test_runner.py p2p_tx_download ninja bitcoin-fuzzers Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9554

79c02c8 Randomize message processing peer order (Pieter Wuille) Pull request description: Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the `vNodes` vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a "first-served-by-loop-first" basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays. As there isn't anything particularly optimal about the current ordering, just make it unpredictable by randomizing. Reported by Crypt-iQ. ACKs for top commit: jnewbery: ACK 79c02c8 Crypt-iQ: ACK 79c02c8 sdaftuar: utACK 79c02c8 achow101: Code Review ACK 79c02c8 jamesob: crACK 79c02c8 jonatack: ACK 79c02c8 vasild: ACK 79c02c8 theStack: ACK 79c02c8 Tree-SHA512: 9a87c4dcad47c2d61b76c4f37f59674876b78f33f45943089bf159902a23e12de7a5feae1a73b17cbc3f2e37c980ecf0f7fd86af9e6fa3a68099537a3c82c106

fanquake added the P2P label Sep 21, 2020

sipa force-pushed the 202009_txrequest_rand_wtxid branch from 1564c23 to 48d0630 Compare September 21, 2020 09:22

sipa mentioned this pull request Sep 21, 2020

Overhaul transaction request logic #19184

Closed

This was referenced Sep 21, 2020

Per-Peer Message Capture #19509

Merged

p2p: Move all header verification into the network layer, extend logging #19107

Merged

bloom: use Span instead of std::vector for insert and contains [ZAP3] #18985

Closed

sipa force-pushed the 202009_txrequest_rand_wtxid branch from 48d0630 to 2992819 Compare September 21, 2020 18:01

sipa force-pushed the 202009_txrequest_rand_wtxid branch from 2992819 to 2cac006 Compare September 22, 2020 00:44

DrahtBot mentioned this pull request Sep 22, 2020

p2p: don't add AlreadyHave transactions to recentRejects #19753

Merged

maflcko reviewed Sep 22, 2020

View reviewed changes

src/txrequest.h Outdated Show resolved Hide resolved

ajtowns reviewed Sep 23, 2020

View reviewed changes

sipa force-pushed the 202009_txrequest_rand_wtxid branch 2 times, most recently from 2c53de0 to 7949d08 Compare September 24, 2020 01:01

jnewbery reviewed Sep 24, 2020

View reviewed changes

src/txrequest.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Sep 24, 2020

View reviewed changes

src/txrequest.h Outdated Show resolved Hide resolved

src/txrequest.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202009_txrequest_rand_wtxid branch from 7949d08 to 4508a6e Compare September 24, 2020 17:33

instagibbs reviewed Sep 24, 2020

View reviewed changes

sipa force-pushed the 202009_txrequest_rand_wtxid branch from 4508a6e to 9d0dbb8 Compare September 24, 2020 23:43

ajtowns reviewed Sep 25, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 25, 2020

net: Use alternative port for incoming Tor connections #19991

Merged

sipa force-pushed the 202009_txrequest_rand_wtxid branch 3 times, most recently from 2a17aca to 363fc32 Compare September 27, 2020 07:40

laanwj merged commit c2c4dba into bitcoin:master Oct 14, 2020

ajtowns reviewed Oct 14, 2020

View reviewed changes

ariard mentioned this pull request Oct 16, 2020

Add functional test test_txid_inv_delay #20171

Merged

michaelfolkson mentioned this pull request Oct 25, 2020

Dependency on GitHub #20227

Closed

ariard mentioned this pull request Oct 31, 2020

test: Extend p2p_ibd_tx_relay.py to verify no-transaction are requested during IBD #20277

Closed

sipa mentioned this pull request Dec 6, 2020

fuzz: Link all targets once #20560

Merged

ariard mentioned this pull request Dec 10, 2020

Periodically make block-relay connections and sync headers #19858

Merged

ajtowns mentioned this pull request Feb 19, 2021

[p2p] Halt processing of unrequested transactions #21224

Closed

sipa mentioned this pull request Jun 3, 2021

Randomize message processing peer order #22144

Merged

jnewbery mentioned this pull request Jun 9, 2021

net processing: Remove hash and fValidatedHeaders from QueuedBlock #22141

Merged

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

Overhaul transaction request logic #19988

Overhaul transaction request logic #19988

Uh oh!

Conversation

sipa commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Sep 21, 2020

Uh oh!

DrahtBot commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

instagibbs commented Sep 21, 2020

Uh oh!

jonatack commented Sep 21, 2020

Uh oh!

sipa commented Sep 22, 2020

Uh oh!

Uh oh!

sipa commented Sep 23, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

instagibbs 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Sep 27, 2020

Uh oh!

ariard commented Oct 14, 2020

Uh oh!

maflcko commented Oct 14, 2020

Uh oh!

naumenkogs commented Oct 14, 2020

Uh oh!

laanwj commented Oct 14, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Oct 14, 2020

sipa commented Sep 21, 2020 •

edited

Loading

DrahtBot commented Sep 21, 2020 •

edited

Loading

instagibbs left a comment •

edited

Loading