Multiwallet: simplest endpoint support #10849

jonasschnelli · 2017-07-17T10:14:35Z

Alternative for #10829 and #10650.
It adds the most simplest form of wallet based endpoint support (/wallet/<filename>).
No v1 and no node/wallet endpoint split.

laanwj · 2017-07-17T10:55:59Z

Looks good to me too, utACK c8115e3.

ryanofsky · 2017-07-17T11:37:38Z

This has no documentation. If we think this approach is a good idea, I would like to see in what way it can be explained to users. Is there going to be a doc/JSPON-RPC-interface.md to complement https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md? Will endpoint be considered experimental (unchanging semantics but possibly temporary) or unstable (possibly changing semantics and possibly temporary)?

Also, from the other issue I think something like -rpcwallet=filename would be better than -usewallet=filename. It's hard to see how "use" could convey any meaning when you would expect any command line option that you specify to be used.

And, also from other issue I think '?wallet=filename makes more sense than /wallet/filename as more extensible and conventional way of passing options (as opposed to required arguments) in urls.

laanwj · 2017-07-17T11:42:47Z

And, also from other issue I think '?wallet=filename makes more sense than /wallet/filename as more extensible and conventional way of passing options (as opposed to required arguments) in urls.

Definitely no! Let's not repeat this: JSON-RPC uses POST only, and with POST it is weird to have ?a=b URL-encoded arguments in the URL as well. It also implies more arguments could be added later, which is not true. This is only to select the wallet, which is inherently hierarchical.
(also e.g. ?a=b syntax isn't deterministic, it's possible to add arguments that are ignored, change the order of the arguments, etc, whereas a /wallet/<name>/ URL is simply a fixed thing)

ryanofsky · 2017-07-17T11:47:59Z

POST it is weird to have ?a=b URL-encoded arguments in the URL as well

Don't know your source for this but this is done all the time.

It also implies more arguments could be added later, which is not true.

I don't know why it wouldn't be true. I can think of lots of RPC options you might want to add outside the RPC request: timeouts, completions handlers, etc.

?a=b syntax isn't deterministic, it's possible to add arguments that are ignored, change the order of the arguments, etc, whereas a /wallet// URL is simply a fixed thing

Not true, you don't have to ignore any option, but it is good practice to accept options in any order. This is one of the reasons people choose to used named options instead of positional arguments.

laanwj · 2017-07-17T11:49:08Z

I don't know why it it wouldn't be true, but ok. I can think of lots of RPC options you might want to add outside the RPC request: timeouts, completions handlers, etc.

Those really shouldn't be passed in the URL. The URL locates something, such as the wallet in this case.

ryanofsky · 2017-07-17T11:52:33Z

Those really shouldn't be passed in the URL. The URL locates something, such as the wallet in this case.

They shouldn't be part of the uri-path, but you would have to explain more why they shouldn't be options in a query string...

gmaxwell · 2017-07-17T11:56:12Z

ryanofsky: Among other reasons the path approach will already work with most but not all software, e.g. you change your RPC endpoint url to be one with the path, and tada-- done, no source code modifications in some things. I believe a query string would not achieve this, and in some cases require deep spelunking.

I could see an argument that something like a timeout might make sense as a query like parameter, but a wallet seems like a pretty perfect 'navigation' like element.

laanwj · 2017-07-17T12:15:10Z

Is there going to be a doc/JSPON-RPC-interface.md to complement https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md?

And yes, there should be. Documentation is good.
But that's not a requirement for the feature freeze. This is an experimental API, it could be changed completely for next release, so not having it documented 100% on first release is not a problem.
(we should make an attempt in the release notes, of course)

ryanofsky · 2017-07-17T12:17:20Z

you change your RPC endpoint url to be one with the patch, and tada-- done

This is true for this PR but not #10650 which is a big reason why I think this PR is better and less user-hostile than #10650.

@laanwj, my bigger point is about extensibility . We want to add a wallet option right now. Great, so that can be as simple as adding ?wallet=filename to the query string to provide the nice user benefit Greg cites. And maybe you have a problem with timeouts or completion handlers, but lets say we want to add a ?network=name parameter in the future (maybe to add extra way to specify that an operation is done on mainnet or testnet, or in some dystopia where there are multiple chains, but no need to go there...). The point is you want to be able to add some new option. With query strings you have a standard way to do this. ?wallet=filename&network=name&timeout=30s works just as well as ?timeout=30s&wallet=filename&network=name as established by long-running convention.

If we use paths instead, are we requiring /wallet/filename/network/name or /network/name/filename or /wallet/filename?network=name, or something else completely arbitrary based on historical accident? Or because a filename is a "location" but a network name isn't?

I guess another thing that is motivating me is that I would like to see less uri-path space annexed for this one multiwallet feature. Right now json-rpc calls go directly to / handler and REST calls go directly to /rest handler. Everything else right now (but not with these endpoint prs) is wide open right now. I'd be happier if more of the path space could be free for other applications like a web interface. If these prs added stuff under an /rpc path, that would also seem like an improvement to me.

