rpc: make `gettxoutsettinfo` run lock-free #6290

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2015_06_gettxoutsetinfo_freedom changing 2 files +4 −3

laanwj commented at 11:52 AM on June 16, 2015: member

For leveldb "An iterator operates on a snapshot of the database taken when the iterator is created". This means that it is unnecessary to lock out other threads while computing statistics, and neither to hold cs_main for the whole time. Let the thread run free.

I've been using this patch for about 8 months without issues, including on a node that automatically dumps the information for every N blocks.

rpc: make `gettxoutsettinfo` run lock-free

For leveldb "An iterator operates on a snapshot of the database taken
when the iterator is created". This means that it is unnecessary to
lock out other threads while computing statistics, and neither to hold
cs_main for the whole time. Let the thread run free.

57092ed9e7

laanwj added the label RPC on Jun 16, 2015
sipa commented at 11:59 AM on June 16, 2015: member

Untested ACK
jgarzik commented at 12:12 PM on June 16, 2015: contributor

ut ACK
jonasschnelli commented at 7:04 AM on June 17, 2015: contributor

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

But i agree that we should do more of these LOCK space reducing.
laanwj commented at 4:20 PM on June 17, 2015: member

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

Whoa. Good catch. I expected that this would invoke CCoinsViewDB::GetBestBlock() which accesses just the db and not any internal fields or global state. But it's a virtual. It shouldn't actually be using the cached value at all but query the db.
laanwj commented at 4:24 PM on June 17, 2015: member

Correction: CCoinsViewCache does not derive from CCoinsViewDB (or the other way around), it just wraps one. So there is no problem AFAIK.
jonasschnelli commented at 7:49 PM on June 17, 2015: contributor

Right. Was following the wrong path... it calls uint256 CCoinsViewDB::GetBestBlock(). Sorry for the noise.
laanwj merged this on Jun 19, 2015
laanwj closed this on Jun 19, 2015
laanwj referenced this in commit 57539884f2 on Jun 19, 2015

in src/txdb.cpp:None in 57092ed9e7

 146 | @@ -147,7 +147,10 @@ bool CCoinsViewDB::GetStats(CCoinsStats &stats) const {
 147 |              return error("%s: Deserialize or I/O error - %s", __func__, e.what());
 148 |          }
 149 |      }
 150 | -    stats.nHeight = mapBlockIndex.find(GetBestBlock())->second->nHeight;
 151 | +    {
 152 | +        LOCK(cs_main);
 153 | +        stats.nHeight = mapBlockIndex.find(stats.hashBlock)->second->nHeight;
 154 | +    }

luke-jr commented at 7:10 AM on June 23, 2015:

Couldn't nHeight change before we get to this block, after we start getting the other stats?

sipa commented at 7:39 AM on June 23, 2015:

This does not query the height of the tip. It queries the height of the block the UTXO in this view corresponds with, and due to the snapshot, that does not change.

luke-jr commented at 7:44 AM on June 23, 2015:

Ah, right.

zkbot referenced this in commit 9af55822fb on Feb 15, 2017
zkbot referenced this in commit a7cf698873 on Mar 4, 2017
random-zebra referenced this in commit 73d26f20e9 on May 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels