Skip to content
Permalink
Browse files

Merge #9283: A few more CTransactionRef optimizations

91335ba Remove unused MakeTransactionRef overloads (Pieter Wuille)
6713f0f Make FillBlock consume txn_available to avoid shared_ptr copies (Pieter Wuille)
62607d7 Convert COrphanTx to keep a CTransactionRef (Pieter Wuille)
c44e4c4 Make AcceptToMemoryPool take CTransactionRef (Pieter Wuille)
  • Loading branch information...
laanwj committed Jan 4, 2017
2 parents d9ae1ce + 91335ba commit 869781c51cc1b6ee0a8635bcefbcbe11db7f5b25
@@ -18,7 +18,7 @@ static void AddTx(const CTransaction& tx, const CAmount& nFee, CTxMemPool& pool)
unsigned int sigOpCost = 4;
LockPoints lp;
pool.addUnchecked(tx.GetHash(), CTxMemPoolEntry(
tx, nFee, nTime, dPriority, nHeight, pool.HasNoInputsOf(tx),
MakeTransactionRef(tx), nFee, nTime, dPriority, nHeight, pool.HasNoInputsOf(tx),
tx.GetValueOut(), spendsCoinbase, sigOpCost, lp));
}

@@ -142,8 +142,9 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const {
return txn_available[index] ? true : false;
}

ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) const {
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) {
assert(!header.IsNull());
uint256 hash = header.GetHash();
block = header;
block.vtx.resize(txn_available.size());

@@ -154,8 +155,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
return READ_STATUS_INVALID;
block.vtx[i] = vtx_missing[tx_missing_offset++];
} else
block.vtx[i] = txn_available[i];
block.vtx[i] = std::move(txn_available[i]);
}

// Make sure we can't call FillBlock again.
header.SetNull();
txn_available.clear();

if (vtx_missing.size() != tx_missing_offset)
return READ_STATUS_INVALID;

@@ -170,10 +176,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
return READ_STATUS_CHECKBLOCK_FAILED;
}

LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", header.GetHash().ToString(), prefilled_count, mempool_count, vtx_missing.size());
LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, vtx_missing.size());
if (vtx_missing.size() < 5) {
for (const auto& tx : vtx_missing)
LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", header.GetHash().ToString(), tx->GetHash().ToString());
LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString());
}

return READ_STATUS_OK;
@@ -202,7 +202,7 @@ class PartiallyDownloadedBlock {

ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock);
bool IsTxAvailable(size_t index) const;
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) const;
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing);
};

#endif
@@ -51,7 +51,7 @@ struct IteratorComparator

struct COrphanTx {
// When modifying, adapt the copy of this definition in tests/DoS_tests.
CTransaction tx;
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
};
@@ -586,9 +586,9 @@ void UnregisterNodeSignals(CNodeSignals& nodeSignals)
// mapOrphanTransactions
//

bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
uint256 hash = tx.GetHash();
const uint256& hash = tx->GetHash();
if (mapOrphanTransactions.count(hash))
return false;

@@ -599,7 +599,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(c
// have been mined or received.
// 100 orphans, each of which is at most 99,999 bytes big is
// at most 10 megabytes of orphans and somewhat more byprev index (in the worst case):
unsigned int sz = GetTransactionWeight(tx);
unsigned int sz = GetTransactionWeight(*tx);
if (sz >= MAX_STANDARD_TX_WEIGHT)
{
LogPrint("mempool", "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString());
@@ -608,7 +608,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(c

auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME});
assert(ret.second);
BOOST_FOREACH(const CTxIn& txin, tx.vin) {
BOOST_FOREACH(const CTxIn& txin, tx->vin) {
mapOrphanTransactionsByPrev[txin.prevout].insert(ret.first);
}

@@ -622,7 +622,7 @@ int static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
map<uint256, COrphanTx>::iterator it = mapOrphanTransactions.find(hash);
if (it == mapOrphanTransactions.end())
return 0;
BOOST_FOREACH(const CTxIn& txin, it->second.tx.vin)
BOOST_FOREACH(const CTxIn& txin, it->second.tx->vin)
{
auto itPrev = mapOrphanTransactionsByPrev.find(txin.prevout);
if (itPrev == mapOrphanTransactionsByPrev.end())
@@ -644,7 +644,7 @@ void EraseOrphansFor(NodeId peer)
map<uint256, COrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
if (maybeErase->second.fromPeer == peer)
{
nErased += EraseOrphanTx(maybeErase->second.tx.GetHash());
nErased += EraseOrphanTx(maybeErase->second.tx->GetHash());
}
}
if (nErased > 0) LogPrint("mempool", "Erased %d orphan tx from peer %d\n", nErased, peer);
@@ -665,7 +665,7 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRE
{
map<uint256, COrphanTx>::iterator maybeErase = iter++;
if (maybeErase->second.nTimeExpire <= nNow) {
nErased += EraseOrphanTx(maybeErase->second.tx.GetHash());
nErased += EraseOrphanTx(maybeErase->second.tx->GetHash());
} else {
nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
}
@@ -736,7 +736,7 @@ void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIn
auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout);
if (itByPrev == mapOrphanTransactionsByPrev.end()) continue;
for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
const CTransaction& orphanTx = (*mi)->second.tx;
const CTransaction& orphanTx = *(*mi)->second.tx;
const uint256& orphanHash = orphanTx.GetHash();
vOrphanErase.push_back(orphanHash);
}
@@ -1600,7 +1600,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,

deque<COutPoint> vWorkQueue;
vector<uint256> vEraseQueue;
CTransaction tx(deserialize, vRecv);
CTransactionRef ptx;
vRecv >> ptx;
const CTransaction& tx = *ptx;

CInv inv(MSG_TX, tx.GetHash());
pfrom->AddInventoryKnown(inv);
@@ -1613,7 +1615,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
pfrom->setAskFor.erase(inv.hash);
mapAlreadyAskedFor.erase(inv.hash);

if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs)) {
if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs)) {
mempool.check(pcoinsTip);
RelayTransaction(tx, connman);
for (unsigned int i = 0; i < tx.vout.size(); i++) {
@@ -1638,7 +1640,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
mi != itByPrev->second.end();
++mi)
{
const CTransaction& orphanTx = (*mi)->second.tx;
const CTransactionRef& porphanTx = (*mi)->second.tx;
const CTransaction& orphanTx = *porphanTx;
const uint256& orphanHash = orphanTx.GetHash();
NodeId fromPeer = (*mi)->second.fromPeer;
bool fMissingInputs2 = false;
@@ -1650,7 +1653,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,

if (setMisbehaving.count(fromPeer))
continue;
if (AcceptToMemoryPool(mempool, stateDummy, orphanTx, true, &fMissingInputs2)) {
if (AcceptToMemoryPool(mempool, stateDummy, porphanTx, true, &fMissingInputs2)) {
LogPrint("mempool", " accepted orphan tx %s\n", orphanHash.ToString());
RelayTransaction(orphanTx, connman);
for (unsigned int i = 0; i < orphanTx.vout.size(); i++) {
@@ -1703,7 +1706,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
pfrom->AddInventoryKnown(_inv);
if (!AlreadyHave(_inv)) pfrom->AskFor(_inv);
}
AddOrphanTx(tx, pfrom->GetId());
AddOrphanTx(ptx, pfrom->GetId());

// DoS prevention: do not allow mapOrphanTransactions to grow unbounded
unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS));
@@ -453,8 +453,6 @@ struct CMutableTransaction
typedef std::shared_ptr<const CTransaction> CTransactionRef;
static inline CTransactionRef MakeTransactionRef() { return std::make_shared<const CTransaction>(); }
template <typename Tx> static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared<const CTransaction>(std::forward<Tx>(txIn)); }
static inline CTransactionRef MakeTransactionRef(const CTransactionRef& txIn) { return txIn; }
static inline CTransactionRef MakeTransactionRef(CTransactionRef&& txIn) { return std::move(txIn); }

/** Compute the weight of a transaction, as defined by BIP 141 */
int64_t GetTransactionWeight(const CTransaction &tx);
@@ -883,8 +883,8 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
CMutableTransaction mtx;
if (!DecodeHexTx(mtx, request.params[0].get_str()))
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
CTransaction tx(std::move(mtx));
uint256 hashTx = tx.GetHash();
CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
const uint256& hashTx = tx->GetHash();

bool fLimitFree = false;
CAmount nMaxRawTxFee = maxTxFee;
@@ -899,7 +899,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
// push to local node and sync with wallets
CValidationState state;
bool fMissingInputs;
if (!AcceptToMemoryPool(mempool, state, tx, fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {
if (!AcceptToMemoryPool(mempool, state, std::move(tx), fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {
if (state.IsInvalid()) {
throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason()));
} else {
@@ -24,11 +24,11 @@
#include <boost/test/unit_test.hpp>

// Tests this internal-to-main.cpp method:
extern bool AddOrphanTx(const CTransaction& tx, NodeId peer);
extern bool AddOrphanTx(const CTransactionRef& tx, NodeId peer);
extern void EraseOrphansFor(NodeId peer);
extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans);
struct COrphanTx {
CTransaction tx;
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
};
@@ -122,7 +122,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
BOOST_CHECK(!connman->IsBanned(addr));
}

CTransaction RandomOrphan()
CTransactionRef RandomOrphan()
{
std::map<uint256, COrphanTx>::iterator it;
it = mapOrphanTransactions.lower_bound(GetRandHash());
@@ -150,30 +150,30 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
tx.vout[0].nValue = 1*CENT;
tx.vout[0].scriptPubKey = GetScriptForDestination(key.GetPubKey().GetID());

AddOrphanTx(tx, i);
AddOrphanTx(MakeTransactionRef(tx), i);
}

// ... and 50 that depend on other orphans:
for (int i = 0; i < 50; i++)
{
CTransaction txPrev = RandomOrphan();
CTransactionRef txPrev = RandomOrphan();

CMutableTransaction tx;
tx.vin.resize(1);
tx.vin[0].prevout.n = 0;
tx.vin[0].prevout.hash = txPrev.GetHash();
tx.vin[0].prevout.hash = txPrev->GetHash();
tx.vout.resize(1);
tx.vout[0].nValue = 1*CENT;
tx.vout[0].scriptPubKey = GetScriptForDestination(key.GetPubKey().GetID());
SignSignature(keystore, txPrev, tx, 0, SIGHASH_ALL);
SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL);

AddOrphanTx(tx, i);
AddOrphanTx(MakeTransactionRef(tx), i);
}

// This really-big orphan should be ignored:
for (int i = 0; i < 10; i++)
{
CTransaction txPrev = RandomOrphan();
CTransactionRef txPrev = RandomOrphan();

CMutableTransaction tx;
tx.vout.resize(1);
@@ -183,15 +183,15 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
for (unsigned int j = 0; j < tx.vin.size(); j++)
{
tx.vin[j].prevout.n = j;
tx.vin[j].prevout.hash = txPrev.GetHash();
tx.vin[j].prevout.hash = txPrev->GetHash();
}
SignSignature(keystore, txPrev, tx, 0, SIGHASH_ALL);
SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL);
// Re-use same signature for other inputs
// (they don't have to be valid for this test)
for (unsigned int j = 1; j < tx.vin.size(); j++)
tx.vin[j].scriptSig = tx.vin[0].scriptSig;

BOOST_CHECK(!AddOrphanTx(tx, i));
BOOST_CHECK(!AddOrphanTx(MakeTransactionRef(tx), i));
}

// Test EraseOrphansFor:
@@ -85,17 +85,23 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
BOOST_CHECK_EQUAL(pool.size(), poolSize - 1);

CBlock block2;
std::vector<CTransactionRef> vtx_missing;
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions
{
PartiallyDownloadedBlock tmp = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
partialBlock = tmp;
}

vtx_missing.push_back(block.vtx[2]); // Wrong transaction
partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that
// Wrong transaction
{
PartiallyDownloadedBlock tmp = partialBlock;
partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that
partialBlock = tmp;
}
bool mutated;
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));

