Postgresql driver for the multi-database abstraction by cdecker · Pull Request #3057 · ElementsProject/lightning

cdecker · 2019-09-13T20:42:16Z

This is the counterpart to #2924. It implements the postgresql driver for lightningd, the changes to the query rewriting script and the changes to the testing framework to select the DB to test against.

The tests can be run, if you have postgresql installed, using the TEST_DB_PROVIDER=postgres environment variable. The tests pass, with one exception test_reserve_enforcement, which I am still investigating, and the tests are a bit flaky when running in parallel.

Sadly I had to modify the migrations in order to add the information that sqlite3 just glossed over (field size, foreign key enforcement in a transaction, autoincrement primary keys). Changing the migrations is something that I warned against back when I created the migration system, however it is safe to do here since we only reorder one single migration pair (since postgres requires the dependency table peers to be created before the dependent table channels, safe because it was part of the same git commit back in the day and there is no way someone has a DB with one table but not the other), and some changes in types.

The type changes are limited to the INTEGER changing to BIGSERIAL if it is the primary key, or BIGINT if it is backing a u64. There are one or two occurrences where I had to change from TEXT to BLOB (which then gets translated to BYTEA for postgres), since TEXT requires the contents to be valid UTF-8 strings, which public keys are not.

Finally, I was planning on using sqlparse to make the statement rewriting context aware, and safer than simple query-replace, but I found it rather cumbersome to work with, and we are just replacing a couple of types and placeholders anyway, so I ripped that dependency out again in this PR.

Some things that I will try to address before removing the draft status:

Auto-detect the location of the initdb and the postgres binaries instead of hardcoding the path
Fix test_reserve_enforcement
Run valgrind with postgres to ensure that everything is still memory-safe.

Looking forward to the feedback 😉

cdecker · 2019-09-14T14:41:12Z

Turns out test_reserve_enforcement was broken because the db.execute() call wasn't committing the DB transaction. So when we restarted, it'd still use the non-zero reserve and failing locally instead of trying and having the remote end slam the door in our face 🤦‍♂️

cdecker · 2019-09-17T11:38:14Z

This should be ready for review. The tests now pass correctly and I checked with valgrind. I didn't add a Travis-CI config for it, but I'm planning to add a quick check in our homegrown CI for it, since that's cheaper.

Ping @rustyrussell and @ZmnSCPxj 😉

rustyrussell

Minor changes only, this is both a nice addition and a good clean out of some nastiness!

rustyrussell · 2019-09-18T01:08:43Z

doc/lightningd-config.5

 .SH AUTHOR

-Rusty Russell &lt;\fIrusty@rustcorp.com.au\fR&gt; wrote this man page, and
+Rusty Russell &lt;\fBNone\fR (\fIrusty@rustcorp.com.au\fR)&gt; wrote this man page, and


This kind of gratuitous format change seems to keep happening: do we need to fixup the md files somehow?

Yep, it seems to be an issue with mrkd which assumes we have link text even on bare links. We can fix this in mrkd since @darosior has a fork, either by ignoring non-marked up links of default them to the link URL.

PR up: darosior/mrkd#1

Will switch the requirements.txt file to use that branch once the PR gets merged.

I've also PRed it upstream : refi64/mrkd#2

Nice, hadn' noticed that refi64 is still merging PRs :-)

rustyrussell · 2019-09-18T01:11:25Z

wallet/db_sqlite3.c

+		db_fatal("Could not parse the wallet DSN: %s", db->filename);
+
+	/* Strip the scheme from the dsn. */
+	filename = db->filename + strlen("sqlite3://");


Wonder if we could use a strafter helper which did this dance for us?

Sounds good, would you like 2 versions: one returning a pointer into the string and the other copying into a separate tallocated string?

I'll fix this up in my cleanups PR at the end of the week.

rustyrussell · 2019-09-18T01:13:21Z

wallet/db.h

 * generate identical names to work correctly.
 */
