Fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=49347
LimitOrphans() can log expired tx and it should log evicted tx as well instead of returning the nEvicted number for caller to print the message.
Since LimitOrphans() now returns void, the redundant assertion check in fuzz test is also removed.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
128 | // Sweep again 5 minutes after the next entry that expires in order to batch the linear scan. 129 | nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; 130 | - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased); 131 | + if (nExpired > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nExpired); 132 | } 133 | + unsigned int nEvicted = nExpired;
You are now logging expired txs as evicted, see the caller:
// DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789)
unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS));
unsigned int nEvicted = m_orphanage.LimitOrphans(nMaxOrphanTx);
if (nEvicted > 0) {
LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted);
}
I didn't notice LimitOrphans is called here. So it meant to return the number of evicted tx (not including expired), though the number is only used for logging.
Not sure if the prefix is accurate. You are modifying a log message, not a test.
I think an alternative way to solve this would be to move both log messages into the function and not return anything?
134 | @@ -135,7 +135,8 @@ unsigned int TxOrphanage::LimitOrphans(unsigned int max_orphans) 135 | EraseTx(m_orphan_list[randompos]->first); 136 | ++nEvicted; 137 | } 138 | - return nEvicted; 139 | + if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); 140 | + return nExpired + nEvicted;
The return value isn't used outside of tests, so I think it can be removed. If someone is interested in the size, they can just call .size().
auto size_before = orphanage.Size();
auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
auto n_evicted = WITH_LOCK(g_cs_orphans, return orphanage.LimitOrphans(limit));
Assert(size_before - n_evicted <= limit);
Assert(orphanage.Size() <= limit);
In most cases size_before - n_evicted == orphange.Size(), so we don't need the return value of LimitOrphans. But IMO it will not hurt to use the return value for an additional check. Actually I also think EraseForBlock can return nErased.
In most cases size_before - n_evicted == orphange.Size(), so we don't need the return value of LimitOrphans
Can you give an example where the equality wouldn't hold, assuming after the changes in this pull requests?
The equality wouldn't hold if n_evicted or n_expired is incremented but EraseTx fail to remove tx from m_orphanage. It seems this never happen since the txid passed to EraseTx is valid. So feel free to request change and I will make the function return void.
Yeah, I am thinking, either:
Regardless of which case holds, removing the return value is likely the correct "fix".
127 | } 128 | } 129 | // Sweep again 5 minutes after the next entry that expires in order to batch the linear scan. 130 | nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; 131 | - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased); 132 | + if (nExpired > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nExpired);
The current name isn't optimal. There are two options here:
nErased to nExpired, because it is not needed.num_expired, because the style guide recommends that for new symbols.
I prefer the 1st option. Since other functions in txorphanage also have camelCase symbols, its better to change them all in a style: commit.
Feel free to change the PR title since its not only about log now.
111 | static int64_t nNextSweep;
112 | int64_t nNow = GetTime();
113 | if (nNextSweep <= nNow) {
114 | // Sweep out expired orphan pool entries:
115 | - int nErased = 0;
116 | + unsigned int nErased = 0;
This doesn't matter, but it might be better to leave this as-is as well, as the format token is %d, not %u
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
review ACK 9fd3102319903694f27adabdaafdaf770df40e83
ACK 9fd3102319903694f27adabdaafdaf770df40e83
LimitOrphans() removes expired transactions from the orphanage and additionally evicts transactions at random should the limit still be exceeded after removing expired transactions. Before this PR, the number of evicted transactions (excluding the number of expired transactions) was returned, which let to hitting an asserting in the fuzz tests that assumed the return value to be the total number of removed transactions (evicted + expired).
This PR removes the assertion from the fuzz test and changes LimitOrphans() to return nothing (the logging of the number of evicted transactions is therefore moved to the orphanage).
nit: The PR description still says "Now the function returns the sum of expired and evicted tx, used for test assertion." which is not true for the current version of the PR.
`LimitOrphans()` can log expired tx and it should log evicted tx as well
instead of returning the number for caller to print the message.
Since `LimitOrphans()` now return void, the redundant assertion check in
fuzz test is also removed.
Thanks for reviewing, I modify the PR description and a typo redundant in commit message.
