A few more CTransactionRef optimizations

sipa commented at 10:36 am on December 5, 2016: member

This improves a few more things through the use of CTransactionRef (aka std::shared_ptr<const CTransaction>):

By making AcceptToMemoryPool take a CTransactionRef, we can move transactions during a reorg back from loaded blocks to the mempool without copying them.
By making COrphanTx store a CTransactionRef, we can avoid a copy when orphans move to the mempool.
By making PartiallyDownloadedBlock::FillBlock consume its previously-initialized state, we can avoid 2 atomic refcount updates per mempool-found transaction in a reconstructed compact block.

TheBlueMatt commented at 0:58 am on December 6, 2016: member

Concept ACK….was just working on a branch and added a TODO to make orphan/ATMP take a ref….

dcousens approved

in src/validation.h: in 4b23a0b850 outdated

310@@ -311,11 +311,11 @@ void FlushStateToDisk();
311 void PruneAndFlush();
312 
313 /** (try to) add transaction to memory pool **/
314-bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
315+bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const std::shared_ptr<const CTransaction> &tx, bool fLimitFree,

kallewoof commented at 4:30 am on December 6, 2016:

Why not const CTransactionRef& tx as 3rd argument?

sipa commented at 4:32 am on December 6, 2016:

Good point!

kallewoof commented at 4:34 am on December 6, 2016:

Saw the same thing in 3 other places. I can tag them for you, but didn’t wanna spam in case I was misunderstanding.

sipa commented at 4:39 am on December 6, 2016:

Most of this PR was written before CTransactionRef was introduced, and I forgot to patch it up. I’m updating it now (and indeed, 3 cases were left).

sipa force-pushed on Dec 6, 2016

dcousens approved

fanquake added the label Refactoring on Dec 8, 2016

CodeShark commented at 9:00 am on December 11, 2016: contributor

concept ACK

gmaxwell commented at 8:25 pm on December 12, 2016: contributor

ACK.

paveljanik commented at 10:50 am on December 13, 2016: contributor

Compiles cleanly utACK https://github.com/bitcoin/bitcoin/pull/9283/commits/8308ca1e922ca20fe8cba4bb2a094bf2a94abfac

Make AcceptToMemoryPool take CTransactionRef c44e4c467c

Convert COrphanTx to keep a CTransactionRef 62607d796c

Make FillBlock consume txn_available to avoid shared_ptr copies 6713f0f142

Remove unused MakeTransactionRef overloads 91335ba389

sipa force-pushed on Dec 22, 2016

sipa commented at 2:27 am on December 22, 2016: member

Rebased after the merge of #8589. This supersedes #9395.

sipa commented at 6:50 pm on December 27, 2016: member

Can I haz review?

in src/blockencodings.cpp: in 91335ba389

141@@ -142,8 +142,9 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const {
142     return txn_available[index] ? true : false;
143 }
144 
145-ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) const {
146+ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) {
147     assert(!header.IsNull());
148+    uint256 hash = header.GetHash();

dcousens commented at 11:33 pm on December 27, 2016:

const?

dcousens approved

dcousens commented at 11:38 pm on December 27, 2016: contributor

re-utACK 91335ba

jonasschnelli approved

jonasschnelli commented at 7:53 am on December 28, 2016: contributor

Code Review utACK 91335ba389918966c94eeb7071b6c5a998bf1be8

theuni commented at 1:48 am on December 30, 2016: member

Are the atomic increments in FillBlock enough to justify making it non-reusable? I have no knowledge of how it might be called, and no preference either way. Just seems like a weird optim.

theuni commented at 1:49 am on December 30, 2016: member

utACK https://github.com/bitcoin/bitcoin/commit/91335ba389918966c94eeb7071b6c5a998bf1be8.

sipa commented at 2:00 pm on December 30, 2016: member

@theuni I don’t know - it’s certainly the least important commit in here.

laanwj commented at 11:23 am on January 4, 2017: member

lightly-tested ACK 91335ba

laanwj merged this on Jan 4, 2017

laanwj closed this on Jan 4, 2017

laanwj referenced this in commit 869781c51c on Jan 4, 2017

codablock referenced this in commit 8f423499dd on Jan 18, 2018

gladcow referenced this in commit 078324f4b6 on Mar 5, 2018

gladcow referenced this in commit 327ea4507d on Mar 8, 2018

gladcow referenced this in commit a80c2a51da on Mar 13, 2018

gladcow referenced this in commit e39841a5aa on Mar 14, 2018

gladcow referenced this in commit e6fcd67e11 on Mar 15, 2018

gladcow referenced this in commit f1939b3098 on Mar 15, 2018

gladcow referenced this in commit 45e32c9c4a on Mar 15, 2018

gladcow referenced this in commit 72091614b9 on Mar 15, 2018

gladcow referenced this in commit 9f53577fcc on Mar 24, 2018

gladcow referenced this in commit 385fb8d7f9 on Apr 4, 2018

UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018

andvgal referenced this in commit 592dc07136 on Jan 6, 2019

andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019

CryptoCentric referenced this in commit f7e30fe0de on Feb 26, 2019

CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019

in src/rpc/rawtransaction.cpp:902 in 91335ba389

898@@ -899,7 +899,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
899         // push to local node and sync with wallets
900         CValidationState state;
901         bool fMissingInputs;
902-        if (!AcceptToMemoryPool(mempool, state, tx, fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {
903+        if (!AcceptToMemoryPool(mempool, state, std::move(tx), fLimitFree, &fMissingInputs, false, nMaxRawTxFee)) {

jnewbery commented at 12:06 pm on August 24, 2020:

I believe this std::move is unnecessary. ATMP takes a const reference to tx, so casting tx to an rvalue here doesn’t do anything (ATMP must copy from tx if it wants to retain shared ownership).

DrahtBot locked this on Feb 15, 2022

A few more CTransactionRef optimizations #9283