Skip to content

[GetTransaction] remove unneeded cs_main lock acquire#22609

Merged
fanquake merged 1 commit intobitcoin:masterfrom
theStack:202107-gettransaction_remove_lock
Aug 3, 2021
Merged

[GetTransaction] remove unneeded cs_main lock acquire#22609
fanquake merged 1 commit intobitcoin:masterfrom
theStack:202107-gettransaction_remove_lock

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Aug 2, 2021

This PR is a follow-up to #22383. For reading from the mempool, only mempool.cs needs to be locked (see suggestion by MarcoFalke):

bitcoin/src/txmempool.h

Lines 554 to 558 in b620b2d

* 2. Locking `mempool.cs` without `cs_main` will give a view of a mempool
* consistent with some chain that was active since `cs_main` was last
* locked, and that is fully populated as described above. It is ok for
* code that only needs to query or remove transactions from the mempool
* to lock just `mempool.cs` without `cs_main`.

CTxMemPool::get() acquires this lock:

bitcoin/src/txmempool.cpp

Lines 822 to 829 in b620b2d

CTransactionRef CTxMemPool::get(const uint256& hash) const
{
LOCK(cs);
indexed_transaction_set::const_iterator i = mapTx.find(hash);
if (i == mapTx.end())
return nullptr;
return i->GetSharedTx();
}

so we don't need to acquire any lock ourselves in GetTransaction(), as the other functions called in the remaining parts also don't need to have cs_main locked.

@fanquake fanquake requested review from jnewbery and maflcko August 3, 2021 01:50
@tryphe
Copy link
Contributor

tryphe commented Aug 3, 2021

Concept ACK. tested 4a1b2a7 but not extensively.

@jnewbery
Copy link
Contributor

jnewbery commented Aug 3, 2021

Code review ACK 4a1b2a7

ReadBlockFromDisk() takes cs_main internally, so this also removes a recursive lock of cs_main 😗 👌

I had a bunch of other small improvements to GetTransaction() here: https://github.com/jnewbery/bitcoin/tree/2021-08-get-transaction. Feel free to take any of them if you think they're useful.

@fanquake
Copy link
Member

fanquake commented Aug 3, 2021

I had a bunch of other small improvements to GetTransaction() here: https://github.com/jnewbery/bitcoin/tree/2021-08-get-transaction. Feel free to take any of them if you think they're useful.

Lets PR those separately.

@fanquake fanquake merged commit eaf09bd into bitcoin:master Aug 3, 2021
@LarryRuane
Copy link
Contributor

LarryRuane commented Aug 3, 2021

ACK (post-merge)

I did some research, may as well post the results here.

GetTransaction() (which we've removed the LOCK() from) can call TxIndex::FindTx(); I was wondering how we can be sure that no lock is required. I stepped through the relevant code with gdb, and got to this backtrace:

#0  leveldb::MutexLock::MutexLock (this=0x7f5e09a46450, mu=0x1) at ./leveldb/util/mutexlock.h:25
#1  0x0000562206cd5591 in leveldb::DBImpl::Get (this=0x5622082b6b30, options=..., key=..., value=0x7f5e09a464f0) at leveldb/db/db_impl.cc:1114
#2  0x0000562206879abd in CDBWrapper::Read<std::pair<unsigned char, uint256>, CDiskTxPos> (this=0x5622082aa570, key={...}, value=...) at ./dbwrapper.h:239
#3  0x00005622068775a0 in TxIndex::DB::ReadTxPos (this=0x5622082aa570, txid=..., pos=...) at index/txindex.cpp:45
#4  0x0000562206878a7f in TxIndex::FindTx (this=0x5622083db050, tx_hash=..., block_hash=..., tx=std::shared_ptr<const CTransaction> (empty) = {...}) at index/txindex.cpp:234
#5  0x00005622064d580a in GetTransaction (block_index=0x7f5dd0004110, mempool=0x5622082b1600, hash=..., consensusParams=..., hashBlock=...) at node/transaction.cpp:137

So internally, leveldb is doing its own locking, which makes sense. @theStack also found this brief but useful article about leveldb locking that indicates that what we're doing is safe.

As for the other things GetTransaction() does, the mempool has been well-researched, and reading from a block file is okay without a lock, because they're append-only.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021
@fanquake
Merge bitcoin#22609: [GetTransaction] remove unneeded cs_main lock ac…
cf4a7f7 
…quire

4a1b2a7 [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner)

Pull request description:

  This PR is a follow-up to bitcoin#22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](bitcoin#22383 (comment))):

  https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.h#L554-L558

  `CTxMemPool::get()` acquires this lock:

  https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.cpp#L822-L829

   so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked.

ACKs for top commit:
  tryphe:
    Concept ACK. tested 4a1b2a7 but not extensively.
  jnewbery:
    Code review ACK 4a1b2a7

Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jnewbery jnewbery Awaiting requested review from jnewbery

@maflcko maflcko Awaiting requested review from maflcko

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@theStack @tryphe @jnewbery @fanquake @LarryRuane