tests: add invalid P2P message tests #14522

jamesob · 2018-10-19T17:46:10Z

Adds p2p_invalid_messages.py: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages.
Adds TestNode.assert_memory_usage_stable: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test.
Adds P2PConnection.send_raw_message: which allows us to construct and send messages with tweaked headers.

DrahtBot · 2018-10-20T09:49:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14519 (tests: add utility to easily profile node performance with perf by jamesob)

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/p2p_invalid_messages.py

test/functional/test_framework/test_node.py

jamesob · 2018-10-23T15:00:40Z

Addressed @practicalswift nits.

conscott · 2018-10-23T15:17:33Z

ACK dfbb8624c3747f4e9e76c9b9a051e973857a15c3

jamesob · 2018-10-24T20:02:37Z

Pushed a small change to ensure get_mem_rss() failure is handled gracefully on Windows.

conscott · 2018-10-25T05:51:53Z

test/functional/test_framework/test_node.py

+        if perc_increase_memory_usage > perc_increase_allowed:
+            self._raise_assertion_error(
+                "Memory usage increased over threshold of {}% from {} to {} ({}%)".format(
+                    perc_increase_allowed, before_memory_usage, after_memory_usage,


Since you are displaying as a % instead of decimal, I think you just need to multiply by 100

.format(perc_increase_allowed*100, before_memory_usage, after_memory_usage, perc_increase_memory_usage*100)

Now it'll show something like
Memory usage increased over threshold of 3.0%....

test/functional/test_framework/test_node.py

laanwj · 2018-10-31T18:05:39Z

looks good to me, thanks for adding testing for this particular corner

test/functional/p2p_invalid_messages.py

laanwj · 2018-10-31T20:29:40Z

checked: fails on FreeBSD:

user@vm13:~/bitcoin % test/functional/p2p_invalid_messages.py
2018-10-31T20:28:53.767000Z TestFramework (INFO): Initializing test directory /tmp/testw9nvke44
2018-10-31T20:28:54.336000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/usr/home/user/bitcoin/test/functional/test_framework/test_framework.py", line 172, in main
    self.run_test()
  File "test/functional/p2p_invalid_messages.py", line 69, in run_test
    with node.assert_memory_usage_stable(perc_increase_allowed=0.03):
  File "/usr/local/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "/usr/home/user/bitcoin/test/functional/test_framework/test_node.py", line 291, in assert_memory_usage_stable
    before_memory_usage = self.get_mem_rss()
  File "/usr/home/user/bitcoin/test/functional/test_framework/test_node.py", line 129, in get_mem_rss
    "ps h -o rss {}".format(self.process.pid), shell=True).strip())
ValueError: invalid literal for int() with base 10: b'RSS\n49084'
2018-10-31T20:28:54.405000Z TestFramework (INFO): Stopping nodes
2018-10-31T20:28:54.676000Z TestFramework (WARNING): Not cleaning up dir /tmp/testw9nvke44
2018-10-31T20:28:54.676000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/testw9nvke44/test_framework.log
2018-10-31T20:28:54.677000Z TestFramework (ERROR): Hint: Call /usr/home/user/bitcoin/test/functional/combine_logs.py '/tmp/testw9nvke44' to consolidate all logs

jamesob · 2018-11-01T15:24:26Z

Addressed feedback from @laanwj and @conscott; thanks for the looks.

Adds a utility to get resident set size memory usage for a test node and a context manager that allows assertions based upon maximum memory use increase.

E.g., ensure that we can't DoS a node by sending it a bunch of large, unrecognized messages.

conscott · 2018-11-06T08:12:45Z

Tested Re-ACK d20a9fa

laanwj · 2018-11-06T10:15:23Z

utACK d20a9fa