Also, so my other points doesn't get lost above, it would be great to see something done on documentation and -usewallet if we are taking this endpoint approach. Even if there is not "100% documentation" I think it would be very helpful to see uri-path endpoints explained from user point of view.

gmaxwell · 2017-07-17T12:24:49Z

I'd be happier if more of the path space could be free for other applications like a web interface. If all this stuff was moved under an /rpc path

It's under /wallet/ at least, so I think it won't have any clashes with other use.

As far as usewallet-- I think we intend that argument to eventually specify all wallets that get loaded, not just specific to rpc. e.g. it would also tell it which ones to load in the UI when that support is complete.

ryanofsky · 2017-07-17T12:39:59Z

It's under /wallet/ at least, so I think it won't have any clashes with other use.

I could easily imagine a web interface wanting to handle /wallet path. The point is that if RPCs are restricted to exactly one path / (current case) or two paths / and /rpc then it's then it's clean and easy to do anything else with the rest of the space. If there's a bigger changing list of RPC paths, that would seem to make things messier.

As far as usewallet-- I think we intend that argument to eventually specify all wallets that get loaded, not just specific to rpc. e.g. it would also tell it which ones to load in the UI when that support is complete.

That's interesting. I thought the UI was would just have tabs or a dropdown to choose between any of the loaded wallets. But I guess the advantage of having the bitcoin-qt executable accept a -usewallet=filename instead of a more descriptive -showwallet=filename or -onlyshowwallet=filename would be that you can specify -usewallet in your bitcoind.conf and have it picked up by both bitcoin-cli and bitcoin-qt.

jnewbery · 2017-07-17T14:33:57Z

src/bitcoin-cli.cpp

@@ -241,7 +242,20 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
    assert(output_buffer);
    evbuffer_add(output_buffer, strRequest.data(), strRequest.size());

-    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
+    // check if we should use a special wallet endpoint
+    std::string endpoint;


bug: if no wallet is specified then the endpoint will be "" instead of "/"

result: bitcoin-cli doesn't work without a -usewallet parameter:

→ bitcoin-cli getinfo error: no response from server

Fix is:

std::string endpoint = "/";

jnewbery · 2017-07-17T15:25:50Z

It's a shame that this PR doesn't prevent individual wallet endpoints from accessing server-level RPCs. However that can be added at a later date.

I have a branch here: https://github.com/jnewbery/bitcoin/tree/pr10849 that does a couple of things:

fixes Multiwallet: simplest endpoint support #10849 (comment) (in commit bugfix - allow bitcoin-cli to work without -wallet parameter)
prevents bitcoin-cli from reading -wallet arguments from the conf file and removes the -usewallet argument. That means that to run a command on a wallet in multiwallet mode, the bitcoin-cli user must specify -wallet on the command line.

Feel free to squash the first commit and cherry-pick the next two if you think they're useful.

jonasschnelli · 2017-07-17T15:43:54Z

Force push fixed the endpoint issue (thanks @jnewbery).
I think we should keep it minimal with a hope this can slip in before 0.15 freeze.

Fixes can be done later.

jnewbery · 2017-07-17T16:05:32Z

Fixes can be done later.

Agree. Multiwallet interface should be considered unstable for v0.15.

achow101 · 2017-07-17T18:30:23Z

So this will remove the concept of "default wallet" from the RPCs? Any attempt to use a wallet RPC without specifying a wallet will fail, right?

jonasschnelli · 2017-07-17T18:36:03Z

@achow101: only if you call a wallet RPC and if you run with multiple wallets.

achow101 · 2017-07-17T18:39:02Z

@jonasschnelli Ok, cool!

utACK 6b9faf7

theuni · 2017-07-17T23:01:55Z

test/functional/multiwallet.py

+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3']]


Could you change one of these to just 'w' to catch possible length extension bugs? If we're asking for w2, we want to make sure that we don't get w. on accident.

theuni · 2017-07-18T00:21:09Z

Code review utACK 6b9faf7. No opinion on the endpoint discussion.

NicolasDorier · 2017-09-02T16:23:45Z

