Hardcode CSV and SEGWIT deployment #11398

jl2012 · 2017-09-25T10:06:04Z

Similar to BIP90, this hardcode the deployment of CSV softfork, which has been active for over a year.

gmaxwell

Concept ACK

dcousens

concept ACK

jnewbery

Concept ACK. I've lightly reviewed the code and left a few comments inline.

Some high-level feedback:

I think this needs a BIP or announcement (as per Consensus: Remove ISM #8391 (comment))
Why set CSV height to 576 in regtest, instead of 432 where it currently activates?
I haven't tested all the extended tests, but bip9-softforks.py certainly fails. I think we may be able to just remove that test entirely at this point.

jnewbery · 2017-09-26T15:10:50Z

src/rpc/blockchain.cpp

+            "        \"height\": \"xxxxxx\",  (numeric) height of the first block which the rules are enforced\n"
+            "        \"type\": \"xxxx\",      (string) original activation mechanism (\"ism\" or \"bip9\")\n"
+            "        \"version\": xx,         (numeric) block version (only for \"ism\" type)\n"
+            "        \"reject\": {            (object) progress toward rejecting pre-softfork blocks (only for \"ism\" type)\n"


why not combine ['reject']['status'] with ['active'] below? Once these buried softforks are pinned, an active ISM softfork is equivalent to an active BIP9 softfork.

No special reason. I just want to keep existing response untouched

I honestly think ['reject']['status'] makes almost no sense in a post-ISM world. This PR is already a breaking API change (getblockchaininfo RPC no longer returns bip9_softforks['csv']) so why not use the opportunity to rationalize the way the information is presented to the user?

ok. Then I prefer to just unify softforks and bit_softforks, so we don't need to break the API everytime we bury a softfork

jnewbery · 2017-09-26T15:11:46Z

test/functional/bip68-sequence.py

        height = self.nodes[0].getblockcount()
        assert(height < min_activation_height)
        self.nodes[0].generate(min_activation_height-height)
-        assert(get_bip9_status(self.nodes[0], 'csv')['status'] == 'active')
+        assert(get_buried_softforks_status(self.nodes[0], 'csv'))


assert is a statement, not a function. This should be:

assert get_buried_softforks_status(self.nodes[0], 'csv')

jnewbery · 2017-09-26T15:12:27Z

test/functional/test_framework/util.py

@@ -308,6 +308,15 @@ def get_bip9_status(node, key):
    info = node.getblockchaininfo()
    return info['bip9_softforks'][key]

+def get_buried_softforks_status(node, key):


nit: prefer a name like buried_softfork_active(), since this is returning a bool (True if the softfork is active)

jnewbery · 2017-09-26T15:14:19Z

test/functional/test_framework/util.py

+                return fork['reject']['status']
+            else:
+                return fork['active']
+    assert(not "unknown softfork")


I don't understand what this is supposed to be doing. This will always assert if we drop down to this line (a string has truthiness True, so not "string" is always False)

It would drop down to this line only if key is not found in the results of getblockchaininfo(). I want it to exit with the error "unknown softfork" printed

ok, in that case you can be much more compact:

def get_buried_softforks_status(node, key): softforks = [sf for sf in node.getblockchaininfo()['softforks'] if sf['id'] == key] assert softforks, "get_buried_softforks_status() called with invalid softfork %s" % key return softforks[0]['active']

(assuming you make the change from ['reject']['status'] with ['active'] suggested above)

jl2012 · 2017-09-26T17:34:35Z

@jnewbery

Yes it needs a BIP. Not sure if it should be added to BIP90, or have a new BIP
It's because bip68-112-113-p2p.py activates it at 576

jnewbery · 2017-09-26T18:40:09Z

test/functional/bip68-112-113-p2p.py

@@ -13,11 +13,8 @@
 activation after a further 144 blocks


Can remove these two lines

not addressed in 48e683d18d4d3d995e7192048d4182c4c0eac0be

jnewbery · 2017-09-26T18:45:08Z

src/chainparams.cpp

@@ -271,6 +263,7 @@ class CRegTestParams : public CChainParams {
        consensus.BIP34Hash = uint256();
        consensus.BIP65Height = 1351; // BIP65 activated on regtest (Used in rpc activation tests)
        consensus.BIP66Height = 1251; // BIP66 activated on regtest (Used in rpc activation tests)
+        consensus.CSVHeight = 576; // CSV activated on regtest (Used in rpc activation tests)


I suggest you change this to 432 to avoid having to change bip68-sequence.py

jnewbery · 2017-09-26T18:45:25Z

test/functional/bip68-112-113-p2p.py

-        assert_equal(get_bip9_status(self.nodes[0], 'csv')['status'], 'defined')
-        test_blocks = self.generate_blocks(61, 4)
+        # Activation height is hardcoded and the softfork will active with any block version
+        test_blocks = self.generate_blocks(489, 4)


Suggestion: Change this to 345

jnewbery · 2017-09-26T18:45:39Z

test/functional/bip68-112-113-p2p.py

        # Not yet advanced to ACTIVE, height = 574 (will activate for block 576, not 575)
-        assert_equal(get_bip9_status(self.nodes[0], 'csv')['status'], 'locked_in')
+        assert(not get_buried_softforks_status(self.nodes[0], 'csv'))


assert is a statement, not a function

gmaxwell · 2017-09-27T00:56:22Z

If this is getting a BIP (I agree thats reasonable) it should perhaps be bundled with the ones for BIP143/BIP147.

jl2012 · 2017-09-27T18:16:29Z

As suggested I made another commit to hardcode SEGWIT deployment. To make it compatible with #11403, segwit is always active on regtest, unless overridden

jnewbery

Concept ACK pinning the segwit activation height. I know that @sdaftuar was working on something similar, so you should link up with him.

A few more comments inline.

jnewbery · 2017-09-27T19:45:15Z

src/rpc/blockchain.cpp

+    softforks.push_back(BuriedSoftForkDesc("bip34", 2, tip, consensusParams.BIP34Height));
+    softforks.push_back(BuriedSoftForkDesc("bip66", 3, tip, consensusParams.BIP66Height));
+    softforks.push_back(BuriedSoftForkDesc("bip65", 4, tip, consensusParams.BIP65Height));
+    softforks.push_back(BuriedSoftForkDesc("csv", 9, tip, consensusParams.CSVHeight));


9 seems like an abuse of the version field here

jnewbery · 2017-09-27T19:47:52Z

src/rpc/blockchain.cpp

-
-static UniValue SoftForkDesc(const std::string &name, int version, CBlockIndex* pindex, const Consensus::Params& consensusParams)
+/** For buried softforks */
+static UniValue BuriedSoftForkDesc(const std::string &name, int version, CBlockIndex* pindex, const int &height)


just pass height by value

jnewbery · 2017-09-27T19:50:04Z

test/functional/bip68-112-113-p2p.py

@@ -13,11 +13,8 @@
 activation after a further 144 blocks


not addressed in 48e683d18d4d3d995e7192048d4182c4c0eac0be

jnewbery · 2017-09-27T19:52:38Z

test/functional/bip68-112-113-p2p.py

-mine 140 blocks and seed block chain with the 82 inputs will use for our tests at height 572
-mine 3 blocks and verify still at LOCKED_IN and test that enforcement has not triggered
+mine 345 blocks and seed block chain with the 82 inputs will use for our tests at height 428
+mine 3 blocks and verify still not ACTIVE and test that enforcement has not triggered
 mine 1 block and test that enforcement has triggered (which triggers ACTIVE)
 Test BIP 113 is enforced
 Mine 4 blocks so next height is 580 and test BIP 68 is enforced for time and height


This (and other comments in this test) need to be changed to reflect CSV activation at height 432

jnewbery · 2017-09-27T19:58:19Z

src/init.cpp

@@ -442,6 +442,7 @@ std::string HelpMessage(HelpMessageMode mode)
        strUsage += HelpMessageOpt("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT));
        strUsage += HelpMessageOpt("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT));
        strUsage += HelpMessageOpt("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)");
+        strUsage += HelpMessageOpt("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)");


sort alphabetically?

jnewbery · 2017-09-27T19:59:47Z

src/init.cpp

+        if (!chainparams.MineBlocksOnDemand()) {
+            return InitError("Segwit activation parameters may only be overridden on regtest.");
+        }
+        int nHeight = gArgs.GetArg("-segwitheight", chainparams.GetConsensus().SEGWITHeight);


nit: no hungarian notation please. segwit_height or height should do.

jnewbery · 2017-09-27T20:06:46Z

src/validation.cpp

@@ -2844,8 +2844,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P

 bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params)
 {
-    LOCK(cs_main);
-    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == THRESHOLD_ACTIVE);
+    int nHeight = pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1;


nit: no hungarian notation.

You could just do away with the local variable entirely:

return ((int)chainActive.Height() >= params.SEGWITHeight);

Many different places use IsWitnessEnabled but not all look at the tip

jnewbery · 2017-09-27T20:08:10Z

src/validation.cpp

@@ -2880,7 +2880,7 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
    std::vector<unsigned char> commitment;
    int commitpos = GetWitnessCommitmentIndex(block);
    std::vector<unsigned char> ret(32, 0x00);
-    if (consensusParams.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout != 0) {
+    if (gArgs.GetArg("-segwitheight", consensusParams.SEGWITHeight) >= 0) {


why do you need to check the -segwitheight argument here? Hasn't consensusParams.SEGWITHeight been updated by the time this function is called?

I try to keep the current behavior that the witness commitment is not made if segwit is disabled (before: nTimeout == 0 / after: SEGWITHeight < 0). If you use -segwitheight=-1, it should work identical to a non-segwit node. This is needed for some tests in p2p-segwit.py related to the witness commitment. (we could remove those tests, of course)

ok. Maybe I should use consensusParams.SEGWITHeight != std::numeric_limits<int>::max() instead

sorry, I misunderstood. I think your existing change is fine (and I agree that we should remove this at a later date once the tests are updated)

jtimon · 2017-09-30T12:27:38Z

Concept ACK, I was about to write this same thing on top of #11426
Would you consider rebasing on top of it? I think it would make the code slightly nicer (you can just move DEPLOYMENT_CSV and DEPLOYMENT_SEGWIT from DeploymentPos to BuriedDeploymentPos instead of having to declare consensusParams.CSVHeight and consensusParams.SEGWITHeight ).

jl2012 · 2017-09-30T12:49:29Z

@jtimon will do

jl2012 · 2017-09-30T17:38:35Z

rebased on #11426

jtimon · 2017-09-30T18:09:46Z

Review by stages, so far up to 9ff60be

EDIT: perhaps it would be nice to do the rpc preparations before chaging anything from bip9 to bip90.

Although I know some people don't like dividing things so much, I wouldn't complain about this rebasing on top of a PR with all the common preparations and the move only for csv (than whatever extra test preparations for segwit and the move for segit in this PR).

jnewbery · 2017-10-03T20:53:10Z

I don't think this should be dependant on #11426. I'm indifferent to whether #11426 is merged, but there's at least some disagreement as to whether it's beneficial.

This PR had already received some code review prior to the rebase, so I think it makes sense to leave it based on master.

sdaftuar · 2017-10-10T19:46:45Z

Concept ACK. As @jnewbery pointed out in #11389 (review), the script interpreter doesn't allow SCRIPT_VERIFY_WITNESS if P2SH is not also set. I think we can just pin P2SH to also always be on in regtest to ensure this never happens?

sipa · 2017-10-10T20:34:21Z

Perhaps this is beyond the scope of this PR, but I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing (and instead plug in the burying into the BIP9 status logic, so that it just always returns DEFINED before the bury height, and always ACTIVE after?). Advantages:

Easier to review the change that buries a deployment.
Support making a deployment buried in regtest while it isn't yet in mainnet/testnet (which can simplify functional testing)

maflcko · 2017-11-10T18:28:34Z

Needs rebase

jtimon · 2017-11-11T19:53:34Z

src/rpc/blockchain.cpp

+    // This is used when softfork codes are merged without specifying the deployment schedule.
+    if (consensusParams.vDeployments[id].nTimeout <= 1230768000)
+        return;
+    UniValue bip9(UniValue::VOBJ);


bikeshed: s/bip9/versionbits/ ?

jnewbery · 2018-01-26T19:05:37Z

Needs rebase. @jl2012 - is this abandoned? I think it'd be great to get it merged around the same time as #11739 to formalize segwit activation rules and clear up the testing (although there are no dependencies between the two PRs).

I still think you should remove the dependency on #11426.

jnewbery · 2018-02-05T17:23:48Z

It appears that this PR is abandoned. I've opened #12360 as a replacement.

fanquake · 2018-02-05T22:24:13Z

Closing in favour of #12360

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415) - The return format from `addmultisigaddress` has changed (#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

e78aaf4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery) 8319e73 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne) 0328dcd [Consensus] Bury segwit deployment (John Newbery) 1c93b9b [Consensus] Bury CSV deployment height (John Newbery) 3862e47 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery) Pull request description: This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66. CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt. This was originally attempted by jl2012 in #11398 and again by me in #12360. ACKs for top commit: ajtowns: ACK e78aaf4 ; checked diff to previous acked commit, checked tests still work ariard: ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected. MarcoFalke: ACK e78aaf4 (still didn't check if the mainnet block heights are correct, but the code looks good now) Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (bitcoin#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (bitcoin#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (bitcoin#11415) - The return format from `addmultisigaddress` has changed (bitcoin#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after bitcoin#11739 and bitcoin#11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

e48826a tests: remove ComputeBlockVersion shortcut from versionbits tests (Anthony Towns) c5f3672 [refactor] Move ComputeBlockVersion into VersionBitsCache (Anthony Towns) 4a69b4d [move-only] Move ComputeBlockVersion from validation to versionbits (Anthony Towns) 0cfd6c6 [refactor] versionbits: make VersionBitsCache a full class (Anthony Towns) 8ee3e0b [refactor] rpc/blockchain.cpp: SoftForkPushBack (Anthony Towns) 92f48f3 deploymentinfo: Add DeploymentName() (Anthony Towns) ea68b3a [move-only] Rename versionbitsinfo to deploymentinfo (Anthony Towns) c64b2c6 scripted-diff: rename versionbitscache (Anthony Towns) de55304 [refactor] Add versionbits deployments to deploymentstatus.h (Anthony Towns) 2b0d291 [refactor] Add deploymentstatus.h (Anthony Towns) eccd736 versionbits: Use dedicated lock instead of cs_main (Anthony Towns) 36a4ba0 versionbits: correct doxygen comments (Anthony Towns) Pull request description: Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from [11398](#11398 (comment)) "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing". This provides three functions: `DeploymentEnabled()` which tests if a deployment can ever be active, `DeploymentActiveAt()` which checks if a deployment should be enforced in the given block, and `DeploymentActiveAfter()` which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments. This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on `cs_main`. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation. ACKs for top commit: jnewbery: ACK e48826a gruve-p: ACK e48826a MarcoFalke: re-ACK e48826a 🥈 Tree-SHA512: c846ba64436d36f8180046ad551d8b0d9e20509b9bc185aa2639055fc28803dd8ec2d6771ab337e80da0b40009ad959590d5772f84a0bf6199b65190d4155bed

laanwj added the Consensus label Sep 25, 2017

jl2012 force-pushed the csvburied branch from 221a40d to 947d28a Compare September 25, 2017 10:38

gmaxwell approved these changes Sep 25, 2017

View reviewed changes

dcousens approved these changes Sep 26, 2017

View reviewed changes

jnewbery reviewed Sep 26, 2017

View reviewed changes

jl2012 force-pushed the csvburied branch from 947d28a to 29a3e32 Compare September 27, 2017 18:14

jl2012 force-pushed the csvburied branch from 29a3e32 to 48e683d Compare September 27, 2017 18:20

jl2012 changed the title ~~Hardcode CSV deployment~~ Hardcode CSV and SEGWIT deployment Sep 27, 2017

jnewbery reviewed Sep 27, 2017

View reviewed changes

jtimon and others added 3 commits September 30, 2017 15:10

BIP90: Make buried deployments slightly more easily extensible

4a87820

Hardcode CSV deployment

9ff60be

Hardcode SEGWIT deployment

ff357fd

jl2012 force-pushed the csvburied branch 2 times, most recently from ff12542 to eae4a2d Compare September 30, 2017 17:25

jl2012 added 2 commits October 2, 2017 03:47

[rpc] Combine reporting of buried and ongoing softforks

ae3ed97

[test] Test for buried CSV and SEGWIT deployments

92c5ecc

jl2012 force-pushed the csvburied branch from eae4a2d to 92c5ecc Compare October 1, 2017 19:50

jtimon mentioned this pull request Oct 4, 2017

BIP90: Make buried deployments slightly more easily extensible #11426

Closed

sdaftuar mentioned this pull request Oct 10, 2017

Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389

Merged

jtimon reviewed Nov 11, 2017

View reviewed changes

jnewbery mentioned this pull request Jan 26, 2018

Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis #11739

Merged

This was referenced Feb 2, 2018

Remove deprecated rpc options #12336

Merged

Bury bip9 deployments #12360

Closed

fanquake closed this Feb 5, 2018

jnewbery mentioned this pull request May 20, 2019

Bury bip9 deployments #16060

Merged

jnewbery mentioned this pull request Jul 2, 2019

Standardise deployment handling #16229

Closed

ajtowns mentioned this pull request Jul 3, 2020

Introduce deploymentstatus #19438

Merged

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

Hardcode CSV and SEGWIT deployment #11398

Hardcode CSV and SEGWIT deployment #11398

Conversation

jl2012 commented Sep 25, 2017

gmaxwell left a comment

Choose a reason for hiding this comment

dcousens left a comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl2012 commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Sep 27, 2017

jl2012 commented Sep 27, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl2012 Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Sep 30, 2017

jl2012 commented Sep 30, 2017

jl2012 commented Sep 30, 2017

jtimon commented Sep 30, 2017 • edited Loading

jnewbery commented Oct 3, 2017

sdaftuar commented Oct 10, 2017

sipa commented Oct 10, 2017

maflcko commented Nov 10, 2017

Choose a reason for hiding this comment

jnewbery commented Jan 26, 2018

jnewbery commented Feb 5, 2018

fanquake commented Feb 5, 2018

jl2012 Sep 28, 2017 •

edited

Loading

jtimon commented Sep 30, 2017 •

edited

Loading