Always add default_witness_commitment with GBT client support

sipa commented at 8:23 pm on November 18, 2016: member

As a consistency check, it is useful to see whether GBT clients are ready for segwit deployment before segwit is actually used on the network (and even before it activates).

The earlier behaviour only inserted a default_witness_commitment field in the GBT response whenever actual witness transactions were present. This PR changes it to do whenever 1) the GBT client claims to support segwit and 2) a segwit softfork is defined (which is effectively always in the current codebase, but may not be true for backports).

sipa force-pushed on Nov 18, 2016

theuni commented at 9:00 pm on November 18, 2016: member

Concept ACK. Will test this on some live pools using #9000.

in src/test/miner_tests.cpp: in 26af85c9c5 outdated

213@@ -214,6 +214,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
214         txCoinbase.vin[0].scriptSig = CScript();
215         txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce);
216         txCoinbase.vin[0].scriptSig.push_back(chainActive.Height());
217+        txCoinbase.vout.resize(1);

sdaftuar commented at 9:28 pm on November 18, 2016:

I’d forgotten how this test worked and it took me a little while to track down why this is needed. In case other reviewers are wondering: the blocks generated here won’t have valid proof-of-work if we change the contents, because of the hardcoded nonces up above.

sdaftuar commented at 9:29 pm on November 18, 2016: member

ACK. Tested with some hacks to p2p-segwit.py to verify that the default_witness_commitment is correct (afaik there was no explicit test for this before(?)), and that it is only produced if the segwit softfork is defined and the client specifies the segwit rule in the gbt call.

sipa commented at 9:54 pm on November 18, 2016: member

@sdaftuar Awesome, can you publish those tests?

gmaxwell commented at 10:42 am on November 19, 2016: contributor

Concept ACK, looking forward to hearing back poolserver test results. It would be good to try a mutation test with that harness Suhas mentioned.

gmaxwell commented at 10:42 am on November 19, 2016: contributor

This should be tagged for backport.

MarcoFalke added the label Needs backport on Nov 19, 2016

MarcoFalke added this to the milestone 0.13.2 on Nov 19, 2016

sdaftuar commented at 11:59 am on November 19, 2016: member

@sipa I tried to clean up my hacks but this is still on the quick&dirty side of things, but maybe useful for others to continue testing: https://github.com/sdaftuar/bitcoin/commit/cdafaad41fca79fd62bb944ca201c25e34fb2983

gmaxwell commented at 6:31 pm on November 21, 2016: contributor

@sipa needs rebase

Always add default_witness_commitment with GBT client support ad04d1cb35

[qa] Test getblocktemplate default_witness_commitment 95f4a03777

sipa force-pushed on Nov 21, 2016

sipa commented at 11:07 pm on November 21, 2016: member

Rebased.

theuni commented at 0:03 am on November 22, 2016: member

Tested ACK, works as intended.

Unfortunately though, several pool implementations only check for default_witness_commitment if they receive the segwit rule. I’ll work on upstreaming patches for insertion any time default_witness_commitment is present.

gmaxwell commented at 0:08 am on November 22, 2016: contributor

ACK.

laanwj merged this on Nov 25, 2016

laanwj closed this on Nov 25, 2016

laanwj referenced this in commit d932159f34 on Nov 25, 2016

laanwj referenced this in commit 96a7651d9a on Dec 2, 2016

laanwj referenced this in commit 460c9abe47 on Dec 2, 2016

laanwj commented at 7:23 am on December 2, 2016: member

Backported in #9264, removing tag.

laanwj removed the label Needs backport on Dec 2, 2016

laanwj referenced this in commit 99477c71c4 on Dec 2, 2016

laanwj referenced this in commit b96a8f7df2 on Dec 2, 2016

laanwj added the label P2P on Dec 19, 2016

MarcoFalke locked this on Sep 8, 2021

Always add default_witness_commitment with GBT client support #9189