From 569596cc5148ef868350a9720013d38faf3e34ce Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 8 Mar 2017 15:56:59 -0500 Subject: [PATCH 1/2] 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. Github-Pull: #9955 Rebased-From: abe7b3d3abe10e3554b770f40824174b3b217490 --- qa/rpc-tests/p2p-segwit.py | 8 +++++--- qa/rpc-tests/segwit.py | 19 +++++-------------- src/miner.cpp | 4 ++-- src/miner.h | 2 +- src/rpc/mining.cpp | 18 ++++++++++++++---- src/versionbits.cpp | 2 +- 6 files changed, 28 insertions(+), 25 deletions(-) diff --git a/qa/rpc-tests/p2p-segwit.py b/qa/rpc-tests/p2p-segwit.py index 2f339bb54..479f1c679 100755 --- a/qa/rpc-tests/p2p-segwit.py +++ b/qa/rpc-tests/p2p-segwit.py @@ -1701,9 +1701,11 @@ class SegWitTest(BitcoinTestFramework): for node in [self.nodes[0], self.nodes[2]]: gbt_results = node.getblocktemplate() block_version = gbt_results['version'] - # If we're not indicating segwit support, we should not be signalling - # for segwit activation, nor should we get a witness commitment. - assert_equal(block_version & (1 << VB_WITNESS_BIT), 0) + # If we're not indicating segwit support, we will still be + # signalling for segwit activation. + assert_equal((block_version & (1 << VB_WITNESS_BIT) != 0), node == self.nodes[0]) + # If we don't specify the segwit rule, then we won't get a default + # commitment. assert('default_witness_commitment' not in gbt_results) # Workaround: diff --git a/qa/rpc-tests/segwit.py b/qa/rpc-tests/segwit.py index d6831e00e..5dfc7a2f0 100755 --- a/qa/rpc-tests/segwit.py +++ b/qa/rpc-tests/segwit.py @@ -251,20 +251,11 @@ class SegWitTest(BitcoinTestFramework): assert(tmpl['transactions'][0]['txid'] == txid) assert(tmpl['transactions'][0]['sigops'] == 8) - print("Verify non-segwit miners get a valid GBT response after the fork") - send_to_witness(1, self.nodes[0], find_unspent(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.998")) - try: - tmpl = self.nodes[0].getblocktemplate({}) - assert(len(tmpl['transactions']) == 1) # Doesn't include witness tx - assert(tmpl['sizelimit'] == 1000000) - assert('weightlimit' not in tmpl) - assert(tmpl['sigoplimit'] == 20000) - assert(tmpl['transactions'][0]['hash'] == txid) - assert(tmpl['transactions'][0]['sigops'] == 2) - assert(('!segwit' in tmpl['rules']) or ('segwit' not in tmpl['rules'])) - except JSONRPCException: - # This is an acceptable outcome - pass + print("Non-segwit miners are able to use GBT response after activation.") + txid = send_to_witness(1, self.nodes[0], find_unspent(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.998")) + tmpl = self.nodes[0].getblocktemplate() + # TODO: add a transaction with witness to mempool, and verify it's not + # selected for mining. print("Verify behaviour of importaddress, addwitnessaddress and listunspent") diff --git a/src/miner.cpp b/src/miner.cpp index d01edd93b..7b3d94d0e 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -127,7 +127,7 @@ void BlockAssembler::resetBlock() blockFinished = false; } -std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn) +std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, bool fMineWitnessTx) { resetBlock(); @@ -165,7 +165,7 @@ std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& sc // -promiscuousmempoolflags is used. // TODO: replace this with a call to main to assess validity of a mempool // transaction (which in most cases can be a no-op). - fIncludeWitness = IsWitnessEnabled(pindexPrev, chainparams.GetConsensus()); + fIncludeWitness = IsWitnessEnabled(pindexPrev, chainparams.GetConsensus()) && fMineWitnessTx; addPriorityTxs(); addPackageTxs(); diff --git a/src/miner.h b/src/miner.h index 3ba92b16b..29013c3bc 100644 --- a/src/miner.h +++ b/src/miner.h @@ -165,7 +165,7 @@ private: public: BlockAssembler(const CChainParams& chainparams); /** Construct a new block template with coinbase to scriptPubKeyIn */ - std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn); + std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn, bool fMineWitnessTx=true); private: // utility functions diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 77cd282a3..38d7b1eb1 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -519,12 +519,22 @@ UniValue getblocktemplate(const JSONRPCRequest& request) // TODO: Maybe recheck connections/IBD and (if something wrong) send an expires-immediately template to stop miners? } + const struct BIP9DeploymentInfo& segwit_info = VersionBitsDeploymentInfo[Consensus::DEPLOYMENT_SEGWIT]; + // If the caller is indicating segwit support, then allow CreateNewBlock() + // to select witness transactions, after segwit activates (otherwise + // don't). + bool fSupportsSegwit = setClientRules.find(segwit_info.name) != setClientRules.end(); + // Update block static CBlockIndex* pindexPrev; static int64_t nStart; static std::unique_ptr pblocktemplate; + // Cache whether the last invocation was with segwit support, to avoid returning + // a segwit-block to a non-segwit caller. + static bool fLastTemplateSupportsSegwit = true; if (pindexPrev != chainActive.Tip() || - (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - nStart > 5)) + (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - nStart > 5) || + fLastTemplateSupportsSegwit != fSupportsSegwit) { // Clear pindexPrev so future calls make a new block, despite any failures from here on pindexPrev = nullptr; @@ -533,10 +543,11 @@ UniValue getblocktemplate(const JSONRPCRequest& request) nTransactionsUpdatedLast = mempool.GetTransactionsUpdated(); CBlockIndex* pindexPrevNew = chainActive.Tip(); nStart = GetTime(); + fLastTemplateSupportsSegwit = fSupportsSegwit; // Create new block CScript scriptDummy = CScript() << OP_TRUE; - pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy); + pblocktemplate = BlockAssembler(Params()).CreateNewBlock(scriptDummy, fSupportsSegwit); if (!pblocktemplate) throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory"); @@ -686,8 +697,7 @@ 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()) { + if (!pblocktemplate->vchCoinbaseCommitment.empty() && fSupportsSegwit) { result.push_back(Pair("default_witness_commitment", HexStr(pblocktemplate->vchCoinbaseCommitment.begin(), pblocktemplate->vchCoinbaseCommitment.end()))); } diff --git a/src/versionbits.cpp b/src/versionbits.cpp index d73f34051..8a7cce748 100644 --- a/src/versionbits.cpp +++ b/src/versionbits.cpp @@ -17,7 +17,7 @@ const struct BIP9DeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION }, { /*.name =*/ "segwit", - /*.gbt_force =*/ false, + /*.gbt_force =*/ true, } }; From 2cd2cd51f7ae954160d5422e105089ff1f598aa6 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 9 Mar 2017 13:49:50 -0500 Subject: [PATCH 2/2] Test transaction selection when gbt called without segwit support Github-Pull: #9955 Rebased-From: c85ffe6d8d57132c1825c16a572d3847419030a6 --- qa/rpc-tests/segwit.py | 55 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/qa/rpc-tests/segwit.py b/qa/rpc-tests/segwit.py index 5dfc7a2f0..c814399f9 100755 --- a/qa/rpc-tests/segwit.py +++ b/qa/rpc-tests/segwit.py @@ -11,9 +11,9 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import * from test_framework.mininode import sha256, ripemd160, CTransaction, CTxIn, COutPoint, CTxOut from test_framework.address import script_to_p2sh, key_to_p2pkh -from test_framework.script import CScript, OP_HASH160, OP_CHECKSIG, OP_0, hash160, OP_EQUAL, OP_DUP, OP_EQUALVERIFY, OP_1, OP_2, OP_CHECKMULTISIG +from test_framework.script import CScript, OP_HASH160, OP_CHECKSIG, OP_0, hash160, OP_EQUAL, OP_DUP, OP_EQUALVERIFY, OP_1, OP_2, OP_CHECKMULTISIG, OP_TRUE from io import BytesIO -from test_framework.mininode import FromHex +from test_framework.mininode import ToHex, FromHex, COIN NODE_0 = 0 NODE_1 = 1 @@ -251,11 +251,54 @@ class SegWitTest(BitcoinTestFramework): assert(tmpl['transactions'][0]['txid'] == txid) assert(tmpl['transactions'][0]['sigops'] == 8) + self.nodes[0].generate(1) # Mine a block to clear the gbt cache + print("Non-segwit miners are able to use GBT response after activation.") - txid = send_to_witness(1, self.nodes[0], find_unspent(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.998")) - tmpl = self.nodes[0].getblocktemplate() - # TODO: add a transaction with witness to mempool, and verify it's not - # selected for mining. + # Create a 3-tx chain: tx1 (non-segwit input, paying to a segwit output) -> + # tx2 (segwit input, paying to a non-segwit output) -> + # tx3 (non-segwit input, paying to a non-segwit output). + # tx1 is allowed to appear in the block, but no others. + txid1 = send_to_witness(1, self.nodes[0], find_unspent(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.996")) + hex_tx = self.nodes[0].gettransaction(txid)['hex'] + tx = FromHex(CTransaction(), hex_tx) + assert(tx.wit.is_null()) # This should not be a segwit input + assert(txid1 in self.nodes[0].getrawmempool()) + + # Now create tx2, which will spend from txid1. + tx = CTransaction() + tx.vin.append(CTxIn(COutPoint(int(txid1, 16), 0), b'')) + tx.vout.append(CTxOut(int(49.99*COIN), CScript([OP_TRUE]))) + tx2_hex = self.nodes[0].signrawtransaction(ToHex(tx))['hex'] + txid2 = self.nodes[0].sendrawtransaction(tx2_hex) + tx = FromHex(CTransaction(), tx2_hex) + assert(not tx.wit.is_null()) + + # Now create tx3, which will spend from txid2 + tx = CTransaction() + tx.vin.append(CTxIn(COutPoint(int(txid2, 16), 0), b"")) + tx.vout.append(CTxOut(int(49.95*COIN), CScript([OP_TRUE]))) # Huge fee + tx.calc_sha256() + txid3 = self.nodes[0].sendrawtransaction(ToHex(tx)) + assert(tx.wit.is_null()) + assert(txid3 in self.nodes[0].getrawmempool()) + + # Now try calling getblocktemplate() without segwit support. + template = self.nodes[0].getblocktemplate() + + # Check that tx1 is the only transaction of the 3 in the template. + template_txids = [ t['txid'] for t in template['transactions'] ] + assert(txid2 not in template_txids and txid3 not in template_txids) + assert(txid1 in template_txids) + + # Check that running with segwit support results in all 3 being included. + template = self.nodes[0].getblocktemplate({"rules": ["segwit"]}) + template_txids = [ t['txid'] for t in template['transactions'] ] + assert(txid1 in template_txids) + assert(txid2 in template_txids) + assert(txid3 in template_txids) + + # Mine a block to clear the gbt cache again. + self.nodes[0].generate(1) print("Verify behaviour of importaddress, addwitnessaddress and listunspent")