vtx_missing[0] = block.vtx[1];
CBlock block3;
BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK);
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
BOOST_CHECK(!mutated);
@@ -181,17 +187,24 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);

CBlock block2;
std::vector<CTransactionRef> vtx_missing;
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions
{
PartiallyDownloadedBlock tmp = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
partialBlock = tmp;
}

vtx_missing.push_back(block.vtx[1]); // Wrong transaction
partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that
// Wrong transaction
{
PartiallyDownloadedBlock tmp = partialBlock;
partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that
partialBlock = tmp;
}
bool mutated;
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));

vtx_missing[0] = block.vtx[0];
CBlock block3;
BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK);
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
BOOST_CHECK(!mutated);
@@ -200,7 +213,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
block.vtx.clear();
block2.vtx.clear();
block3.vtx.clear();
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
}
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
}
@@ -240,8 +253,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);

CBlock block2;
std::vector<CTransactionRef> vtx_missing;
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK);
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK);
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
bool mutated;
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
@@ -250,7 +263,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
txhash = block.vtx[1]->GetHash();
block.vtx.clear();
block2.vtx.clear();
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
}
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
}
@@ -151,7 +151,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransaction &txn, CTxMemPo
// Hack to assume either its completely dependent on other mempool txs or not at all
CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0;

return CTxMemPoolEntry(txn, nFee, nTime, dPriority, nHeight,
return CTxMemPoolEntry(MakeTransactionRef(txn), nFee, nTime, dPriority, nHeight,
hasNoDependencies, inChainValue, spendsCoinbase, sigOpCost, lp);
}

@@ -23,7 +23,7 @@ ToMemPool(CMutableTransaction& tx)
LOCK(cs_main);

CValidationState state;
return AcceptToMemoryPool(mempool, state, tx, false, NULL, true, 0);
return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), false, NULL, true, 0);
}

BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)

0 comments on commit 869781c

Please sign in to comment.
You can’t perform that action at this time.