Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[0.17] Mandatory coinbase feature #430

Merged
merged 2 commits into from
Oct 22, 2018
Merged

[0.17] Mandatory coinbase feature #430

merged 2 commits into from
Oct 22, 2018

Conversation

instagibbs
Copy link
Collaborator

@instagibbs instagibbs commented Oct 12, 2018 •
edited

Includes functional test

@instagibbs instagibbs mentioned this pull request Oct 12, 2018
Closed
30 tasks
@instagibbs instagibbs force-pushed the mandatory_coinbase branch 2 times, most recently from 9897b5e to 5d77f54 Compare October 15, 2018 18:40
@instagibbs instagibbs changed the title [WIP, 0.17] Mandatory coinbase feature [0.17] Mandatory coinbase feature Oct 15, 2018
@instagibbs instagibbs force-pushed the mandatory_coinbase branch 2 times, most recently from a5536b4 to 22396a7 Compare October 15, 2018 19:49
instagibbs
instagibbs commented Oct 15, 2018
View reviewed changes
src/init.cpp Outdated
@@ -514,6 +514,7 @@ void SetupServerArgs()
gArgs.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", false, OptionsCategory::RPC);
gArgs.AddArg("-rpcworkqueue=<n>", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), true, OptionsCategory::RPC);
gArgs.AddArg("-server", "Accept command line and JSON-RPC commands", false, OptionsCategory::RPC);
gArgs.AddArg("-con_mandatorycoinbase", "All non-zero valued coinbase outputs must go to this scriptPubKey, if set.", false, OptionsCategory::RPC);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be OptionsCategory::CHAINPARAMS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and it should be in chainparamsbase.cpp

jtimon
jtimon reviewed Oct 16, 2018
View reviewed changes
src/chainparams.cpp Outdated
@@ -322,6 +334,9 @@ class CRegTestParams : public CChainParams {
assert(consensus.hashGenesisBlock == uint256S("0x0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206"));
assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));

// All non-zero coinbase outputs must go to this scriptPubKey
consensus.mandatory_coinbase_destination = StrHexToScriptWithDefault(gArgs.GetArg("-con_mandatorycoinbase", ""), CScript()); // Blank script allows any coinbase destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just

Suggested change
consensus.mandatory_coinbase_destination = StrHexToScriptWithDefault(gArgs.GetArg("-con_mandatorycoinbase", ""), CScript()); // Blank script allows any coinbase destination
// Blank script allows any coinbase destination
consensus.mandatory_coinbase_destination = ParseHex(args.GetArg("-con_mandatorycoinbase", ""));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it implicitly convert it? It's a script, not a series of bytes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be working on f0391db#diff-64cbe1ad5465e13bc59ee8bb6f3de2e7R431

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ok in that case we don't need StrHexToScriptWithDefault at all, though I'll make the conversion explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right C++: It can implicitly use constructor.

jtimon
jtimon reviewed Oct 16, 2018
View reviewed changes
src/consensus/params.h
@@ -11,6 +11,8 @@
#include <map>
#include <string>

#include <script/script.h> // mandatory_coinbase_destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this dependency giving you problems? I would swear I tried the same with #433

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully not, we do the same thing for elements-0.14.1 IIRC!

@jtimon jtimon added the 0.17 label Oct 16, 2018
@stevenroose
Copy link
Member

Hmm, the test doesn't seem to be working here: CreateNewBlock: TestBlockValidity failed: bad-coinbase-txos (code 16) (-1)

./test/functional/mandatory_coinbase.py        
2018-10-18T13:48:41.565000Z TestFramework (INFO): Initializing test directory /tmp/test83wa3eq6
2018-10-18T13:48:41.967000Z TestFramework (INFO): generatetoaddress: Making blocks of various kinds, checking for rejection
2018-10-18T13:48:41.970000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/steven/blockstream/elements/test/functional/test_framework/test_framework.py", line 161, in main
    self.run_test()
  File "./test/functional/mandatory_coinbase.py", line 44, in run_test
    node0.generatetoaddress(101, mandatory_address)
  File "/home/steven/blockstream/elements/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/steven/blockstream/elements/test/functional/test_framework/authproxy.py", line 138, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: CreateNewBlock: TestBlockValidity failed: bad-coinbase-txos (code 16) (-1)
2018-10-18T13:48:42.025000Z TestFramework (INFO): Stopping nodes
2018-10-18T13:48:42.233000Z TestFramework (WARNING): Not cleaning up dir /tmp/test83wa3eq6
2018-10-18T13:48:42.233000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test83wa3eq6/test_framework.log
2018-10-18T13:48:42.234000Z TestFramework (ERROR): Hint: Call /home/steven/blockstream/elements/test/functional/combine_logs.py '/tmp/test83wa3eq6' to consolidate all logs

@instagibbs
Copy link
Collaborator Author

hmm it appears my refactoring broke something.

@instagibbs
Copy link
Collaborator Author

oh, I only applied the argument to regtest, tip is using "regtest2" custom chain. We should be removing regtest at some point, right @jtimon ? I'll move it to custom chain params now.

@instagibbs
Copy link
Collaborator Author

Turns out me being explicit with construction means I was doing it the wrong way, the implicit way automagically was correct.

@stevenroose
Copy link
Member

Turns out me being explicit with construction means I was doing it the wrong way, the implicit way automagically was correct.

tACK 0d20b55

jtimon
jtimon reviewed Oct 19, 2018
View reviewed changes
src/validation.cpp
@@ -1850,6 +1850,17 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl

nBlocksTotal++;

// Check that all non-zero coinbase outputs pay to the required destination
const CScript& mandatory_coinbase_destination = chainparams.GetConsensus().mandatory_coinbase_destination;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this local variable worth it? small nit, feel free to ignore.

@jtimon
Copy link
Contributor

jtimon commented Oct 19, 2018

We can remove regtest, testnet and main when we can no longer support them for real, I guess.

Didn't look with much attention to the test, but the code looks good. utACK 0d20b55

@instagibbs instagibbs merged commit 0d20b55 into ElementsProject:elements-0.17 Oct 22, 2018
instagibbs added a commit that referenced this pull request Oct 22, 2018
@instagibbs
Merge #430: [0.17] Mandatory coinbase feature
101ec23 
0d20b55 add mandatory coinbase functional test (Gregory Sanders)
edb0532 add consensus.mandatory_coinbase_destination (Gregory Sanders)

Pull request description:

  Includes functional test

Tree-SHA512: e7fdf4584c5a61516778dff272c0f4c9964fee515b705c4fd3ade842954bcc7426bdaf5273a64e4b2472fedc940ead7552c0ec80b4608aac381f91a8f2356d95
@instagibbs
Copy link
Collaborator Author

sigh, I didn't add the test to the test list... will fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtimon jtimon jtimon left review comments

Assignees
No one assigned
Labels
0.17
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@instagibbs @stevenroose @jtimon