Don't require segwit in getblocktemplate for segwit signalling or mining

sdaftuar commented at 10:00 pm on March 8, 2017: member

Segwit’s version bit will be signalled for all invocations of CreateNewBlock, and not specifying segwit only will cause CreateNewBlock to skip transactions with witness from being selected. (Note that CreateNewBlock’s performance may suffer if the mempool has witness transactions that are being skipped, so this is not a recommended configuration.)

Originally we didn’t signal segwit unless the getblocktemplate caller explicitly indicated support, in order to prevent segwit from activating before any miners actually had support for the witness commitment. Now that many blocks are signalling segwit already, this is not a material concern.

Also, I need to figure out how to add a test to segwit.py to check that witness transactions would in fact be skipped.

As per IRC discussion, I think we should consider this for backport to the 0.14 branch.

gmaxwell commented at 10:04 pm on March 8, 2017: contributor

Concept ACK.

in qa/rpc-tests/segwit.py: in 61d1a9ea29 outdated

254+        print("Non-segwit miners are able to use GBT response after activation.")
255+        txid = send_to_witness(1, self.nodes[0], find_unspent(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.998"))
256+        #assert_raises_jsonrpc(-8, "Support for 'segwit' rule requires explicit client support", self.nodes[0].getblocktemplate, {})
257+        tmpl = self.nodes[0].getblocktemplate()
258+        # TODO: add a transaction with witness to mempool, and verify it's not
259+        # selected for mining.

luke-jr commented at 10:14 pm on March 8, 2017:

Should test that witness tx can’t sneak in as an ancestor of non-witness, too.

sdaftuar commented at 1:48 am on March 9, 2017:

I concur.

sdaftuar commented at 6:53 pm on March 9, 2017:

Done in 7db565c

in src/rpc/mining.cpp: in 61d1a9ea29 outdated

688@@ -683,7 +689,6 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
689     result.push_back(Pair("bits", strprintf("%08x", pblock->nBits)));
690     result.push_back(Pair("height", (int64_t)(pindexPrev->nHeight+1)));
691 
692-    const struct BIP9DeploymentInfo& segwit_info = VersionBitsDeploymentInfo[Consensus::DEPLOYMENT_SEGWIT];
693     if (!pblocktemplate->vchCoinbaseCommitment.empty() && setClientRules.find(segwit_info.name) != setClientRules.end()) {

luke-jr commented at 10:16 pm on March 8, 2017:

Use fMineWitnessTransactions here?

sdaftuar commented at 1:48 am on March 9, 2017:

Addressed in 24428bd.

in src/rpc/mining.cpp: in 61d1a9ea29 outdated

538@@ -533,7 +539,7 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
539 
540         // Create new block
541         CScript scriptDummy = CScript() << OP_TRUE;
542-        pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy);
543+        pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy, fMineWitnessTransactions);

luke-jr commented at 10:17 pm on March 8, 2017:

This is cached across calls, so it requires a bit more complexity.

sdaftuar commented at 1:48 am on March 9, 2017:

I think I fixed this in 24428bd.

luke-jr changes_requested

JeremyRubin commented at 10:20 pm on March 8, 2017: contributor

Maybe transactions with a witness should not get inserted into the mempool?

sdaftuar commented at 1:59 am on March 9, 2017: member

Maybe transactions with a witness should not get inserted into the mempool? @JeremyRubin Sure, and miners could also continue to use 0.13.0 or older for getblocktemplate (while submitting their blocks to later version software, which would work just fine post-segwit activation). Many working configurations are possible; I think we’re just trying to add a simple way to support more miners out of the box. Personally I don’t think it’s worth making big code changes – like redoing mempool policy on the fly for miners who aren’t invoking gbt with segwit support – to achieve this functionality, as this isn’t a recommended setup, just a tolerable one.

(If we wanted to make CNB fast in this configuration, then I think the easiest thing to do would be to say add an int to each mempool entry that counts the number of ancestors that have witnesses, and use that skip over transactions faster in CNB, but this doesn’t seem worth it to me.)

luke-jr approved

luke-jr commented at 2:14 am on March 9, 2017: member

utACK (might be better to have two caches, but I doubt it matters)

jonasschnelli added the label Mining on Mar 9, 2017

laanwj commented at 9:03 am on March 9, 2017: member

Concept ACK

sdaftuar commented at 6:57 pm on March 9, 2017: member

Added a test to segwit.py

laanwj added this to the milestone 0.14.1 on Mar 9, 2017

laanwj added the label Needs backport on Mar 9, 2017

sdaftuar force-pushed on Mar 10, 2017

sdaftuar commented at 1:30 pm on March 10, 2017: member

Rebased (conflict was in segwit.py) and squashed the fixup commit.

TheBlueMatt commented at 4:59 pm on March 10, 2017: member

utACK a4a20d8a8de77b45e33cbdb8018dc30c9176fccd

gmaxwell commented at 6:02 am on March 12, 2017: contributor

ACK.

Had to go refresh myself on the proper meaning of the “!’ prefix. Perhaps the documentation (e.g. BIPs) should be updated to give that a name, “Mandatory prefix”. BIP 145 also needs a clear and direct call out to the extensions established in BIP9, not after it’s already using them. Otherwise it’s easily confusing. :)

laanwj commented at 10:31 am on March 14, 2017: member

Merge conflict in segwit.py after #9970

Don't require segwit in getblocktemplate for segwit signalling or mining

Segwit's version bit will be signalled for all invocations of CreateNewBlock,
and not specifying segwit only will cause CreateNewBlock to skip transactions
with witness from being selected.

abe7b3d3ab

Test transaction selection when gbt called without segwit support c85ffe6d8d

sdaftuar force-pushed on Mar 14, 2017

sdaftuar commented at 10:53 am on March 14, 2017: member

Rebased.

laanwj merged this on Mar 14, 2017

laanwj closed this on Mar 14, 2017

laanwj referenced this in commit 416809c11b on Mar 14, 2017

sdaftuar referenced this in commit 569596cc51 on Mar 16, 2017

sdaftuar referenced this in commit 2cd2cd51f7 on Mar 16, 2017

MarcoFalke removed the label Needs backport on Mar 27, 2017

DrahtBot locked this on Sep 8, 2021

Don’t require segwit in getblocktemplate for segwit signalling or mining #9955