Add commandline flag to set genesis block script destination #55

apoelstra · 2015-09-30T18:10:45Z

Fixes #54

jtimon · 2015-09-30T20:16:24Z

Why not call ScriptDestinationFromCommandLine etc directly in CRegTestParams instead of in SelectParamsFromCommandLine() ?
Or maybe CRegTestParams's constructor could take a CScript parameter that defaults to OP_TRUE.
Certainly ChangeGenesisScriptDestination doesn't need to be a public method, all this can be a just a static function in chainparams.cpp
In fact, this PR is buggy since ChangeGenesisScriptDestination doesn't update hashGenesisBlock nor mapCheckpointsTestnet[0].

apoelstra · 2015-09-30T22:08:27Z

@jtimon For "why did I structure the functions this way" I was trying to copy as best I could what is done for the -testnet/-regtest parameters. As for giving a parameter to the CRegTestParams that's tricky because the regtest paramaters are a static variable, and IIRC in C++ we have no control over what order constructors for static variables are run in.

As for the bugs, you're right, thanks.

jtimon · 2015-09-30T22:32:05Z

re CRegTestParams::CRegTestParams() parameter: I was assuming some of bitcoin/bitcoin#6382 had already been merged, but some of those changes were rejected for alpha so never mind.

Anyway, the solution is still simple: just put all the new code inside CRegTestParams::CRegTestParams(), before the hash of the genesis block is calculated.

See bitcoin/bitcoin#6382 which also builds the genesis block dynamically from a command line parameter for the sizetest std::numeric_limits<uint64_t>::max() different chains.

apoelstra · 2015-10-01T20:17:42Z

src/chainparams.cpp