-#define SQL(x) NAMED_SQL( __FILE__ ":" stringify(__LINE__) ":" stringify(__COUNTER__), x)
+#define SQL(x) NAMED_SQL( __FILE__ ":" stringify(__COUNTER__), x)


Hmm, I guess we can assume all compilers support __COUNTER__. Should be fine.

Yeah, the __LINE__ macro didn't work across multiple compilers. I'm now simply using the raw string of the original query, so we could drop this again (or reinstate the __LINE__ as a debugging hint when working on the rewriter).

I was hoping to shortcut the string comparison a bit by having shorter names, rather than comparing the full statements which are longer, but generating identifiers proved to be more difficult.

rustyrussell · 2019-09-18T01:18:05Z

wallet/db.c

     * height. This may introduce a block with NULL height if we didn't have any
     * blocks, remove that in the next. */
-    {SQL("INSERT OR IGNORE INTO blocks (height) VALUES ((SELECT "
+    {SQL("INSERT INTO blocks (height) VALUES ((SELECT "


Both should be happy with INSERT .... ON CONFLICT IGNORE

~~Nope, postgres doesn't like IGNORE, it calls it DO NOTHING. How I love these little nuggets 😉 I'll add a rewrite rule for it~~

Nope, turns out sqlite3 doesn't support ON CONFLICT statements up until version 3.24 so this is not portable in any case, unless we want to make for a really weird rewrite-rule that transforms a INSERT OR IGNORE into a INSERT INTO ...loads of other stuff... ON CONFLICT DO NOTHING.

In this case I don't think it's worth it, since it predates v0.6 😉

We should make this explicit then, I think (separate patch at the end). Remove early migrations, complain if db is before that, and replace them with nicer db statements which are only used on initializing the db.

Yech, that's admitting defeat! I'll give it another go.

rustyrussell · 2019-09-18T01:18:45Z

wallet/db.c

    {SQL("ALTER TABLE invoices ADD paid_timestamp INTEGER;"), NULL},
    {SQL("UPDATE invoices"
-	 "   SET paid_timestamp = strftime('%s', 'now')"
+	 "   SET paid_timestamp = 1"


Fair enough, though we could argue this belongs in the postgres translation, this is good enough since it will Never Happen.

Yep, that was my reason for just setting a dummy value, nobody should have a DB this old, and nothing bad happens if it goes wrong.

Just to make sure I went and looked up where the migration was introduced: a88c73a

This predates v0.6 as well.

rustyrussell · 2019-09-18T01:38:58Z

wallet/wallet.c

+		/* Some databases return a NULL result if there are no
+		 * operands for the SUM(). This is the case for example for
+		 * postgres. */
+		total = AMOUNT_MSAT(0);


COALESCE(SUM(in_msatoshi - out_msatoshi), 0) is Sequelly I think?

Yeah, problem is COALESCE doesn't result in a BIGINT, so a CAST(COALESCE(field, 0) AS BIGINT) is required.

rustyrussell · 2019-09-18T01:41:25Z

wallet/wallet.c

+
+	stmt = db_prepare_v2(w->db, SQL("INSERT INTO blocks "
 					"(height, hash, prev_hash) "
 					"VALUES (?, ?, ?);"));


ON CONFLICT IGNORE? Or was the "OR IGNORE" unnecessary?

Turns out that given the above check for wallet_have_block and being in a DB transaction this should be unnecessary.

rustyrussell · 2019-09-18T01:42:20Z

wallet/wallet.c

+	if (!db_column_is_null(stmt, 0))
+		blockheight = db_column_int(stmt, 0);
+	else
+		blockheight = 0;


Maybe a db_column_int_or_null() helper for this pattern?

I had a branch with db_column_type_or_default() at some point which I may dig up again if this happens alot.

rustyrussell · 2019-09-18T01:46:54Z

wallet/wallet.c

 	stmt = db_prepare_v2(w->db,
-			     SQL("INSERT OR REPLACE INTO forwarded_payments ("
+			     SQL("INSERT INTO forwarded_payments ("
 				 "  in_htlc_id"


Upsert went into postgres 9.5, and we seem to be insisting on postgres 10 anyway?

Yep, but the ON CONFLICT(field) DO UPDATE syntax is not in sqlite3 below 3.24, so we can't use it. Splitting it into two statements was the only really portable way I found.

Yech. Nobody cares about compatibility, do they?!?

rustyrussell · 2019-09-18T01:51:22Z

wallet/wallet.c


 	stmt = db_prepare_v2(w->db, SQL("SELECT"
-					" SUM(in_msatoshi - out_msatoshi) "
+					" CAST(SUM(in_msatoshi - out_msatoshi) AS BIGINT)"


Hard to find exaclty, but I think CAST(NULL) gives 0 as expected. Might deserve a comment though?

The combination of CAST(COALESCE(field, 0) AS BIGINT) should take care of this.

Just noticed that the COALESCE call comes in in a later commit so not touching it here.

cdecker · 2019-09-18T14:51:44Z

I addressed most of the things that @rustyrussell commented on

I didn't add the upsert variant for really old migrations (all pre-dating v0.6) and I had to split the upsert that is still active because sqlite3 lower than 3.24 doesn't understand the ON CONFLICT syntax...

For easier review I created a range-diff that just contains the changes between 7c5d5de to a997888 😉 Couldn't find a way to share the colored output of range-diff which is way easier to read. If you'd like to reconstruct it this is the command: git range-diff 697b5011..7c5d5de 697b5011..a9978886

rustyrussell · 2019-09-19T00:06:19Z

Ack a997888

I would like to see that db migration cleanup though. With a message for old dbs that they have to migrate to 0.7+ first.

MINIMUM FIX is to simply refuse migrations if old db version is below 85 (v0.6.0).

If we have the client library for postgres configure will define HAVE_POSTGRES the same way it already handled libsqlite3 an we start linking against it. Signed-off-by: Christian Decker <decker.christian@gmail.com>

We will soon have a postgres backend as well, so we need a way to control the postgres process and to provision DBs to the nodes. The two interfaces are the dsn that we pass to the node, and the python query interface needed to query from tests. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Will be demuxed into starting the selected DB backend in one of the next commits. Defaults to the old database location. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Signed-off-by: Christian Decker <decker.christian@gmail.com>

We used to do some of the setup work in db.c, which is now free of any sqlite3-specific code. In addition we also switch over to fully qualified DSNs to specify the location of the wallet. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This is dangerous but needed since postgres is not as forgiving about unsatisfied foreign key constraints even while in a DB transaction. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Using a generated identifier with filename and line proved to be brittle since compilers assign the __LINE__ macro differently on multi-line macro invocations. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Needed to change a couple of migrations. The changes are mostly innocuous: - changing BLOB to TEXT for short_channel_ids which is the correct type anyway, and sqlite3 treats them the same anyway. - Use `int` for version since the byte representation is checked by postgres. - Change anything that is INT, but will be bound to u64 to BIGINT (again postgres checks these more carefully than sqlite3). Two migrations were replaced with dummy values, since they are buried deep enough, and I found no portable way of expressing `strftime()` and `INSERT OR IGNORE`. Signed-off-by: Christian Decker <decker.christian@gmail.com>

Signed-off-by: Christian Decker <decker.christian@gmail.com>

The first ever query to check if the version DB exists may fail. We allow this, but we need to restart the DB transaction since postgres fails the current transaction and rolls back any changes. This just commits (and fails) and starts a new transaction so the rest of the migration can continue. Signed-off-by: Christian Decker <decker.christian@gmail.com>

We were doing exact matches before, but prefix is sufficient. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This was weird right from the start, so we just split the table into integers and blobs, so each column has a well-defined format. It is also required for postgres not to cry about explicit casts in the `paramTypes` array. Signed-off-by: Christian Decker <decker.christian@gmail.com>

We now have an abstract rewriter that will perform some common extractions and replacements (type replacement for example), that can then be customized in derived classes. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sqlite3 doesn't really do any validation whatsoever, and there is no difference between 64bit and 32bit numbers. Posgtres on the other hand gets very upset if the size doesn't match. This commit swaps out handwavy types with the ones that should be there :-) Signed-off-by: Christian Decker <decker.christian@gmail.com>

sqlite3 will just report 0 for anything that it thinks should be numeric, or is accessed using a numeric accessor. Postgres does not, so we need to check for is_null before trying to read it. Signed-off-by: Christian Decker <decker.christian@gmail.com>

This was already done in `db_step` but `db_count_changes` and `db_last_insert_id` also rely on the statement being executed. Furthermore we now check that the statement was executed before freeing it, so it can't happen that we dispose of a statement we meant to execute but forgot. The combination of these could be used to replace the pending_statement tracking based on lists, since we now make sure to execute all statements and we use the memleak checker to make sure we don't keep a statement in memory. Signed-off-by: Christian Decker <decker.christian@gmail.com>

sqlite3 was forgiving, postgres isn't, so let's make sure we use the strictest field type possible, relaxing when rewriting. The commit consists just of the following mapping - INTEGER -> BIGSERIAL if it is the primary key - INTEGER -> BIGINT if it is an amount or a reference to a primary key Signed-off-by: Christian Decker <decker.christian@gmail.com>

The DB field type has to match the size of the accessor-type, and we had to split the `REPLACE INTO` and `INSERT INTO OR IGNORE` queries into two queries (update and insert if not updated) since there is no portable UPSERT operation, but impact should be minimal. Signed-off-by: Christian Decker <decker.christian@gmail.com>

These will not work since they touch the DB file itself. Signed-off-by: Christian Decker <decker.christian@gmail.com>

The short_channel_id is already in text format, no need to hexlify it :-) Signed-off-by: Christian Decker <decker.christian@gmail.com>

This replaces the hard-coded path to the `postgres` and `initdb` binaries with a slightly more flexible search. It'll pick the newest version installed. Signed-off-by: Christian Decker <decker.christian@gmail.com>

The test was implicitly relying on us selecting the larger output and then not touching the smaller, leaving it there for the final `withdraw` to claim. This ordering of UTXOs is not guaranteed, and in particular can fail when switching DB backends. To stabilize we just need to make sure to select the change output as well.

cdecker · 2019-09-20T18:36:58Z

Ok, I looked a little bit harder and it turns out rewriting INSERT OR REPLACE INTO into the standard SQL upsert form is rather easy, so I did that instead of dumbing down the statements. This way we don't have to break backward compatibility and everybody is happy :-)

I rebased and squashed right away, but here's the range-diff for between the reviewed version and the current head:

 1:  9d7560e63 =  1:  52a2a1e27 make: Add configuration detection and linking of libpq
 2:  71953a205 =  2:  4f2c8a27f pytest: Add db_provider and db instances for configurable backends
 3:  f0816fb50 =  3:  42118f221 cli: Add command line option to specify the wallet location
 4:  25d29a16d =  4:  5d1ac31c7 postgres: Add postgres statement rewriting support
 5:  7f92a07ad !  5:  30af21620 db: Move remainder of the sqlite3 into the apropriate file
    @@ -137,24 +137,6 @@
      	err = sqlite3_step(stmt);
      	sqlite3_finalize(stmt);
     @@
    - #if !HAVE_SQLITE3_EXPANDED_SQL
    - 	/* Register the tracing function if we don't have an explicit way of
    - 	 * expanding the statement. */
    --	sqlite3_trace(db->sql, trace_sqlite3, stmt);
    -+	sqlite3_trace(db->conn, trace_sqlite3, stmt);
    - #endif
    - 
    - 	if (!db_sqlite3_query(stmt)) {
    -@@
    - #else
    - 	/* Unregister the trace callback to avoid it accessing the potentially
    - 	 * stale pointer to stmt */
    --	sqlite3_trace(db->sql, NULL, NULL);
    -+	sqlite3_trace(db->conn, NULL, NULL);
    - #endif
    - 
    - 	return true;
    -@@
      
      static void db_sqlite3_close(struct db *db)
      {
 6:  524c8a010 =  6:  bdcda5573 db: Reorder migrations to reflect their relationship
 7:  8cb65ba36 =  7:  6d0172dc1 db: Switch statement lookup to use the original query instead
 8:  c8635a29e !  8:  3bea36c80 db: Adjust some db migrations to be compatible with postgres
    @@ -61,7 +61,7 @@
          {SQL("ALTER TABLE invoices ADD paid_timestamp INTEGER;"), NULL},
          {SQL("UPDATE invoices"
     -	 "   SET paid_timestamp = strftime('%s', 'now')"
    -+	 "   SET paid_timestamp = 1"
    ++	 "   SET paid_timestamp = CURRENT_TIMESTAMP()"
      	 " WHERE state = 1;"),
           NULL},
          /* We need to keep the route node pubkeys and short channel ids to
 9:  55427326c =  9:  ebe1e6ff3 db: Implement postgres driver primitives
10:  7dde498c9 = 10:  628a4a1ca db: Allow some internal queries to fail
11:  c02078fdf = 11:  d1a6f2013 db: Select driver by dsn prefix
12:  be77ae6f1 = 12:  2ccb97226 db: Split the vars table to have type-specific columns
13:  47bc85bc0 ! 13:  1c17a9cc7 db: Implement SQL statement rewriting
    @@ -63,9 +63,11 @@
     +class Sqlite3Rewriter(Rewriter):
     +    def rewrite_single(self, query):
     +        typemapping = {
    -+            'BIGINT': 'INTEGER',
    -+            'BIGINTEGER': 'INTEGER',
    -+            'BIGSERIAL': 'INTEGER',
    ++            r'BIGINT': 'INTEGER',
    ++            r'BIGINTEGER': 'INTEGER',
    ++            r'BIGSERIAL': 'INTEGER',
    ++            r'CURRENT_TIMESTAMP\(\)': "strftime('%s', 'now')",
    ++            r'INSERT INTO[ \t]+(.*)[ \t]+ON CONFLICT.*DO NOTHING;': 'INSERT OR IGNORE INTO \\1;',
     +        }
     +        return self.rewrite_types(query, typemapping)
     +
    @@ -84,6 +86,7 @@
     +
     +        typemapping = {
     +            r'BLOB': 'BYTEA',
    ++            r'CURRENT_TIMESTAMP\(\)': "EXTRACT(epoch FROM now())",
     +        }
     +
     +        query = self.rewrite_types(query, typemapping)
14:  c12fd9fe6 = 14:  977c535b5 db: Change migrations to use types of the correct cardinality
15:  5d183b02f = 15:  2c6bcb7b6 db: Strengthen some null-checks on queries
16:  83bd8ff4d = 16:  6f4cc8b9d db: Check execution when accessing the result of a statement
17:  51b8fa0e4 ! 17:  e489527fc db: Change table field types to be more specific
    @@ -164,7 +164,7 @@
     -    {SQL("ALTER TABLE invoices ADD paid_timestamp INTEGER;"), NULL},
     +    {SQL("ALTER TABLE invoices ADD paid_timestamp BIGINT;"), NULL},
          {SQL("UPDATE invoices"
    - 	 "   SET paid_timestamp = 1"
    + 	 "   SET paid_timestamp = CURRENT_TIMESTAMP()"
      	 " WHERE state = 1;"),
     @@
           * because we cannot safely save them as blobs due to byteorder
18:  b9b28f71b = 18:  9b8c9accc db: Adjust queries to work with postgres
19:  12e1a6448 = 19:  d8bd6e9aa pytest: Skip some tests that assume we have a sqlite3 db on postgres
20:  799b1c9e5 = 20:  4c2a7b482 pytest: Consolidate fee-fetching in test_setchannelfee_usage
21:  d66481ea3 = 21:  514471754 pytest: Have the DB provider search for the postgres binary
22:  a9978886c = 22:  bfce0ba58 pytest: Stabilize test_no_fee_estimate against UTXO selection issues
 -:  --------- > 23:  ccdd5995b fixup! wallet: Annotate migrations using the SQL macro

Notice that whenever possible I chose the standardized syntax over the sqlite3 specific, and then rewrote the sqlite3 version, hopefully that reduces the need for many rewrite rules for future DBs.

cdecker requested a review from rustyrussell September 13, 2019 20:42

cdecker force-pushed the multi-db-postgres branch from 220ba45 to ada5925 Compare September 14, 2019 14:39

cdecker force-pushed the multi-db-postgres branch 3 times, most recently from 031b30c to 7c5d5de Compare September 17, 2019 10:34

cdecker marked this pull request as ready for review September 17, 2019 11:33

cdecker requested a review from ZmnSCPxj as a code owner September 17, 2019 11:33

rustyrussell approved these changes Sep 18, 2019

View reviewed changes

cdecker force-pushed the multi-db-postgres branch from 7c5d5de to a997888 Compare September 18, 2019 14:46

cdecker added 7 commits September 20, 2019 18:49

make: Add configuration detection and linking of libpq

52a2a1e

If we have the client library for postgres configure will define HAVE_POSTGRES the same way it already handled libsqlite3 an we start linking against it. Signed-off-by: Christian Decker <decker.christian@gmail.com>

cli: Add command line option to specify the wallet location

42118f2

Will be demuxed into starting the selected DB backend in one of the next commits. Defaults to the old database location. Signed-off-by: Christian Decker <decker.christian@gmail.com>

postgres: Add postgres statement rewriting support

5d1ac31

Signed-off-by: Christian Decker <decker.christian@gmail.com>

db: Reorder migrations to reflect their relationship

bdcda55

This is dangerous but needed since postgres is not as forgiving about unsatisfied foreign key constraints even while in a DB transaction. Signed-off-by: Christian Decker <decker.christian@gmail.com>

db: Switch statement lookup to use the original query instead

6d0172d

Using a generated identifier with filename and line proved to be brittle since compilers assign the __LINE__ macro differently on multi-line macro invocations. Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker force-pushed the multi-db-postgres branch from a997888 to ccdd599 Compare September 20, 2019 18:30

cdecker added 9 commits September 20, 2019 20:32

db: Implement postgres driver primitives

6a7c92a

Signed-off-by: Christian Decker <decker.christian@gmail.com>

db: Select driver by dsn prefix

5d94267

We were doing exact matches before, but prefix is sufficient. Signed-off-by: Christian Decker <decker.christian@gmail.com>

db: Implement SQL statement rewriting

5f92f9f

We now have an abstract rewriter that will perform some common extractions and replacements (type replacement for example), that can then be customized in derived classes. Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker added 6 commits September 20, 2019 20:32

pytest: Skip some tests that assume we have a sqlite3 db on postgres

5e4c3af

These will not work since they touch the DB file itself. Signed-off-by: Christian Decker <decker.christian@gmail.com>

pytest: Consolidate fee-fetching in test_setchannelfee_usage

4abbd6f

The short_channel_id is already in text format, no need to hexlify it :-) Signed-off-by: Christian Decker <decker.christian@gmail.com>

pytest: Have the DB provider search for the postgres binary

a4dc503

This replaces the hard-coded path to the `postgres` and `initdb` binaries with a slightly more flexible search. It'll pick the newest version installed. Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker force-pushed the multi-db-postgres branch from ccdd599 to 951193e Compare September 20, 2019 18:33

rustyrussell merged commit 9915386 into ElementsProject:master Sep 22, 2019

Conversation

cdecker commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker commented Sep 14, 2019

Uh oh!

cdecker commented Sep 17, 2019

Uh oh!

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

rustyrussell Sep 18, 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!

cdecker Sep 18, 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!

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!

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!

cdecker commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker commented Sep 20, 2019

cdecker commented Sep 13, 2019 •

edited

Loading

rustyrussell Sep 18, 2019 •

edited

Loading

cdecker Sep 18, 2019 •

edited

Loading

cdecker commented Sep 18, 2019 •

edited

Loading

rustyrussell commented Sep 19, 2019 •

edited

Loading