[GetTransaction] remove unneeded cs_main lock acquire #22609

1. 
   theStack commented at 4:48 PM on August 2, 2021: member

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

   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.

2. [GetTransaction] remove unneeded `cs_main` lock acquire 4a1b2a7ba7
3. fanquake requested review from jnewbery on Aug 3, 2021
4. fanquake requested review from MarcoFalke on Aug 3, 2021
5. 
   tryphe commented at 6:47 AM on August 3, 2021: contributor

   Concept ACK. tested 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f but not extensively.

6. 
   jnewbery commented at 10:57 AM on August 3, 2021: member

   Code review ACK 4a1b2a7ba7f804e656a8cd29d5aa80fcbd40904f

   ReadBlockFromDisk() takes cs_main internally, so this also removes a recursive lock of cs_main :kissing: :ok_hand:

   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.

7. 
   fanquake commented at 12:07 PM on August 3, 2021: member

   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.

8. fanquake merged this on Aug 3, 2021
9. fanquake closed this on Aug 3, 2021

   ---

10. 
    LarryRuane commented at 5:58 PM on August 3, 2021: contributor

    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](/bitcoin-bitcoin/0/)  leveldb::MutexLock::MutexLock (this=0x7f5e09a46450, mu=0x1) at ./leveldb/util/mutexlock.h:25
    [#1](/bitcoin-bitcoin/1/)  0x0000562206cd5591 in leveldb::DBImpl::Get (this=0x5622082b6b30, options=..., key=..., value=0x7f5e09a464f0) at leveldb/db/db_impl.cc:1114
    [#2](/bitcoin-bitcoin/2/)  0x0000562206879abd in CDBWrapper::Read<std::pair<unsigned char, uint256>, CDiskTxPos> (this=0x5622082aa570, key={...}, value=...) at ./dbwrapper.h:239
    [#3](/bitcoin-bitcoin/3/)  0x00005622068775a0 in TxIndex::DB::ReadTxPos (this=0x5622082aa570, txid=..., pos=...) at index/txindex.cpp:45
    [#4](/bitcoin-bitcoin/4/)  0x0000562206878a7f in TxIndex::FindTx (this=0x5622083db050, tx_hash=..., block_hash=..., tx=std::shared_ptr<const CTransaction> (empty) = {...}) at index/txindex.cpp:234
    [#5](/bitcoin-bitcoin/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.

11. sidhujag referenced this in commit cf4a7f7c15 on Aug 4, 2021
12. DrahtBot locked this on Aug 18, 2022

Review Requested
jnewberyMarcoFalke