[GetTransaction] remove unneeded cs_main lock acquire

[theStack]

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.

[tryphe]

Concept ACK. tested 4a1b2a7 but not extensively.

[jnewbery]

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]

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.

[LarryRuane]

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.