d20a9fa tests: add tests for invalid P2P messages (James O'Beirne) 62f94d3 tests: add P2PConnection.send_raw_message (James O'Beirne) 5aa31f6 tests: add utility to assert node memory usage hasn't increased (James O'Beirne) Pull request description: - Adds `p2p_invalid_messages.py`: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages. - Adds `TestNode.assert_memory_usage_stable`: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test. - Adds `P2PConnection.send_raw_message`: which allows us to construct and send messages with tweaked headers. Tree-SHA512: 720a4894c1e6d8f1551b2ae710e5b06c9e4f281524623957cb01599be9afea82671dc26d6152281de0acb87720f0c53b61e2b27d40434d30e525dd9e31fa671f

maflcko

Concept ACK. Nice tests!

maflcko · 2018-11-06T22:49:02Z

test/functional/test_framework/test_node.py

+            self.log.warning("Unable to detect memory usage (RSS) - skipping memory check.")
+            return
+
+        perc_increase_memory_usage = 1 - (float(before_memory_usage) / after_memory_usage)


You are taking after_memory_usage as reference, so a increase of n% in the memory usage will result always in perc_increase_memory_usage < n%.

For example you double the memory usage from 1GB to 2GB, this will give you 50%:

>>> 1 - (1 / 2) 0.5

Also you don't need the float conversion with /

maflcko · 2018-11-06T22:51:29Z

test/functional/test_framework/test_node.py

+        try:
+            return int(subprocess.check_output(
+                "ps h -o rss {}".format(self.process.pid),
+                shell=True, stderr=subprocess.DEVNULL).strip())


Why is shell required here?

maflcko · 2018-11-06T22:52:28Z

test/functional/test_framework/test_node.py

+        # isn't installed or doesn't work as expected, e.g. OpenBSD.
+        #
+        # We could later use something like `psutils` to work across platforms.
+        except Exception:


I'd prefer to explicitly catch subprocess.SubprocessError and ValueError, so that KeyError et al is not caught.

maflcko · 2018-11-06T22:53:17Z

test/functional/test_framework/test_node.py

+        """
+        if not (self.running and self.process):
+            self.log.warning("Couldn't get memory usage; process isn't running.")
+            return None


Couldn't this be an assert? How is this different from calling an RPC method when the node is not running?

maflcko · 2018-11-06T23:01:11Z

test/functional/p2p_invalid_messages.py

+            self.log.info("Sending a message with incorrect size of {}".format(wrong_size))
+
+            # Unmodified message should submit okay.
+            node.p2p.send_and_ping(msg)


nit: Could assert debug log for unknown message type?

maflcko · 2018-11-06T23:02:11Z

test/functional/p2p_invalid_messages.py

+        #
+        # Send a message with a too-long command name.
+        #
+        node.p2p.send_message(msg_nametoolong("foobar"))


Could assert debug log with the exact error?

maflcko · 2018-11-06T23:02:38Z

test/functional/p2p_invalid_messages.py

+            except IOError:
+                pass
+
+            node.p2p.wait_for_disconnect(timeout=10)


Could assert debug log?

maflcko · 2018-11-07T20:49:02Z

Also, the test fails on macOS (with OSError: [Errno 41] Protocol wrong type for socket printed to stderr):

stdout:
2018-11-07T18:35:07.995000Z TestFramework (INFO): Initializing test directory /var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/test_runner_₿_🏃_20181107_182921/p2p_invalid_messages_46
2018-11-07T18:35:08.836000Z TestFramework.node0 (ERROR): Unable to get memory usage
Traceback (most recent call last):
  File "/Users/travis/build/MarcoFalke/btc_nightly/bitcoin/test/functional/test_framework/test_node.py", line 130, in get_mem_rss
    shell=True, stderr=subprocess.DEVNULL).strip())
ValueError: invalid literal for int() with base 10: b'RSS\n 71200'
2018-11-07T18:35:08.838000Z TestFramework (INFO): Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...
2018-11-07T18:35:20.251000Z TestFramework (INFO): Waiting for node to drop junk messages.
2018-11-07T18:35:20.330000Z TestFramework.node0 (ERROR): Unable to get memory usage
Traceback (most recent call last):
  File "/Users/travis/build/MarcoFalke/btc_nightly/bitcoin/test/functional/test_framework/test_node.py", line 130, in get_mem_rss
    shell=True, stderr=subprocess.DEVNULL).strip())
ValueError: invalid literal for int() with base 10: b'RSS\n 80896'
2018-11-07T18:35:20.331000Z TestFramework.node0 (WARNING): Unable to detect memory usage (RSS) - skipping memory check.
2018-11-07T18:35:20.393000Z TestFramework.mininode (WARNING): Connection lost to 127.0.0.1:11368 due to [Errno 41] Protocol wrong type for socket
2018-11-07T18:35:20.495000Z TestFramework (INFO): Sending a message with incorrect size of 2
2018-11-07T18:35:20.659000Z TestFramework (INFO): Sending a message with incorrect size of 77
2018-11-07T18:35:20.836000Z TestFramework (INFO): Sending a message with incorrect size of 78
2018-11-07T18:35:21.010000Z TestFramework (INFO): Sending a message with incorrect size of 79
2018-11-07T18:35:21.442000Z TestFramework (INFO): Stopping nodes
2018-11-07T18:35:21.803000Z TestFramework (INFO): Cleaning up /var/folders/nz/vv4_9tw56nv9k3tkvyszvwg80000gn/T/test_runner_₿_🏃_20181107_182921/p2p_invalid_messages_46 on exit
2018-11-07T18:35:21.803000Z TestFramework (INFO): Tests successful
stderr:
Fatal write error on socket transport
protocol: <test_framework.mininode.P2PDataStore object at 0x103d87e48>
transport: <_SelectorSocketTransport fd=10 read=polling write=<idle, bufsize=0>>
Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.6.5_1/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/selector_events.py", line 761, in write
    n = self._sock.send(data)
OSError: [Errno 41] Protocol wrong type for socket

fa9ed38 test_node: get_mem_rss fixups (MarcoFalke) Pull request description: Follow up to #14522: * Fix math (Memory usage increase relative to previous memory usage, not final memory usage) * remove `shell=True` * assert that the node is running * Make it work on BSD-like systems Tree-SHA512: fc1b4f88173914b6cb6373655cffd781044a0c146339e3fa90da03b197faa20954567a77335965b857d29d27f32661698b6a0340f0c616f643b8c4510cd360c2

jnewbery · 2019-02-21T15:32:30Z

I'm very late to this party, but my opinion is that testing memory usage is very useful, but should not be part of a functional test suite. Functional tests should test the functionality of the product, ie the outputs that are triggered by passing in various inputs. Memory usage is related to the implementation of the product. As the follow-on problems with this PR show, making rigid tests that depend on implementation details are brittle when run over different platforms.

I think that this code could be very useful when incorporated into a soak testing framework, but that it shouldn't be included in our functional test suite.

jamesob · 2019-02-21T15:53:14Z

I think that this code could be very useful when incorporated into a soak testing framework, but that it shouldn't be included in our functional test suite.

Probably worth adding some guidance around what is and isn't appropriate for the functional test suite in one of the READMEs.

I'm generally inclined to agree with you, but I think having a comprehensive and easy-to-use framework for adding checks like this is very worthwhile. I'm also inclined to think that packaging such a framework within this repo is probably worthwhile, though there are certainly arguments against that. What do you think about introducing some kind of decorator to denote these sorts of checks on individual test methods and then introducing an --extended-like flag for use with the runner?

jnewbery · 2019-02-21T16:06:49Z

Probably worth adding some guidance around what is and isn't appropriate for the functional test suite in one of the READMEs.

Good idea. I'll add that to my list (or I'm happy to review yours if you want to add it)

I think having a comprehensive and easy-to-use framework for adding checks like this is very worthwhile.

Also agree. The TestNode class is supposed to be reusable, eg for stress or soak testing.

I'm also inclined to think that packaging such a framework within this repo is probably worthwhile, though there are certainly arguments against that. What do you think about introducing some kind of decorator to denote these sorts of checks on individual test methods and then introducing an --extended-like flag for use with the runner?

I think including it in the repo is fine, but my preference would be to separate it somehow from the functional tests, for example having a soak testing framework that imports the TestNode() and then adds the methods it needs. I have a concern that /test/functional will continue to expand to be a test-all-the-things tool, even where other testing techniques are more appropriate.

Summary: d20a9fa13d1c13f552e879798c0508be70190e71 tests: add tests for invalid P2P messages (James O'Beirne) 62f94d39f8de88a44bb0a8a2837d864f777aaacc tests: add P2PConnection.send_raw_message (James O'Beirne) 5aa31f6ef26f51ce461c917654dd1cfbbdd1409a tests: add utility to assert node memory usage hasn't increased (James O'Beirne) Pull request description: - Adds `p2p_invalid_messages.py`: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages. - Adds `TestNode.assert_memory_usage_stable`: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test. - Adds `P2PConnection.send_raw_message`: which allows us to construct and send messages with tweaked headers. Tree-SHA512: 720a4894c1e6d8f1551b2ae710e5b06c9e4f281524623957cb01599be9afea82671dc26d6152281de0acb87720f0c53b61e2b27d40434d30e525dd9e31fa671f Merge #14672: tests: Send fewer spam messages in p2p_invalid_messages 3d305e3b89 Send fewer spam messages in p2p_invalid_messages (James O'Beirne) Pull request description: Builds on travis are failing because the test node isn't able to drop all the bad messages sent within the given timeout. Reduce the number of bad messages we're sending and increase the timeout to avoid failures on travis. Tree-SHA512: 11c389619d9590caf7eca74e0efe6d950469415d34220072770689024b350cc08a2d5ec90634237d87ff71ba8b638c1152b8a45ffbb2815a48bde6a88fbb8fc6 Backport of Core [[bitcoin/bitcoin#14522 | PR14522]] and [[bitcoin/bitcoin#14672 | PR14672]] (to reduce flakiness) Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien, deadalnix Reviewed By: #bitcoin_abc, Fabien, deadalnix Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6178

…t framework update) 7b04c0a [Test] Clean duplicate connections creation in mining_pos_coldStaking.py (furszy) 15a799e [Test] MAX_PROTOCOL_MESSAGE_LENGTH PIVXified. (furszy) 0aedf35 [tests] Don't import asyncio to test magic bytes (John Newbery) 9bb5309 Refactor resource exhaustion test (Troy Giorshev) 589a780 Fix "invalid message size" test (Troy Giorshev) 8a0c12b Move size limits to module-global (Troy Giorshev) b3391c5 Remove two unneeded tests (Troy Giorshev) 1404e68 test: Add various low-level p2p tests (furszy) 8aaf7e1 test: Remove fragile assert_memory_usage_stable (MarcoFalke) f68e22c [Test] p2p_invalid_messages.py do not change msg.command in test_command and raise sync_with_ping timeout (furszy) c851c0b Test framework: Wait for verack by default on every new connection (furszy) c02e9a0 [Test] move framework subversion to string (furszy) 3472a39 Replace coroutine with async def in p2p_invalid_messages.py (Elichai Turkel) 33c7b19 net_processing: align msg checksum check properly. (furszy) 7f71b1b Hash P2P messages as they are received instead of at process-time (furszy) 215a638 test: Skip flaky p2p_invalid_messages test on macOS (Fabian Jahr) c11f565 qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke) be979ad test: Fix race in p2p_invalid_messages (MarcoFalke) 6a72f0c qa: Add tests for invalid message headers (MarcoFalke) 51ddd3d Introduce p2p_invalid_messages.py test (furszy) 55a37b5 net: fix missing jump line in "Oversized message from peer.." error. (furszy) 0edfce5 test_node: get_mem_rss fixups (MarcoFalke) 6f21213 tests: add P2PConnection.send_raw_message (James O'Beirne) ae68c6e tests: add utility to assert node memory usage hasn't increased (James O'Beirne) 8469afe test: forward timeouts properly in send_blocks_and_test (James O'Beirne) db28a53 Skip is_closing() check when not available. (Daniel Kraft) be9dacb tests: fixes mininode's P2PConnection sending messages on closing transport (marcoagner) 53599c8 qa: Avoid start/stop of the network thread mid-test (furszy) 688190c [qa] mininode: Expose connection state through is_connected (MarcoFalke) Pull request description: Part of the deep and long net and ser work that I'm doing (and Tor v3 onion addresses support). Friend of #2359. Focused on the end goal of implementing the `p2p_invalid_messages` functional test which validates that invalid msg headers, msg types, oversized payloads and inventory msgs aren't accepted nor can cause a resource exhaustion. And an extra covered scenario, in `p2p_invalid_tx.py`, for the orphan pool overflow. Plus, to get up to the goal, had to work over the functional test framework as well. So.. adapted list: * bitcoin#9045. * bitcoin#13512. * bitcoin#13517. * bitcoin#13715. * bitcoin#13747. * bitcoin#14456. * bitcoin#14522. * bitcoin#14672. * bitcoin#14693. * bitcoin#14812. * bitcoin#15246. * bitcoin#15330. * bitcoin#15697. * bitcoin#16445. * bitcoin#17931. * bitcoin#17469. * bitcoin#18628 (only `p2p_invalid_tx.py` and `p2p_invalid_messages.py`. We don't support the other tests yet). * bitcoin#19177. * bitcoin#19264. ACKs for top commit: random-zebra: utACK 7b04c0a and merging... Tree-SHA512: 48d1f1a6acc24a9abce033fbf1f281ba4392147da39d118a694a09d63c3e0610cc1a29d711b434b16cc0d0e030dd9f1a89843870091b6999b862b9ab18a20679

d20a9fa tests: add tests for invalid P2P messages (James O'Beirne) 62f94d3 tests: add P2PConnection.send_raw_message (James O'Beirne) 5aa31f6 tests: add utility to assert node memory usage hasn't increased (James O'Beirne) Pull request description: - Adds `p2p_invalid_messages.py`: tests based on behavior for dealing with invalid and malformed P2P messages. Includes a test verifying that we can't DoS a node by spamming it with large invalid messages. - Adds `TestNode.assert_memory_usage_stable`: a context manager that allows us to ensure memory usage doesn't significantly increase on a node during some test. - Adds `P2PConnection.send_raw_message`: which allows us to construct and send messages with tweaked headers. Tree-SHA512: 720a4894c1e6d8f1551b2ae710e5b06c9e4f281524623957cb01599be9afea82671dc26d6152281de0acb87720f0c53b61e2b27d40434d30e525dd9e31fa671f

backport 0.18 PR bitcoin#14522 bitcoin#14672 bitcoin#14812 bitcoin#14494 bitcoin#14690 bitcoin#14700 bitcoin#14705 bitcoin#14478 bitcoin#14247

fanquake added the Tests label Oct 19, 2018

DrahtBot mentioned this pull request Oct 20, 2018

tests: add utility to easily profile node performance with perf #14519

Merged

practicalswift reviewed Oct 22, 2018

View reviewed changes

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

practicalswift reviewed Oct 22, 2018

View reviewed changes

test/functional/test_framework/test_node.py Show resolved Hide resolved

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

conscott reviewed Oct 23, 2018

View reviewed changes

test/functional/test_framework/test_node.py Show resolved Hide resolved

jamesob force-pushed the 2018-10-invalid-msgs-test branch from 435445f to dfbb862 Compare October 23, 2018 15:00

jamesob force-pushed the 2018-10-invalid-msgs-test branch from dfbb862 to 56ecb63 Compare October 24, 2018 20:02

conscott reviewed Oct 25, 2018

View reviewed changes

laanwj reviewed Oct 31, 2018

View reviewed changes

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

laanwj reviewed Oct 31, 2018

View reviewed changes

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

jamesob force-pushed the 2018-10-invalid-msgs-test branch from 56ecb63 to 306ff9d Compare November 1, 2018 14:21

jamesob added 3 commits November 1, 2018 14:52

tests: add utility to assert node memory usage hasn't increased

5aa31f6

Adds a utility to get resident set size memory usage for a test node and a context manager that allows assertions based upon maximum memory use increase.

tests: add P2PConnection.send_raw_message

62f94d3

tests: add tests for invalid P2P messages

d20a9fa

E.g., ensure that we can't DoS a node by sending it a bunch of large, unrecognized messages.

jamesob force-pushed the 2018-10-invalid-msgs-test branch from 306ff9d to d20a9fa Compare November 1, 2018 18:52

laanwj merged commit d20a9fa into bitcoin:master Nov 6, 2018

jamesob deleted the 2018-10-invalid-msgs-test branch November 6, 2018 14:24

maflcko reviewed Nov 6, 2018

View reviewed changes

maflcko mentioned this pull request Nov 8, 2018

test_node: get_mem_rss fixups #14693

Merged

IlyasRidhuan mentioned this pull request Jul 24, 2019

test: Skip flaky p2p_invalid_messages test on macOS #16445

Merged

jnewbery mentioned this pull request Jun 12, 2020

test: Fix and clean p2p_invalid_messages functional tests #19177

Merged

furszy mentioned this pull request Jun 4, 2021

[Test] Add p2p invalid messages functional test (and test framework update) PIVX-Project/PIVX#2410

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

merge bitcoin#14522: tests: add invalid P2P message tests

b7bee7c

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

merge bitcoin#14522: tests: add invalid P2P message tests

11bd300

knst mentioned this pull request Aug 10, 2021

backport 0.18 PR #14522 #14672 #14812 #14494 #14690 #14700 #14705 #14478 #14247 dashpay/dash#4330

Closed

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

merge bitcoin#14522: tests: add invalid P2P message tests

539bfcb

kwvg added a commit to kwvg/dash that referenced this pull request Aug 22, 2021

merge bitcoin#14522: tests: add invalid P2P message tests

e6cce7b

UdjinM6 added a commit to dashpay/dash that referenced this pull request Aug 29, 2021

Merge pull request #4375 from PastaPastaPasta/pr_4330

f38aa44

backport 0.18 PR bitcoin#14522 bitcoin#14672 bitcoin#14812 bitcoin#14494 bitcoin#14690 bitcoin#14700 bitcoin#14705 bitcoin#14478 bitcoin#14247

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

tests: add invalid P2P message tests #14522

tests: add invalid P2P message tests #14522

Uh oh!

Conversation

jamesob commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob commented Oct 23, 2018

Uh oh!

conscott commented Oct 23, 2018

Uh oh!

jamesob commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Oct 31, 2018

Uh oh!

Uh oh!

laanwj commented Oct 31, 2018

Uh oh!

jamesob commented Nov 1, 2018

Uh oh!

conscott commented Nov 6, 2018

Uh oh!

laanwj commented Nov 6, 2018

Uh oh!

maflcko left a comment

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!

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!

maflcko commented Nov 7, 2018

Uh oh!

jnewbery commented Feb 21, 2019

Uh oh!

jamesob commented Feb 21, 2019

Uh oh!

jnewbery commented Feb 21, 2019

Uh oh!

Uh oh!

jamesob commented Oct 19, 2018 •

edited

Loading

DrahtBot commented Oct 20, 2018 •

edited

Loading