This is incompatible with RPC batching. :( (you can't batch a /wallets/ request with one which does not use it)

NicolasDorier · 2017-09-02T17:06:38Z

Ok disregard my comment. I thought that adding /wallet/w1 to a non wallet RPC method would fail. This is not the case, so no issue!

maflcko

Post-merge utACK 6b9faf7

maflcko · 2017-11-10T20:17:01Z

test/functional/multiwallet.py

+        walletinfo = w1.getwalletinfo()
+        assert_equal(walletinfo['immature_balance'], 50)
+
+        #check w1 wallet balance


fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes #10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

Summary: Merge #11743: qa: Add multiwallet prefix test fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin/bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1 Core PR11743 Fixes T177 Test Plan: ./test/functional/test_runner.py multiwallet Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: teamcity Maniphest Tasks: T177 Differential Revision: https://reviews.bitcoinabc.org/D1055

ZIP 32 preparations Includes Makefile changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7689 - bitcoin/bitcoin#10849 Part of #3380.

6b9faf7 [QA] add basic multiwallet test (Jonas Schnelli) 979d0b8 [tests] [wallet] Add wallet endpoint support to authproxy (John Newbery) 76603b1 Select wallet based on the given endpoint (Jonas Schnelli) 32c9710 Fix test_bitcoin circular dependency issue (Jonas Schnelli) 31e0720 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) dd2185c Register wallet endpoint (Jonas Schnelli) Pull request description: Alternative for bitcoin#10829 and bitcoin#10650. It adds the most simplest form of wallet based endpoint support (`/wallet/<filename>`). No v1 and no node/wallet endpoint split. Tree-SHA512: 23de1fd2f9b48d94682928b582fb6909e16ca507c2ee19e1f989d5a4f3aa706194c4b1fe8854d1d79ba531b7092434239776cae1ae715ff536e829424f59f9be

fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

6b9faf7 [QA] add basic multiwallet test (Jonas Schnelli) 979d0b8 [tests] [wallet] Add wallet endpoint support to authproxy (John Newbery) 76603b1 Select wallet based on the given endpoint (Jonas Schnelli) 32c9710 Fix test_bitcoin circular dependency issue (Jonas Schnelli) 31e0720 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) dd2185c Register wallet endpoint (Jonas Schnelli) Pull request description: Alternative for bitcoin#10829 and bitcoin#10650. It adds the most simplest form of wallet based endpoint support (`/wallet/<filename>`). No v1 and no node/wallet endpoint split. Tree-SHA512: 23de1fd2f9b48d94682928b582fb6909e16ca507c2ee19e1f989d5a4f3aa706194c4b1fe8854d1d79ba531b7092434239776cae1ae715ff536e829424f59f9be

fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

fa61c6f6a qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin/bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

fa61c6f qa: Add multiwallet prefix test (MarcoFalke) Pull request description: Fixes bitcoin#10849 (comment) Tree-SHA512: 7967be04e76d935398b3bea60c864ffc9e38dbb4cfb55890bb146a6f16c28d81ca5d89736275e2d0b03642806f6f7093beeea979f5257c464f437c4e5a9684f1

Register wallet endpoint

dd2185c

jonasschnelli added the Wallet label Jul 17, 2017

jonasschnelli mentioned this pull request Jul 17, 2017

Simple, backwards compatible RPC multiwallet support. #10829

Closed

laanwj added this to the 0.15.0 milestone Jul 17, 2017

jnewbery reviewed Jul 17, 2017

View reviewed changes

jonasschnelli and others added 5 commits July 17, 2017 17:42

Add wallet endpoint support to bitcoin-cli (-usewallet)

31e0720

Fix test_bitcoin circular dependency issue

32c9710

Select wallet based on the given endpoint

76603b1

[tests] [wallet] Add wallet endpoint support to authproxy

979d0b8

[QA] add basic multiwallet test

6b9faf7

jonasschnelli force-pushed the 2017/07/mw_endpoint_simple branch from c8115e3 to 6b9faf7 Compare July 17, 2017 15:42

theuni reviewed Jul 17, 2017

View reviewed changes

luke-jr mentioned this pull request Aug 4, 2017

RPC: Restore backward compatibility, in multiwallet mode #10989

Closed

promag mentioned this pull request Sep 13, 2017

multiwallet validateaddress tells me ismine: false without -rpcwallet arg #11324

Closed

maflcko reviewed Nov 10, 2017

View reviewed changes

maflcko mentioned this pull request Nov 21, 2017

qa: Add multiwallet prefix test #11743

Merged

str4d mentioned this pull request Aug 4, 2018

ZIP 32 preparations zcash/zcash#3447

Merged

zkbot added a commit to zcash/zcash that referenced this pull request Aug 5, 2018

Auto merge of #3447 - str4d:zip32-prep, r=str4d

8df048b

ZIP 32 preparations Includes Makefile changes cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7689 - bitcoin/bitcoin#10849 Part of #3380.

ryanofsky mentioned this pull request Nov 12, 2018

doc: Clarify rpcwallet flag url change #14448

Merged

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Multiwallet: simplest endpoint support #10849

Multiwallet: simplest endpoint support #10849

Uh oh!

Conversation

jonasschnelli commented Jul 17, 2017

Uh oh!

laanwj commented Jul 17, 2017

Uh oh!

ryanofsky commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 17, 2017

Uh oh!

gmaxwell commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jul 17, 2017

Uh oh!

ryanofsky commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 17, 2017

Uh oh!

jonasschnelli commented Jul 17, 2017

Uh oh!

jnewbery commented Jul 17, 2017

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

jonasschnelli commented Jul 17, 2017

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

theuni Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

theuni commented Jul 18, 2017

Uh oh!

NicolasDorier commented Sep 2, 2017

Uh oh!

NicolasDorier commented Sep 2, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented Jul 17, 2017 •

edited

Loading

laanwj commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 17, 2017 •

edited

Loading

laanwj commented Jul 17, 2017 •

edited

Loading

gmaxwell commented Jul 17, 2017 •

edited

Loading

laanwj commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 17, 2017 •

edited

Loading