@@ -249,7 +249,7 @@ class CRegTestParams : public CTestNetParams {
        txNew.vout[1].scriptPubKey = CScript() << OP_TRUE;
        genesis.vtx[0] = CTransaction(txNew);

-        CScript scriptDestination(CScript() << OP_TRUE);
+        CScript scriptDestination(CScript() << OP_2 << ParseHex("023303dedc51b9d227b17c9fb4710f96b844e1ccdc2c776e1b7274bd4e246b6202") << ParseHex("03024c3b4f830854d6d26d6e34d92aff4c703bf57e85cd42abe328d928e01d4286") << ParseHex("03fe4e8c8d99b9dcbb529a87d54f606bae0149a34018325547fa0c2239e038a1c9") << OP_3 << OP_CHECKMULTISIG);


Oops, this should not have made it into the PR.

apoelstra · 2015-10-01T20:24:27Z

Thanks for bitcoin/bitcoin#6382 @jtimon, there are a lot of good ideas in there. I think I'll copy your factory pattern.

jtimon · 2015-10-02T06:58:07Z

Awesome! Maybe you want to utACK one of the following open little steps in bitcoin/bitcoin?

Chainparams: Translations: DRY: options and error strings #6235
Don't check the genesis block #6597

apoelstra · 2015-10-02T17:02:42Z

Rebased using a factory pattern as in @jtimon's code. Need to add some unit tests before this is usable.

Like bitcoin/6382, there is an argument that this is only useful for QA and may not make sense to be merged. But in this case, I think it's reasonable to expect users to play with their own network with their own functionaries, and this makes this much easier.

apoelstra · 2015-10-02T23:03:02Z

Added unit tests that confirms the genesis hash is computed correctly (for testnet and regtest, that the output of RPC getblockhash 0 matches the result of directly passes CScripts into SetParams; for mainnet and unittest I assumed the result was correct and added a check for it to catch regressions).

apoelstra · 2015-11-20T16:35:38Z

@jtimon Do you have further thoughts on this PR? I know you have overlapping PRs in Core.

@gmaxwell You commented on bitcoin/bitcoin#6907 that you feel making this a factory is a bad idea. Keeping them static would mean moving the constructor into some sort of init function, have the constructor just call this, and have SelectParams call that rather than calling the factory. Would you prefer this?

In any case I'd like to move forward with this PR.

jtimon · 2015-11-21T11:24:10Z

src/chainparams.h

@@ -80,6 +80,14 @@ class CChainParams
    const std::vector<unsigned char>& Base58Prefix(Base58Type type) const { return base58Prefixes[type]; }
    const std::vector<CAddress>& FixedSeeds() const { return vFixedSeeds; }
    virtual const Checkpoints::CCheckpointData& Checkpoints() const = 0;
+    /** Change the script in the genesis hash */
+    void ChangeGenesisScriptDestination(CScript s) { genesis.proof = CProof(s, CScript()); }


Unused method.

jtimon · 2015-11-21T11:45:53Z

If I understood it correctly, @gmaxwell 's nit was that I was making globalChainParams and globalSwitchingChainParams extern instead of static and it's already solved (but it would be nice if he can clarify whether his nit has been solved or not in bitcoin/bitcoin#6907 ).

As said, I don't like the script being passed to SeelectParams(), the factory, nor the constructors and I would prefer ScriptDestinationFromCommandLine to be called directly within the constructors, but just a preference.
I added a couple of minor nits though.

The overlapping PR/commits in Core are contained in https://github.com/jtimon/bitcoin/tree/alpha-base-0.12 which I will maintain rebased until 0.12 is forked and is what I would l would like @TheBlueMatt to use as a base when rebasing.

apoelstra · 2015-11-22T00:44:59Z

Addressed @jtimon's nits

jtimon · 2015-11-22T10:15:06Z

I'm going to merge.

Add commandline flag to set genesis block script destination

jtimon · 2015-11-22T10:22:05Z

src/chainparams.h

@@ -139,13 +145,15 @@ const CChainParams &Params();
 /** Return parameters for the given network. */
 CChainParams &Params(CBaseChainParams::Network network);


Wait a minute...where is this function defined now?

chainparams.cpp:315

No, I mean the parametrized version.

Oh, hmm, this is not defined anywhere anymore (nor is it called anywhere, of course, else we would have compilation errors).

mhmm I would like to eventually remove it from core too.I guess when rebasing the remaining uses can be replaced with the factory i elements. It's just something I expected to happen in bitcoin core first.

Of course another option is to maintain it in elements until it removed in core.

getrawtransaction: scriptpubkey type is 'fee'

apoelstra reviewed Oct 1, 2015
View reviewed changes

apoelstra force-pushed the scriptdest branch from 8e0ba86 to fea1ade Compare October 2, 2015 17:01

Add unit test for chain genesis block initialization

2843d41

apoelstra force-pushed the scriptdest branch from fea1ade to 4439753 Compare October 2, 2015 23:01

apoelstra force-pushed the scriptdest branch from 4439753 to 86876f1 Compare October 14, 2015 15:54

jtimon reviewed Nov 21, 2015
View reviewed changes

apoelstra added 3 commits November 21, 2015 18:36

Add commandline flag to set genesis block script destination

da45c44

Add unit test for custom blocksign script

081c45b

constify some functions in main.cpp

195a14e

apoelstra force-pushed the scriptdest branch from 86876f1 to 195a14e Compare November 22, 2015 00:44

jtimon added a commit that referenced this pull request Nov 22, 2015

Merge pull request #55 from apoelstra/scriptdest

2f7f3bf

Add commandline flag to set genesis block script destination

jtimon merged commit 2f7f3bf into ElementsProject:alpha Nov 22, 2015

jtimon reviewed Nov 22, 2015
View reviewed changes

apoelstra added a commit to apoelstra/elements that referenced this pull request Nov 23, 2015

Fix GUI build (broken by PR ElementsProject#55)

09aa2eb

apoelstra mentioned this pull request Nov 23, 2015

Fix GUI build (broken by PR #55) #64

Closed

instagibbs added a commit to instagibbs/elements that referenced this pull request Mar 15, 2017

Merge pull request ElementsProject#55 from instagibbs/feenon

eea3b94

getrawtransaction: scriptpubkey type is 'fee'

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commandline flag to set genesis block script destination #55

Add commandline flag to set genesis block script destination #55

apoelstra commented Sep 30, 2015

jtimon commented Sep 30, 2015

apoelstra commented Sep 30, 2015

jtimon commented Sep 30, 2015

apoelstra Oct 1, 2015

apoelstra commented Oct 1, 2015

jtimon commented Oct 2, 2015

apoelstra commented Oct 2, 2015

apoelstra commented Oct 2, 2015

apoelstra commented Nov 20, 2015

jtimon Nov 21, 2015

apoelstra Nov 22, 2015

jtimon commented Nov 21, 2015

apoelstra commented Nov 22, 2015

jtimon commented Nov 22, 2015

jtimon Nov 22, 2015

apoelstra Nov 22, 2015

jtimon Nov 22, 2015

apoelstra Nov 22, 2015

jtimon Nov 22, 2015

jtimon Nov 22, 2015

		@@ -139,13 +145,15 @@ const CChainParams &Params();
		/** Return parameters for the given network. */
		CChainParams &Params(CBaseChainParams::Network network);

Add commandline flag to set genesis block script destination #55

Add commandline flag to set genesis block script destination #55

Conversation

apoelstra commented Sep 30, 2015

jtimon commented Sep 30, 2015

apoelstra commented Sep 30, 2015

jtimon commented Sep 30, 2015

Choose a reason for hiding this comment

apoelstra commented Oct 1, 2015

jtimon commented Oct 2, 2015

apoelstra commented Oct 2, 2015

apoelstra commented Oct 2, 2015

apoelstra commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Nov 21, 2015

apoelstra commented Nov 22, 2015

jtimon commented Nov 22, 2015

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