diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -2,4 +2,5 @@ - +This release includes the following features and fixes: + - Fixes an issue with `getblocktemplate` returning fees and sigops in non-CTOR ordering after Nov 15th CTOR activation. diff --git a/src/miner.h b/src/miner.h --- a/src/miner.h +++ b/src/miner.h @@ -24,10 +24,19 @@ static const bool DEFAULT_PRINTPRIORITY = false; +struct CBlockTemplateEntry { + CTransactionRef tx; + Amount fees; + int64_t sigOpCount; + + CBlockTemplateEntry(CTransactionRef tx, Amount fees, int64_t sigOpCount) + : tx(tx), fees(fees), sigOpCount(sigOpCount){}; +}; + struct CBlockTemplate { CBlock block; - std::vector vTxFees; - std::vector vTxSigOpsCount; + + std::vector entries; }; // Container for tracking updates to ancestor feerate as we include (parent) diff --git a/src/miner.cpp b/src/miner.cpp --- a/src/miner.cpp +++ b/src/miner.cpp @@ -32,9 +32,6 @@ #include #include -#include -#include - ////////////////////////////////////////////////////////////////////////////// // // BitcoinMiner @@ -141,12 +138,8 @@ // Pointer for convenience. pblock = &pblocktemplate->block; - // Add dummy coinbase tx as first transaction. - pblock->vtx.emplace_back(); - // updated at end - pblocktemplate->vTxFees.push_back(-SATOSHI); - // updated at end - pblocktemplate->vTxSigOpsCount.push_back(-1); + // Add dummy coinbase tx as first transaction. It is updated at the end. + pblocktemplate->entries.emplace_back(CTransactionRef(), -SATOSHI, -1); LOCK2(cs_main, mempool.cs); CBlockIndex *pindexPrev = chainActive.Tip(); @@ -178,11 +171,11 @@ if (IsMagneticAnomalyEnabled(*config, pindexPrev)) { // If magnetic anomaly is enabled, we make sure transaction are // canonically ordered. - std::sort(std::begin(pblock->vtx) + 1, std::end(pblock->vtx), - [](const std::shared_ptr &a, - const std::shared_ptr &b) -> bool { - return a->GetId() < b->GetId(); - }); + // FIXME: Use a zipped list. See T479 + std::sort(std::begin(pblocktemplate->entries) + 1, + std::end(pblocktemplate->entries), + [](const CBlockTemplateEntry &a, const CBlockTemplateEntry &b) + -> bool { return a.tx->GetId() < b.tx->GetId(); }); } int64_t nTime1 = GetTimeMicros(); @@ -208,8 +201,8 @@ << std::vector(MIN_TX_SIZE - coinbaseSize - 1); } - pblock->vtx[0] = MakeTransactionRef(coinbaseTx); - pblocktemplate->vTxFees[0] = -1 * nFees; + pblocktemplate->entries[0].tx = MakeTransactionRef(coinbaseTx); + pblocktemplate->entries[0].fees = -1 * nFees; uint64_t nSerializeSize = GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION); @@ -222,8 +215,15 @@ UpdateTime(pblock, *config, pindexPrev); pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, *config); pblock->nNonce = 0; - pblocktemplate->vTxSigOpsCount[0] = GetSigOpCountWithoutP2SH( - *pblock->vtx[0], STANDARD_CHECKDATASIG_VERIFY_FLAGS); + pblocktemplate->entries[0].sigOpCount = GetSigOpCountWithoutP2SH( + *pblocktemplate->entries[0].tx, STANDARD_CHECKDATASIG_VERIFY_FLAGS); + + // Copy all the transactions into the block + // FIXME: This should be removed as it is significant overhead. + // See T479 + for (const CBlockTemplateEntry &tx : pblocktemplate->entries) { + pblock->vtx.push_back(tx.tx); + } CValidationState state; BlockValidationOptions validationOptions(false, false); @@ -350,9 +350,8 @@ } void BlockAssembler::AddToBlock(CTxMemPool::txiter iter) { - pblock->vtx.emplace_back(iter->GetSharedTx()); - pblocktemplate->vTxFees.push_back(iter->GetFee()); - pblocktemplate->vTxSigOpsCount.push_back(iter->GetSigOpCount()); + pblocktemplate->entries.emplace_back(iter->GetSharedTx(), iter->GetFee(), + iter->GetSigOpCount()); nBlockSize += iter->GetTxSize(); ++nBlockTx; nBlockSigOps += iter->GetSigOpCount(); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -632,9 +632,10 @@ entry.pushKV("depends", deps); int index_in_template = i - 1; - entry.push_back( - Pair("fee", pblocktemplate->vTxFees[index_in_template] / SATOSHI)); - int64_t nTxSigOps = pblocktemplate->vTxSigOpsCount[index_in_template]; + entry.pushKV("fee", + pblocktemplate->entries[index_in_template].fees / SATOSHI); + int64_t nTxSigOps = + pblocktemplate->entries[index_in_template].sigOpCount; entry.pushKV("sigops", nTxSigOps); transactions.push_back(entry); diff --git a/test/functional/abc-magnetic-anomaly-mining.py b/test/functional/abc-magnetic-anomaly-mining.py new file mode 100755 --- /dev/null +++ b/test/functional/abc-magnetic-anomaly-mining.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python3 +# Copyright (c) 2014-2016 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test that mining RPC continues to supply correct transaction metadata after +the Nov 2018 protocol upgrade which engages canonical transaction ordering +""" + +import time +import random +import decimal + +from test_framework.blocktools import create_coinbase +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import * + + +class CTORMiningTest(BitcoinTestFramework): + def set_test_params(self): + # Setup two nodes so we can getblocktemplate + # it errors out if it is not connected to other nodes + self.num_nodes = 2 + self.setup_clean_chain = True + self.block_heights = {} + self.tip = None + self.blocks = {} + self.mocktime = int(time.time()) - 600 * 100 + + extra_arg = ['-spendzeroconfchange=0', '-whitelist=127.0.0.1', + "-magneticanomalyactivationtime=%d" % self.mocktime, + "-replayprotectionactivationtime=%d" % (10 * self.mocktime)] + self.extra_args = [extra_arg, extra_arg] + + def run_test(self): + mining_node = self.nodes[0] + + # Helper for updating the times + def update_time(): + mining_node.setmocktime(self.mocktime) + self.mocktime = self.mocktime + 600 + + mining_node.getnewaddress() + + # Generate some unspent utxos and also + # activate magnetic anomaly + for x in range(150): + update_time() + mining_node.generate(1) + + update_time() + unspent = mining_node.listunspent() + + transactions = {} + # Spend all our coinbases + while len(unspent): + inputs = [] + # Grab a random number of inputs + for _ in range(random.randrange(1, 5)): + txin = unspent.pop() + inputs.append({ + 'txid': txin['txid'], + 'vout': 0 # This is a coinbase + }) + if len(unspent) == 0: + break + + outputs = {} + # Calculate a unique fee for this transaction + fee = decimal.Decimal(random.randint( + 1000, 2000)) / decimal.Decimal(1e8) + # Spend to the same number of outputs as inputs, so we can leave + # the amounts unchanged and avoid rounding errors. + # + # NOTE: There will be 1 sigop per output (which equals the number + # of inputs now). We need this randomization to ensure the + # numbers are properly following the transactions in the block + # template metadata + addr = "" + for _ in range(len(inputs)): + addr = mining_node.getnewaddress() + output = { + # 50 BCH per coinbase + addr: decimal.Decimal(50) + } + outputs.update(output) + + # Take the fee off the last output to avoid rounding errors we + # need the exact fee later for assertions + outputs[addr] -= fee + + rawtx = mining_node.createrawtransaction(inputs, outputs) + signedtx = mining_node.signrawtransaction(rawtx) + txid = mining_node.sendrawtransaction(signedtx['hex']) + # number of outputs is the same as the number of sigops in this + # case + transactions.update({txid: {'fee': fee, 'sigops': len(outputs)}}) + + tmpl = mining_node.getblocktemplate() + assert 'proposal' in tmpl['capabilities'] + assert 'coinbasetxn' not in tmpl + + # Check the template transaction metadata and ordering + last_txid = 0 + for txn in tmpl['transactions'][1:]: + txid = txn['txid'] + txnMetadata = transactions[txid] + expectedFeeSats = int(txnMetadata['fee'] * 10**8) + expectedSigOps = txnMetadata['sigops'] + + txid_decoded = int(txid, 16) + + # Assert we got the expected metadata + assert(expectedFeeSats == txn['fee']) + assert(expectedSigOps == txn['sigops']) + # Assert transaction ids are in order + assert(last_txid == 0 or last_txid < txid_decoded) + last_txid = txid_decoded + + +if __name__ == '__main__': + CTORMiningTest().main() diff --git a/test/functional/timing.json b/test/functional/timing.json --- a/test/functional/timing.json +++ b/test/functional/timing.json @@ -1,7 +1,7 @@ [ { "name": "abandonconflict.py", - "time": 13 + "time": 16 }, { "name": "abc-checkdatasig-activation.py", @@ -9,31 +9,35 @@ }, { "name": "abc-cmdline.py", - "time": 9 + "time": 8 }, { "name": "abc-high_priority_transaction.py", - "time": 12 + "time": 10 }, { "name": "abc-magnetic-anomaly-activation.py", - "time": 7 + "time": 6 + }, + { + "name": "abc-magnetic-anomaly-mining.py", + "time": 15 }, { "name": "abc-mempool-accept-txn.py", - "time": 4 + "time": 5 }, { "name": "abc-p2p-compactblocks.py", - "time": 167 + "time": 104 }, { "name": "abc-p2p-fullblocktest.py", - "time": 50 + "time": 81 }, { "name": "abc-parkedchain.py", - "time": 3 + "time": 4 }, { "name": "abc-replay-protection.py", @@ -45,47 +49,47 @@ }, { "name": "abc-transaction-ordering.py", - "time": 15 + "time": 6 }, { "name": "assumevalid.py", - "time": 13 + "time": 20 }, { "name": "bip65-cltv-p2p.py", - "time": 19 + "time": 5 }, { "name": "bip68-112-113-p2p.py", - "time": 34 + "time": 18 }, { "name": "bip68-sequence.py", + "time": 18 + }, + { + "name": "bip68-sequence.py --magnetic-anomaly-time=1", "time": 26 }, { "name": "bipdersig-p2p.py", - "time": 5 + "time": 7 }, { "name": "bitcoin_cli.py", - "time": 13 + "time": 4 }, { "name": "blockchain.py", - "time": 10 + "time": 11 }, { "name": "dbcrash.py", - "time": 1257 + "time": 756 }, { "name": "decodescript.py", - "time": 2 - }, - { - "name": "deprecated_rpc.py", - "time": 18 + "time": 3 }, { "name": "disablewallet.py", @@ -93,15 +97,15 @@ }, { "name": "disconnect_ban.py", - "time": 9 + "time": 8 }, { "name": "example_test.py", - "time": 5 + "time": 4 }, { "name": "fundrawtransaction.py", - "time": 50 + "time": 42 }, { "name": "getblocktemplate_longpoll.py", @@ -109,11 +113,11 @@ }, { "name": "getchaintips.py", - "time": 5 + "time": 4 }, { "name": "httpbasics.py", - "time": 4 + "time": 3 }, { "name": "import-rescan.py", @@ -121,163 +125,163 @@ }, { "name": "importmulti.py", - "time": 24 + "time": 12 }, { "name": "importprunedfunds.py", - "time": 8 + "time": 4 }, { "name": "invalidateblock.py", - "time": 10 + "time": 9 }, { "name": "invalidblockrequest.py", - "time": 14 + "time": 4 }, { "name": "invalidtxrequest.py", - "time": 5 + "time": 4 }, { "name": "keypool-topup.py", - "time": 45 + "time": 19 }, { "name": "keypool.py", - "time": 9 + "time": 8 }, { "name": "listsinceblock.py", - "time": 4 + "time": 5 }, { "name": "listtransactions.py", - "time": 10 + "time": 11 }, { "name": "maxuploadtarget.py", - "time": 49 + "time": 37 }, { "name": "mempool_limit.py", - "time": 7 + "time": 5 }, { "name": "mempool_packages.py", - "time": 34 + "time": 28 }, { "name": "mempool_persist.py", - "time": 21 + "time": 16 }, { "name": "mempool_reorg.py", - "time": 28 + "time": 4 }, { "name": "mempool_resurrect_test.py", - "time": 18 + "time": 3 }, { "name": "mempool_spendcoinbase.py", - "time": 17 + "time": 3 }, { "name": "merkle_blocks.py", - "time": 4 + "time": 5 }, { "name": "minchainwork.py", - "time": 16 + "time": 6 }, { "name": "mining.py", - "time": 18 + "time": 3 }, { "name": "multi_rpc.py", - "time": 5 + "time": 4 }, { "name": "multiwallet.py", - "time": 8 + "time": 9 }, { "name": "net.py", - "time": 17 + "time": 4 }, { "name": "notifications.py", - "time": 7 + "time": 6 }, { "name": "nulldummy.py", - "time": 17 + "time": 4 }, { "name": "p2p-acceptblock.py", - "time": 8 + "time": 7 }, { "name": "p2p-compactblocks.py", - "time": 42 + "time": 23 }, { "name": "p2p-feefilter.py", - "time": 25 + "time": 63 }, { "name": "p2p-fullblocktest.py", - "time": 154 + "time": 184 }, { "name": "p2p-leaktests.py", - "time": 24 + "time": 7 }, { "name": "p2p-mempool.py", - "time": 16 + "time": 3 }, { "name": "p2p-timeouts.py", - "time": 65 + "time": 64 }, { "name": "preciousblock.py", - "time": 5 + "time": 4 }, { "name": "prioritise_transaction.py", - "time": 10 + "time": 6 }, { "name": "proxy_test.py", - "time": 5 + "time": 4 }, { "name": "pruning.py", - "time": 1491 + "time": 1040 }, { "name": "rawtransactions.py", - "time": 37 + "time": 17 }, { "name": "receivedby.py", - "time": 13 + "time": 6 }, { "name": "reindex.py", - "time": 26 + "time": 14 }, { "name": "resendwallettransactions.py", - "time": 5 + "time": 6 }, { "name": "rest.py", - "time": 9 + "time": 11 }, { "name": "rpcbind_test.py", @@ -289,11 +293,11 @@ }, { "name": "sendheaders.py", - "time": 30 + "time": 22 }, { "name": "signmessages.py", - "time": 2 + "time": 3 }, { "name": "signrawtransactions.py", @@ -301,27 +305,27 @@ }, { "name": "txn_clone.py", - "time": 7 + "time": 5 }, { "name": "txn_clone.py --mineblock", - "time": 8 + "time": 6 }, { "name": "txn_doublespend.py", - "time": 6 + "time": 5 }, { "name": "txn_doublespend.py --mineblock", - "time": 5 + "time": 6 }, { "name": "uptime.py", - "time": 3 + "time": 2 }, { "name": "wallet-accounts.py", - "time": 40 + "time": 13 }, { "name": "wallet-dump.py", @@ -329,19 +333,19 @@ }, { "name": "wallet-encryption.py", - "time": 20 + "time": 9 }, { "name": "wallet-hd.py", - "time": 126 + "time": 111 }, { "name": "wallet.py", - "time": 44 + "time": 56 }, { "name": "walletbackup.py", - "time": 101 + "time": 137 }, { "name": "zapwallettxes.py",