rpc: Fix leveldb iterator leak, and flush before `gettxoutsetinfo` #5115

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2014_10_gettxoutsetinfo changing 2 files +2 −1

laanwj commented at 2:19 PM on October 21, 2014: member
This fixes an iterator leak resulting in
```
bitcoind: db/version_set.cc:789: leveldb::VersionSet::~VersionSet(): Assertion `dummy_versions_.next_ == &dummy_versions_' failed."
```
exception on shutdown (as well as keeping a copy of the utxo data pinned on disk until next restart).

Also make sure to flush pcoinsTip before calling GetStats() to make sure we apply them to the current height.

rpc: Fix leveldb iterator leak, and flush before `gettxoutsetinfo`

This fixes an iterator leak resulting in

    bitcoind: db/version_set.cc:789: leveldb::VersionSet::~VersionSet(): Assertion `dummy_versions_.next_ == &dummy_versions_' failed."

exception on shutdown.

Also make sure to flush pcoinsTip before calling GetStats() to make
sure we apply them to the current height.

33dfbf57d3

in src/rpcblockchain.cpp:None in 33dfbf57d3
```
 318 | @@ -319,6 +319,7 @@ Value gettxoutsetinfo(const Array& params, bool fHelp)
 319 |      Object ret;
 320 |  
 321 |      CCoinsStats stats;
 322 | +    pcoinsTip->Flush();
```
theuni commented at 5:23 PM on October 21, 2014:

Doesn't this somewhat defeat the purpose of the cache? I would expect GetStats() to merge the cached and committed data.

laanwj commented at 5:29 PM on October 21, 2014:

GetStats is a very slow call (it iterates over the entire database), so the overhead of an extra flush is negligible in comparison.

laanwj commented at 5:41 PM on October 21, 2014:

Anyhow if flushing here is controversial I'll remove it, the point of this pull is really the other fix.

theuni commented at 5:57 PM on October 21, 2014:

No controversy, just pointing out that it doesn't do what may be expected. A quick comment would be enough to shut me up :)

sipa commented at 10:46 PM on October 21, 2014:

CCoinsView in general has no means of iterating over the entire dataset, so we're necessarily relying on the lowest level view (the database) which does. I agree it would be nicer if some means of getting a combined resulting view and iterating over it would be nice, but just for GetStats that's not worth it. Also, GetStats is already terribly slow, I don't care about a flush before it.

laanwj commented at 7:38 AM on October 22, 2014:

As for leveldb, from the documentation: "An iterator operates on a snapshot of the database taken when the iterator is created". I think this means that we can mark the gettxoutsetinfo RPC call as thread-safe?

Only the Flush would need the cs_main lock, after that it can just take its time to summarize the snapshot and clean up the iterator, then return the result.

sipa commented at 5:36 PM on October 24, 2014:

Interesting; we should do that.

laanwj commented at 7:18 AM on October 25, 2014:

I've tested that approach over the last few days (on a node where I've set blocknotify to call gettxoutsetinfo), and I've had no problems. I'll submit a new pull for that.
theuni commented at 5:58 PM on October 21, 2014: member

ACK on the scoped_ptr change.
sipa commented at 10:47 PM on October 21, 2014: member

utACK on all changes
laanwj added the label RPC on Oct 22, 2014
laanwj merged this on Oct 27, 2014
laanwj closed this on Oct 27, 2014
laanwj referenced this in commit d9702bcf7c on Oct 27, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels