WIP: refactor: Use our own integer parsing/formatting everywhere #17385

laanwj · 2019-11-05T20:43:27Z

This replaces direct use of (often locale dependent) C library functions with our own integer parsing and formatting functions., as the developer notes recommend.

For parsing this introduces parse error checking that was not there before. In many cases it's clear what to do on failure, but sometimes it's not. It also adds explicitly sized types everywhere. Please pay attention to this while reviewing.

See individual commits for details.

This leaves the following "known violations":

"src/bitcoin-tx.cpp.*trim_right" boost function, out of scope
"src/dbwrapper.cpp:.*vsnprintf" copied function, the comment addresses this: https://github.com/bitcoin/bitcoin/blob/master/src/dbwrapper.cpp#L19
"src/httprpc.cpp.*trim" another boost function, out of scope
"src/torcontrol.cpp:.*strtol" octal parsing, out of scope
"src/util/strencodings.cpp:.*strtol" part of ParseInt32 and ParseUInt32, gated by our own checks
"src/util/strencodings.cpp:.*strtoul" part of ParseInt64 and ParseUInt64

Also removes a use of a locale-dependent function.

Replace the direct use of `stoul` in dbwrapper.cpp. This removes a locale dependent exception.

Replace the use of atoi in CleanupBlockRevFiles with ParseUInt32 to handle parse errors and remove direct use of a locale-dependent function.

Replace use of function `snprintf` with our own, locale-independent function `strprintf`.

Replace the use of locale-dependent function strtol with our own error-checking integer parsing function `ParseUInt32`.

Replace use of atoi with `ParseInt64`, whose output is at least well-defined. Concretely, the behaviour should not change.

Replace use of `atoi` in torcontrol for parsing reply code with `ParseUInt32`, adding error checking. Also, change type of `code` to `uint32_t`, it is never negative does not need to be a signed integer.

Replace use of atoi64 and circuitois way to test if something is a valid 64-bit integer, with the use of our own function `ParseInt64`.

Replace the use of atoi64 with error and bound checking `ParseUInt32` - after all, the result is being assigned to an unsigned int.

Replace the direct use of atoi64 with error-checking ParseInt64.

Replace the use of atoi64 with error-checking ParseInt64. Unfortunately, there is currently no way to return a parse error, so return the default in that case.

Replace use of `atoi64` with error-checking `ParseInt64`.

Replace direct use of atoi64 with ParseUInt64 (as the result is an unsigned timestamp).

practicalswift · 2019-11-05T20:58:07Z

Wow! That was quick! :)

Strong Concept ACK

Thanks for taking the time to get rid of those once and for all :)

laanwj · 2019-11-05T21:39:37Z

Test failure in util_tests/util_ChainMerge explained:

This test passes -notestnet=1 and -noregtest=1 to the argument parser.
Somehow, this ends up as InterpretBool("1testnet=1")
atoi returns 1 in this case (evaluating to true) because it stops parsing at the first non-digit, our ParseInt64 fails however and returns false.
This causes the test to exit with a different hash.

Do we really support -notestnet=1 and -noregtest=1? I've never seen this syntax.

ryanofsky · 2019-11-05T21:51:02Z

Do we really support -notestnet=1 and -noregtest=1? I've never seen this syntax.

I'll take a look at this. We could make -notestnet and testnet=0 style options into errors, though this PR is probably not the best place to do it, and maybe there is some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

Any case, will first debug and see what causes reported 1testnet=1

laanwj · 2019-11-05T21:54:10Z

We ideally need a way to signal parse errors from GetArg functions, there's a similar situation in ArgsManager::GetArg(int64). Maybe they could return Optional. Out of scope for this PR anyhow, it only needs to do the sane thing here.

some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

In that case couldn't you simply do -testnet=0 or -regtest=0? I don't see the point of -noX with a value.

though this PR is probably not the best place to do it

I agree, I'm tempted to just drop that commit, though it means atoi(string) sticks around.

This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in bitcoin#17385 (comment)

Empact · 2019-11-05T22:35:09Z

Concept ACK 88e8307

This is way too lenient.

ryanofsky · 2019-11-05T22:44:57Z

Discussion seems somewhat offtopic here, maybe discuss argument parsing behavior more in #16545.

We ideally need a way to signal parse errors from GetArg functions, there's a similar situation in ArgsManager::GetArg(int64). Maybe they could return Optional. Out of scope for this PR anyhow, it only needs to do the sane thing here.

I could be wrong but I think it would actually be less work, and friendlier to both users and developers to validate settings on initialization rather than use. #16545 is intended to move in this direction.

some use case if you have a configuration file that normally sets testnet, but you want to override it on the command line and use the settings on regtest instead or something.

In that case couldn't you simply do -testnet=0 or -regtest=0? I don't see the point of -noX with a value.

For int and bool arguments, I think it's generally reasonable to treat -noarg and -noarg=1 the same as -arg=0.

I agree, I'm tempted to just drop that commit, though it means atoi(string) sticks around.

I think it would be best to save both d78e7be and afb5809 for another PR and not include them here. These are user facing changes which could use release notes and better test coverage.

laanwj · 2019-11-05T22:56:13Z

I think it would be best to save both d78e7be and afb5809 for another PR and not include them here.

I'm fine with adding a release not if you think this is a significant user-facing change, and add a test for these two little functions, but I'd really prefer not to keep either the current behavior or code.

DrahtBot · 2019-11-05T23:19:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17050 (tests: Add fuzzing harnesses for functions parsing scripts, numbers, JSON and HD keypaths (bip32) by practicalswift)
#15934 (Merge settings one place instead of five places 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-11-06T00:22:32Z

I'm fine with adding a release not if you think this is a significant user-facing change, but I'd really prefer not to keep either the current behavior or code.

My thinking is that there probably shouldn't be user-facing changes in a PR that's supposed to be a refactor, and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn't make it worse off.

But the user-facing changes are maybe more minor than I would have thought. With your two commits arguments such as "5 " or " 5" or "5." which are currently parsed as 5/true will now be parsed as 0/false. This applies to numeric arguments with leading whitespace or trailing characters.

laanwj · 2019-11-06T00:33:47Z

My thinking is that there probably shouldn't be user-facing changes in a PR that's supposed to be a refactor, and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn't make it worse off.

Many of the changes in this PR make a subtle difference in the case of invalid input, due to adding strict parse error checking (I mention this in the OP). Maybe I shouldn't call it a refactor. But it's not really a bugfix or feature either.

and that this PR is substantial enough with 17 commits, that pulling out 2 of them to a dedicated PR wouldn't make it worse off.

Sure. But remove any and the cleanup in util.cpp/util.h isn't possible. Which was one of my motivations to do this in the first place.

ryanofsky · 2019-11-06T04:41:26Z

I'll review this tomorrow. I think all the commits here that remove uses of C functions to parse strings which are supposed to have a rigid format (numbers in filenames, test strings, descriptors, database properties) are great, and it's great to clean up this code and avoid misusing these C functions.

But while I'll need to review the PR more carefully to be sure, I think the few changes here that can actually affect users should probably be done more carefully. For example, instead of taking a strangely formatted boolean value that was previously interpreted as true and now silently interpreting it as false, actually add proper error handling and make it an error to specify a badly formatted value. Or if this is too much work, at least log a warning about badly formatted values. And if there has to be a visible change in behavior, document what the change is in release notes. And if the change is too niche to document in release notes, at least say what the change of behavior is in the commit message.

Alternately, it it's possible to drop a few commits in this PR to make it a boring plain refactoring, that would be great, because we'd still be eliminating the majority of misused C function calls, and just adding one or two items to your "known violations" list that could be addressed in followups.

3645e4c Add missing newline in util_ChainMerge test (Russell Yanofsky) Pull request description: This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in #17385 (comment)    ACKs for top commit: laanwj: ACK 3645e4c practicalswift: ACK 3645e4c -- diff looks correct Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165

laanwj · 2019-11-06T09:06:55Z

Closing this until the arguments manager has a way to report errors to the user. I agree that needs to be done. It'd be fairly easy to add error return value to the ArgParse functions for bool and int, however there are so many call sites… and they all assume fast and loose error handling (ie none).

I could be wrong but I think it would actually be less work, and friendlier to both users and developers to validate settings on initialization rather than use. #16545 is intended to move in this direction.

This would definitely be better. If you do this, please change the parsing function as well.

ryanofsky

Partial ACK for checked commits below: all except 3. There seems to be a possible bug in the ParseMoney commit and I think two ArgsManager commits should be dropped and followed up in a separate PR (they could use improved error handling and probably release notes).

Could reopen this PR, or open a new one with the same changes. I can follow up later with another PR later if this stays dormant.

ryanofsky · 2019-11-06T17:32:04Z

src/dbwrapper.cpp

