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

sdaftuar · 2017-03-08T22:00:49Z

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

Concept ACK.

luke-jr · 2017-03-08T22:14:13Z

qa/rpc-tests/segwit.py

+        #assert_raises_jsonrpc(-8, "Support for 'segwit' rule requires explicit client support", self.nodes[0].getblocktemplate, {})
+        tmpl = self.nodes[0].getblocktemplate()
+        # TODO: add a transaction with witness to mempool, and verify it's not
+        # selected for mining.


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

Done in 7db565c

luke-jr · 2017-03-08T22:16:09Z

src/rpc/mining.cpp

@@ -683,7 +689,6 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
    result.push_back(Pair("bits", strprintf("%08x", pblock->nBits)));
    result.push_back(Pair("height", (int64_t)(pindexPrev->nHeight+1)));

-    const struct BIP9DeploymentInfo& segwit_info = VersionBitsDeploymentInfo[Consensus::DEPLOYMENT_SEGWIT];
    if (!pblocktemplate->vchCoinbaseCommitment.empty() && setClientRules.find(segwit_info.name) != setClientRules.end()) {


Use fMineWitnessTransactions here?

Addressed in 24428bd.

luke-jr · 2017-03-08T22:17:28Z

src/rpc/mining.cpp

@@ -533,7 +539,7 @@ UniValue getblocktemplate(const JSONRPCRequest& request)

        // Create new block
        CScript scriptDummy = CScript() << OP_TRUE;
-        pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy);
+        pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy, fMineWitnessTransactions);


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

I think I fixed this in 24428bd.

JeremyRubin · 2017-03-08T22:20:25Z

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

sdaftuar · 2017-03-09T01:59:33Z

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

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

laanwj · 2017-03-09T09:03:54Z

Concept ACK

sdaftuar · 2017-03-09T18:57:39Z

Added a test to segwit.py

sdaftuar · 2017-03-10T13:30:44Z

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

TheBlueMatt · 2017-03-10T16:59:10Z

utACK a4a20d8a8de77b45e33cbdb8018dc30c9176fccd

gmaxwell · 2017-03-12T06:02:40Z

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 · 2017-03-14T10:31:10Z

Merge conflict in segwit.py after #9970

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.

sdaftuar · 2017-03-14T10:53:07Z

Rebased.

…alling or mining c85ffe6 Test transaction selection when gbt called without segwit support (Suhas Daftuar) abe7b3d Don't require segwit in getblocktemplate for segwit signalling or mining (Suhas Daftuar) Tree-SHA512: 172496b6d7cdf1879de1266748f2b4ed9fd2ba9ff4a1fd964d74d73c674c16d74bf01a3ba42bf25f2d69f348217c0bbf3412ac64821f222efc9de25a287a5240

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. Github-Pull: bitcoin#9955 Rebased-From: abe7b3d

Github-Pull: bitcoin#9955 Rebased-From: c85ffe6

gmaxwell reviewed Mar 8, 2017

View reviewed changes

luke-jr suggested changes Mar 8, 2017

View reviewed changes

luke-jr approved these changes Mar 9, 2017

View reviewed changes

jonasschnelli added the Mining label Mar 9, 2017

laanwj added this to the 0.14.1 milestone Mar 9, 2017

laanwj added the Needs backport label Mar 9, 2017

sdaftuar force-pushed the 2017-03-mining-segwit-changes branch from 7db565c to a4a20d8 Compare March 10, 2017 13:30

sdaftuar added 2 commits March 14, 2017 06:43

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

abe7b3d

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.

Test transaction selection when gbt called without segwit support

c85ffe6

sdaftuar force-pushed the 2017-03-mining-segwit-changes branch from a4a20d8 to c85ffe6 Compare March 14, 2017 10:52

laanwj merged commit c85ffe6 into bitcoin:master Mar 14, 2017

sdaftuar mentioned this pull request Mar 16, 2017

[0.14 backport] Don't require segwit in getblocktemplate for segwit signalling or mining #10006

Merged

sdaftuar added a commit to sdaftuar/bitcoin that referenced this pull request Mar 16, 2017

Test transaction selection when gbt called without segwit support

2cd2cd5

Github-Pull: bitcoin#9955 Rebased-From: c85ffe6

maflcko removed the Needs backport label Mar 27, 2017

ajtowns mentioned this pull request Jul 31, 2019

Bury bip9 deployments #16060

Merged

luke-jr mentioned this pull request Jan 17, 2020

Fix GBT: Restore "!segwit" and "csv" to "rules" key #17946

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

sdaftuar commented Mar 8, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

luke-jr Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

luke-jr Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Mar 8, 2017

Uh oh!

sdaftuar commented Mar 9, 2017

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 9, 2017

Uh oh!

sdaftuar commented Mar 9, 2017

Uh oh!

sdaftuar commented Mar 10, 2017

Uh oh!

TheBlueMatt commented Mar 10, 2017

Uh oh!

gmaxwell commented Mar 12, 2017

Uh oh!

laanwj commented Mar 14, 2017

Uh oh!

sdaftuar commented Mar 14, 2017

Uh oh!

Uh oh!