Furthermore, this assumes an equal need for cache across speculative block filter types.
I think it's reasonable to assume the future/custom filters would also be stored in flat files and the indexing code that is relevant for caching and should be constant across indexes. Maybe usage would be a bit different (clients come only more or less frequently) but I think at least that part of the assumptions here is reasonable.
The only way I can interpret the current code under a multi-blockfilter-index future is that index_sizes.filter_index specifies an equal allocation size for all blockfilter index types. With the current 5% allocation for all blockfilter types, adding a second one would result in a halving of the allocation.
Yeah, that was not my intention. I think each blockfilterindex should probably get 5% (up to a limited number of indexes of course). But it seems questionable if multiple of these would even be running at the same time on the same node when someone has a use-case for other blockfilterindex. Presumably they would make their custom index so that it fits all the needs of their protocol.
On the other hand even if somebody would add a second custom index and wouldn't change this number it probably wouldn't be the end of the world. Hopefully they would change the MAX_FILTER_INDEX_CACHE if they are smart enough to build their own index.
Seems more reasonable to add a separate independent IndexCacheSizes-field for the new block filter index if we ever add one.
Going more explicit/expressive and less flexible is reasonable looking at the situation of this code at the moment. But another way to take would be to Increase the whole blockfilterindex allocation to 10% but cap the individual indexes at 5%. And then each index get's 5% unless there are >2 blockfilterindexes which then have to share 10% or so (could also be 15% and >3, haven't thought about what's reasonable here). This keeps the same level of flexibility and I think I would prefer this since it preserves the current behavior of the code more. If we are making the code less flexible in terms of introducing a separate blockfilterindex, as I said, I think it's better to keep it in a separate PR so it can be discussed independently. I am not against exploring that idea, it's rather that I would like this to get as much feedback as possible and potentially, if the feedback is positive, we may want to simplify the code in other places in a similar way.
For the PR here I would prefer something like this I think (with the necessary documentation change of course):
index_sizes.tx_index = std::min(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0);
index_sizes.txospender_index = std::min(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0);
if (n_indexes > 0) {
- Assert(n_indexes == 1); // Currently only support one type of block filter index
- size_t max_cache = std::min(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE);
+ size_t pct = std::min(n_indexes, size_t{2}) * 5;
+ size_t max_cache = std::min(total_cache * pct / 100, MAX_FILTER_INDEX_CACHE);
index_sizes.filter_index = max_cache / n_indexes;
total_cache -= index_sizes.filter_index * n_indexes;
}