+    if (ParseUInt64(memory, &ret)) {
+        return ret;
+    } else {
+        return 0;


In commit "refactor: Replace use of stoul in dbwrapper.cpp" (df211cb)

Should probably add log print here similar to above:

LogPrint(BCLog::LEVELDB, "Failed to parse approximate-memory-usage property\n");

There is also some risk here that this change could make DynamicMemoryUsage now return 0 in cases where it returned nonzero before (if property string has leading or trailing whitespace or other trailing characters), but logging should make it obvious if this is ever the case.

I thought about assert(0) here, but then reconsidered, because it's not important enough to crash the program. Will add a one-time log message.

ryanofsky · 2019-11-06T17:42:55Z

src/init.cpp

    for (const std::pair<const std::string, fs::path>& item : mapBlockFiles) {
-        if (atoi(item.first) == nContigCounter) {
+        uint32_t value;
+        if (ParseUInt32(item.first, &value) && value == nContigCounter) {


In commit "refactor: Replace use of atoi in CleanupBlockRevFiles" (f47450e)

Note: The behavior change here makes this code slightly more robust than before. Now if there are extra files like blkABC they will just be deleted and not throw off the count of expected block files that are supposed to be kept. Throwing off the count would cause files we actually want to keep to be deleted.

ryanofsky · 2019-11-06T17:47:13Z

src/qt/rpcconsole.cpp

@@ -222,10 +222,12 @@ bool RPCConsole::RPCParseCommandLine(interfaces::Node* node, std::string &strRes
                                UniValue subelement;
                                if (lastResult.isArray())
                                {
-                                    for(char argch: curarg)
-                                        if (!IsDigit(argch))


In commit "refactor: Replace ad-hoc validity check and atoi in rpcconsole" (5cdaf4a)

Slight change of behavior here with looser parsing since this now accepts + or - signed values. But this seems fine and doesn't need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

ryanofsky · 2019-11-06T17:48:34Z

src/bitcoin-tx.cpp

-    if (vStrInputParts.size() > 2)
-        nSequenceIn = std::stoul(vStrInputParts[2]);
+    if (vStrInputParts.size() > 2) {
+        if (!ParseUInt32(vStrInputParts[2], &nSequenceIn)) {


In commit "util: Add error handling to sequence id in bitcoin-tx" (f7f2472)

Slight change of behavior here with stricter parsing. But this seems fine and doesn't need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

ryanofsky · 2019-11-06T17:52:50Z

src/rest.cpp

-    long count = strtol(path[0].c_str(), nullptr, 10);
-    if (count < 1 || count > 2000)
+    uint32_t count;
+    if (!ParseUInt32(path[0], &count) || count < 1 || count > 2000)


In commit "refactor: Replace use of strtol in rest.cpp" (a8f7502)

Slight change of behavior here with stricter parsing: trailing characters no longer allowed for example. But this seems fine and doesn't need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

ryanofsky · 2019-11-06T18:04:51Z

src/core_read.cpp

        if (w->empty())
        {
            // Empty string, ignore. (boost::split given '' will return one word)
        }
-        else if (std::all_of(w->begin(), w->end(), ::IsDigit) ||
-            (w->front() == '-' && w->size() > 1 && std::all_of(w->begin()+1, w->end(), ::IsDigit)))
+        else if (ParseInt64(*w, &n))


In commit "refactor: Replace use of atoi64 in core_read" (e4aa1f7)

Parsing here seems a little looser than before, because ParseScript will now accept signed numbers beginning with +, while it previously only accepted numbers beginning with -. I'm assuming this is reasonable behavior. ParseScript appears to only actually be called in tests and by bitcoin-tx.

ParseIntXX accepts signed numbers beginning with +? whoops, I don't think this was intentional. Thanks for catching this, I think that needs to be documented.

Agree it's not really problematic in this specific case, but will re-add the "starts with digit or -" check just in case.

ryanofsky · 2019-11-06T18:07:44Z

src/rpc/mining.cpp

-            nTransactionsUpdatedLastLP = atoi64(lpstr.substr(64));
+            uint32_t temp;
+            if (!ParseUInt32(lpstr.substr(64), &temp)) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "could not parse lpval");


In commit "refactor: Replace use of atoi64 in rpc/mining.cpp" (b7315cb)

Slight change of behavior here with stricter parsing. For example no whitespace or trailing characters allowed anymore. But this seems fine and doesn't need additional documentation (though it could be mentioned in the commit message) since there is immediate feedback in case of an error.

ryanofsky · 2019-11-06T18:12:48Z

src/util/moneystr.cpp

@@ -68,7 +68,10 @@ bool ParseMoney(const char* pszIn, CAmount& nRet)
        return false;
    if (nUnits < 0 || nUnits > COIN)
        return false;
-    int64_t nWhole = atoi64(strWhole);
+    int64_t nWhole;
+    if (!ParseInt64(strWhole, &nWhole)) {


In commit "refactor: Replace use of atoi64 in moneystr.cpp" (2fcdd16)

I might be missing something looking at this code, but it seems like a possible bug if empty string would have been parse successfully as 0 before but now it will fail to parse. Otherwise behavior seems unchanged since strWhole can only contain digit characters.

Seems we need a test for ParseMoney that exercises that!

ryanofsky · 2019-11-06T18:17:59Z

src/wallet/wallet.h

@@ -200,12 +200,10 @@ typedef std::map<std::string, std::string> mapValue_t;

 static inline void ReadOrderPos(int64_t& nOrderPos, mapValue_t& mapValue)
 {
-    if (!mapValue.count("n"))
+    if (!mapValue.count("n") || !ParseInt64(mapValue["n"], &nOrderPos))


In commit "refactor: Replace use of atoi64 in wallet ReadOrderPos" (87a3f13)

Note: Slightly stricter parsing is done here (no whitespace or trailing characters allowed) but this should be fine since string just comes from calling i64tostr. Could potentially log or return an error though if parsing fails, since it would be an indication of corruption.

ryanofsky · 2019-11-06T18:20:48Z

src/wallet/wallet.h

@@ -414,7 +414,12 @@ class CWalletTx
        }

        ReadOrderPos(nOrderPos, mapValue);
-        nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
+        uint64_t temp;
+        if (mapValue.count("timesmart") && ParseUInt64(mapValue["timesmart"], &temp)) {


In commit "refactor: Replace use of atoi64 in timesmart unserialize" (f4218c2)

Note: Slightly stricter parsing is done here (no whitespace or trailing characters allowed) but this should be fine since string just comes from calling strprintf("%u"). Could potentially log or return an error though if parsing fails, since it would be an indication of corruption.

laanwj · 2019-11-06T18:55:35Z

Thanks for the review, have reopened and will address your comments.

This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in bitcoin#17385 (comment)

laanwj · 2019-11-07T10:32:22Z

Sorry for messing around with this again, but I realized something. Before doing this it is important to have our own, controlled, implementation of Parse[U]Int(32|64) that does not call out to libc's locale-dependent functions strtol/strtoll (these might still accept other kinds of numbers according to locale, for example). Starting here is like building a house on a foundation of jelly.

Adding WIP tag (it's not possible to change a PR to draft, unfortunately).

ryanofsky · 2019-11-07T16:18:25Z

Before doing this it is important to have our own, controlled, implementation of Parse[U]Int(32|64) that does not call out to libc's locale-dependent functions strtol/strtoll (these might still accept other kinds of numbers according to locale, for example).

Agree this would make the change safer and easier to reason about. I don't think strictly speaking most of the commits actually need this to be correct, either because looser parsings won't cause harm or because current code is already using the locale anyway. But if we can remove locale from these places, it would be a good thing.

DrahtBot · 2019-11-08T23:28:34Z

Needs rebase

This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in bitcoin#17385 (comment)

Summary: > This was causing a lot of test cases not not be very meaningful because > multiple configuration options were combined into one line. > > The changes in test output with this fix make sense and look like: > > ```diff > - testnet=1 regtest=1 || test > + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at > > most one. > ``` > Issue was reported and debugged by > Wladimir J. van der Laan <laanwj@protonmail.com> in > bitcoin/bitcoin#17385 (comment) This is a backport of Core [[bitcoin/bitcoin#17388 | PR17388]] Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8257

This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in bitcoin#17385 (comment)

laanwj added 12 commits November 5, 2019 20:04

util: Add error handling to sequence id in bitcoin-tx

f7f2472

Also removes a use of a locale-dependent function.

refactor: Replace use of stoul in dbwrapper.cpp

df211cb

Replace the direct use of `stoul` in dbwrapper.cpp. This removes a locale dependent exception.

refactor: Replace use of atoi in CleanupBlockRevFiles

f47450e

Replace the use of atoi in CleanupBlockRevFiles with ParseUInt32 to handle parse errors and remove direct use of a locale-dependent function.

refactor: Replace ad-hoc validity check and atoi in rpcconsole

5cdaf4a

refactor: Replace use of snprintf with strprintf in dbwrapper_tests

812c732

Replace use of function `snprintf` with our own, locale-independent function `strprintf`.

refactor: Replace use of strtol in rest.cpp

a8f7502

Replace the use of locale-dependent function strtol with our own error-checking integer parsing function `ParseUInt32`.

refactor: Replace use of atoi for bool parsing in system.cpp

d78e7be

Replace use of atoi with `ParseInt64`, whose output is at least well-defined. Concretely, the behaviour should not change.

refactor: Replace use of atoi in torcontrol

a8ded10

Replace use of `atoi` in torcontrol for parsing reply code with `ParseUInt32`, adding error checking. Also, change type of `code` to `uint32_t`, it is never negative does not need to be a signed integer.

refactor: Remove now-unused atoi from util

6992e0d

refactor: Replace use of atoi64 in core_read

e4aa1f7

Replace use of atoi64 and circuitois way to test if something is a valid 64-bit integer, with the use of our own function `ParseInt64`.

refactor: Replace use of atoi64 in rpc/mining.cpp

b7315cb

Replace the use of atoi64 with error and bound checking `ParseUInt32` - after all, the result is being assigned to an unsigned int.

refactor: Replace use of atoi64 in moneystr.cpp

2fcdd16

Replace the direct use of atoi64 with error-checking ParseInt64.

laanwj added the Refactoring label Nov 5, 2019

laanwj added 4 commits November 5, 2019 21:47

refactor: Replace use of atoi64 in ArgsManager::GetArg

afb5809

Replace the use of atoi64 with error-checking ParseInt64. Unfortunately, there is currently no way to return a parse error, so return the default in that case.

refactor: Replace use of atoi64 in wallet ReadOrderPos

87a3f13

Replace use of `atoi64` with error-checking `ParseInt64`.

refactor: Replace use of atoi64 in timesmart unserialize

f4218c2

Replace direct use of atoi64 with ParseUInt64 (as the result is an unsigned timestamp).

refactor: Remove no-longer-used atoi64 from util

88e8307

laanwj force-pushed the 2019_11_integer_parsing branch from 2b5ed2f to 88e8307 Compare November 5, 2019 20:48

ryanofsky mentioned this pull request Nov 5, 2019

Add missing newline in util_ChainMerge test #17388

Merged

f: add TODO about returning parse error

55ba388

This is way too lenient.

laanwj closed this Nov 6, 2019

ryanofsky mentioned this pull request Nov 6, 2019

test: Add util_ArgParsing test #17390

Merged

ryanofsky reviewed Nov 6, 2019

View reviewed changes

laanwj reopened this Nov 6, 2019

laanwj changed the title ~~refactor: Use our own integer parsing/formatting everywhere~~ WIP: refactor: Use our own integer parsing/formatting everywhere Nov 7, 2019

DrahtBot added the Needs rebase label Nov 8, 2019

laanwj mentioned this pull request Dec 12, 2019

DecodeBase58(const std::string& str, …) is a bit too liberal when decoding? #17718

Closed

laanwj closed this Jan 4, 2020

maflcko added the Up for grabs label May 7, 2020

laanwj mentioned this pull request Aug 19, 2020

refactor: remove unused c-string variant of atoi64() #19750

Merged

laanwj mentioned this pull request Nov 22, 2020

util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) #20452

Merged

maflcko mentioned this pull request Dec 22, 2021

ArgsManager::GetIntArg should error on out-of-bounds value #23843

Closed

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

WIP: refactor: Use our own integer parsing/formatting everywhere #17385

WIP: refactor: Use our own integer parsing/formatting everywhere #17385

Uh oh!

Conversation

laanwj commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

laanwj commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

laanwj commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 5, 2019

Conflicts

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

laanwj commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

laanwj commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

laanwj Nov 6, 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!

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!

laanwj commented Nov 6, 2019

Uh oh!

laanwj commented Nov 7, 2019

Uh oh!

ryanofsky commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 8, 2019

Uh oh!

Uh oh!

laanwj commented Nov 5, 2019 •

edited

Loading

practicalswift commented Nov 5, 2019 •

edited

Loading

laanwj commented Nov 5, 2019 •

edited

Loading

laanwj commented Nov 5, 2019 •

edited

Loading

Empact commented Nov 5, 2019 •

edited

Loading

laanwj commented Nov 5, 2019 •

edited

Loading

laanwj commented Nov 6, 2019 •

edited

Loading

laanwj Nov 6, 2019 •

edited

Loading

ryanofsky commented Nov 7, 2019 •

edited

Loading