[0.17] Add initialfreecoins option, default of 0 #467

dongcarl · 2018-11-15T00:30:33Z

This PR adds a -initialfreecoins debug option, and a -con_genesis_style to go along with it.

This allows us to correctly set the amount of coins created in the genesis block, either in the elements style or bitcoin style.

Tests have been modified to test that the correct amount of coins are created.

stevenroose · 2018-11-15T17:23:48Z

Relevant section: 7fc252d#diff-64cbe1ad5465e13bc59ee8bb6f3de2e7R516

instagibbs · 2018-12-04T16:18:19Z

@dongcarl this being worked on?

dongcarl · 2018-12-04T16:22:03Z

@instagibbs Yes. I remember this one needs to be rebased on some other changes (maybe issuance consensus/policy?) before it can be fleshed out.

instagibbs · 2018-12-07T15:31:09Z

src/chainparams.cpp

@@ -435,6 +435,7 @@ class CCustomParams : public CRegTestParams {
        // All non-zero coinbase outputs must go to this scriptPubKey
        std::vector<unsigned char> man_bytes = ParseHex(gArgs.GetArg("-con_mandatorycoinbase", ""));
        consensus.mandatory_coinbase_destination = CScript(man_bytes.begin(), man_bytes.end()); // Blank script allows any coinbase destination
+        initialFreeCoins = gArgs.GetArg("-initialfreecoins", 0);


use DEFAULT_INITIAL_FREE_COINS

Would cause circular dependencies... Just used 0

well that constant doesn't appear to be used then

@instagibbs It would seem from surrounding code that the convention is to just specify instead of making a new constant. So I've removed it.

dongcarl · 2018-12-11T02:20:11Z

Seems like rpc_getblockstats.py is still not happy, it does pass locally on my machine, and it seems like blocks just aren't connecting for some reason.

instagibbs · 2018-12-11T14:27:13Z

@dongcarl it's failing the "bitcoind tests" run specifically. Try running it locally using ./test/bitcoin_functional/functional/test_runner.py

instagibbs · 2018-12-11T14:58:50Z

So this is probably because this PR breaks the ability to make and run "vanilla regtest" blockchain, breaking the bitcoin_functional run that uses pre-generated data.

See: #436

I think it makes sense to do a minimal version of that PR, supporting Bitcoin-compatible genesis block generation, as well as elements/liquid-compatible.

instagibbs · 2018-12-11T15:45:25Z

Ok, we "broke" bitcoin regtest block previously to adding the new tests, so I'd suggest you take this, generate the new json block data in the bitcoin_functional section of tests: instagibbs@0447266

This would let us have bitcoin-compatible style blocks for regtest under custom chains, and allow adding more options over time.

stevenroose · 2018-12-11T16:28:41Z

I guess the pegin PR will rebase once this one lands. So I'll add -genesis_style=bitcoin to the tests once this lands.

instagibbs · 2018-12-11T16:32:59Z

otherwise this is looking good!

instagibbs · 2018-12-14T15:58:55Z

unmarked as WIP

instagibbs · 2018-12-14T16:00:08Z

nit: Could you just squash the commits to make all commits past the test?

instagibbs · 2018-12-14T16:22:17Z

test/functional/feature_connect_coinbase.py

+
+        # Issuance transaction is an OP_TRUE, so will be available to second node
+        assert_raises_rpc_error(-5, "No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.", self.nodes[0].getrawtransaction, issuance_tx)
+        self.nodes[1].getrawtransaction(issuance_tx)


Please add a test showing that listunspent has a single(only one!) output worth the amount you set in the conf line: 5000000000:

e.g.,

[ <--- assert there's only one result { "txid": "b22b529c27b20746d5fb658d6e53d9c839304b51a98ebcfae6a4b77bbbdeb9cf", "vout": 0, <--- assert it is the first "scriptPubKey": "51", <---- assert it's an op_true "amount": 21000000.00000000, <--- assert the value "confirmations": 1, <---- assert it's at the right confirmation level for genesis "spendable": true, "solvable": true, "safe": true } ]

This would require anyonecanspendaremine=1 correct? Should I add that flag just for this test or shall we make this the default at least for the non-bitcoin functional tests?

Oh right, I'd set it just for this test.

instagibbs · 2018-12-14T16:31:12Z

light tACK, just add those quick test conditions to make sure the output looks like something you expect

dongcarl · 2018-12-18T02:33:23Z

@instagibbs Tests added, let me know if there are other things!

instagibbs · 2018-12-18T17:18:12Z

re-ACK 9b0632b

stevenroose

1 piece of confusion and some nits..

stevenroose · 2018-12-18T17:23:08Z

src/chainparams.cpp

+// Safer for users if they load incorrect parameters via arguments.
+static std::vector<unsigned char> CommitToArguments(const Consensus::Params& params, const std::string& networkID)
+{
+  CSHA256 sha2;


2-spaces indentation, really? :D

stevenroose · 2018-12-18T17:24:32Z

src/chainparams.cpp

+            std::vector<unsigned char> commit = CommitToArguments(consensus, strNetworkID);
+            genesis = CreateGenesisBlock(CScript(commit), CScript(OP_RETURN), 1296688602, 2, 0x207fffff, 1, 0);
+            if (initialFreeCoins != 0) {
+              AppendInitialIssuance(genesis, COutPoint(uint256(commit), 0), initialFreeCoins, CScript() << OP_TRUE);


Another indentation nit..

stevenroose · 2018-12-18T17:26:18Z

test/bitcoin_functional/functional/data/rpc_getblockstats.json

@@ -1,117 +1,117 @@
 {
  "blocks": [
-    "010000000000000000000000000000000000000000000000000000000000000000000000bdb84e02590a29888f409285dfe25361bbd5560efa5a2dd6996c53488bf37e8edae5494dffff7f20020000000101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff1004ffff001d0104087265677465737432ffffffff0100f2052a01000000015100000000",


Shouldn't it be possible to have this be compatible with bitcoin? With the genesis style and 0 initialfreecoins?

good catch, yes with these commits they should match again

Well technically no, it's "regtest2" network, which gets mixed into the block definition. The network main still should work though. We don't really have tests for that per se, aside from assertions in the chainparams such as:

assert(consensus.hashGenesisBlock == uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f")); assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));

Not sure what the conclusion is here, does this need to be addressed?

I don't think this is a blocker unless this mutation is incorrect behavior based on the rpc_blockstats test...

I also don't get it. What exactly changed to the regtest genesis block?

Can't we skip CommitToArguments if the genesis-style is bitcoin?

The test doesn't specify genesis style of bitcoin(though it maybe should).

instagibbs · 2018-12-18T18:18:07Z

needs rebase

instagibbs · 2018-12-18T18:18:18Z

and please add a description :)

- Add CommitToArgument without fedpegScript or signblockscript - Modify CreateGenesisBlock to take in the genesis scriptSig - Add AppendInitialIssuace without assets support - AppendInitialIssuance to genesis block for Custom chain only - Add -initialfreecoins option - Modify tests to work with initialfreecoins

- also regenerate test data for getblockstats

dongcarl · 2018-12-18T19:54:09Z

@instagibbs Description added.

instagibbs · 2018-12-18T21:00:36Z

utACK 95432aa

95432aa Test listunspent sanity when specifying initialfreecoins (Carl Dong) 1ac14fc restore bitcoin regtest gen block using -genesis_style (Gregory Sanders) 8b8c11a Add initialfreecoins option, default of 0 (Carl Dong) Pull request description: This PR adds a `-initialfreecoins` debug option, and a `-con_genesis_style` to go along with it. This allows us to correctly set the amount of coins created in the genesis block, either in the `elements` style or `bitcoin` style. Tests have been modified to test that the correct amount of coins are created. Tree-SHA512: d3c56e7109e8e6496b6e5b819dc09f3ebf82013882854cc76062a39e4399e7eeb5789b567be527133278c1646a2765d02a09bd2d9618bdaeec5a852042e40c2b

stevenroose · 2018-12-18T21:45:17Z

post-merge ACK 95432aa :) was trying something out regarding the bitcoin_functional tests

dongcarl changed the title ~~WIP: Add initialfreecoins option, default of 0~~ [WIP, 0.17]: Add initialfreecoins option, default of 0 Nov 15, 2018

dongcarl changed the title ~~[WIP, 0.17]: Add initialfreecoins option, default of 0~~ [WIP, 0.17] Add initialfreecoins option, default of 0 Nov 15, 2018

stevenroose mentioned this pull request Nov 15, 2018

Update elements to Bitcoin Core version 0.17.0 #398

Closed

30 tasks

instagibbs reviewed Dec 7, 2018

View reviewed changes

dongcarl force-pushed the 2018-11-initialfreecoins branch 2 times, most recently from 7a5916a to d526b73 Compare December 7, 2018 18:54

instagibbs changed the title ~~[WIP, 0.17] Add initialfreecoins option, default of 0~~ [0.17] Add initialfreecoins option, default of 0 Dec 14, 2018

instagibbs reviewed Dec 14, 2018

View reviewed changes

dongcarl force-pushed the 2018-11-initialfreecoins branch from 6f4ca6e to 9b0632b Compare December 18, 2018 02:13

stevenroose reviewed Dec 18, 2018

View reviewed changes

dongcarl force-pushed the 2018-11-initialfreecoins branch from 9b0632b to 31d82a5 Compare December 18, 2018 18:03

dongcarl and others added 3 commits December 18, 2018 10:21

restore bitcoin regtest gen block using -genesis_style

1ac14fc

- also regenerate test data for getblockstats

Test listunspent sanity when specifying initialfreecoins

95432aa

dongcarl force-pushed the 2018-11-initialfreecoins branch from 31d82a5 to 95432aa Compare December 18, 2018 18:28

instagibbs merged commit 95432aa into ElementsProject:elements-0.17 Dec 18, 2018

instagibbs mentioned this pull request Dec 18, 2018

[0.17] Chainparams: Introduce consensus.genesis_style for compatibility #436

Closed

[0.17] Add initialfreecoins option, default of 0 #467

[0.17] Add initialfreecoins option, default of 0 #467

Conversation

dongcarl commented Nov 15, 2018 • edited Loading

stevenroose commented Nov 15, 2018

instagibbs commented Dec 4, 2018

dongcarl commented Dec 4, 2018 • edited Loading

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

dongcarl commented Dec 11, 2018

instagibbs commented Dec 11, 2018

instagibbs commented Dec 11, 2018

instagibbs commented Dec 11, 2018 • edited Loading

stevenroose commented Dec 11, 2018

instagibbs commented Dec 11, 2018

instagibbs commented Dec 14, 2018

instagibbs commented Dec 14, 2018 • edited Loading

instagibbs Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Dec 14, 2018

dongcarl commented Dec 18, 2018

instagibbs commented Dec 18, 2018

stevenroose 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

instagibbs Dec 18, 2018 • edited Loading

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

instagibbs commented Dec 18, 2018

instagibbs commented Dec 18, 2018

dongcarl commented Dec 18, 2018

instagibbs commented Dec 18, 2018

stevenroose commented Dec 18, 2018

dongcarl commented Nov 15, 2018 •

edited

Loading

dongcarl commented Dec 4, 2018 •

edited

Loading

instagibbs commented Dec 11, 2018 •

edited

Loading

instagibbs commented Dec 14, 2018 •

edited

Loading

instagibbs Dec 14, 2018 •

edited

Loading

instagibbs Dec 18, 2018 •

edited

Loading