Make the class FeeFilterRounder thread-safe so that its methods can be called concurrently by different threads on the same object. Currently it has just one method (round()).
The second commit is optional, but it improves readability, showing that the feeset member will never be changed, thus does not need protection from concurrent access.
This is revived from the original version of #19268 (cc @hebasto, @ajtowns).
This achieves the same purpose as #22053, but in a different way (cc @MarcoFalke).
My reasoning for opening this PR is that #19268 was reduced to just a comment because it is easier to just document the non-thread safe nature of FeeFilterRounder. But #22053 goes beyond that, trying to use it in a thread-safe way. I think it is best to make FeeFilterRounder thread-safe itself, so that it can be used without thread-safety-worries now and in the future if used in new places.
This achieves the same purpose as #22053, but in a different way (cc @MarcoFalke).
I don’t think the two pulls achieve the same goal. In fact I think they are completely orthogonal:
cs_main “global” consistency lock. The consistency lock is needed even in the absence of UB. Also, m_insecure_rand_mutex added here can’t possibly achieve what cs_main achieves right now in MaybeSendFeefilter.So I think the two pull requests are best reviewed and evaluated independently. Also, there is no dependency between them. Either can be merged before the other, or not at all.
1002+static std::set<double> MakeFeeSet(const CFeeRate& min_incremental_fee,
1003+ double max_filter_feerate,
1004+ double fee_filter_spacing)
1005 {
1006- CAmount minFeeLimit = std::max(CAmount(1), minIncrementalFee.GetFeePerK() / 2);
1007+ std::set<double> feeset;
feeset to fee_set in MakeFeeSet() (optionally the class data member too) so that they are all per current style in that function.
ACK ecb5e10247a80356e1d22c2970e8c9b8fabff486
The first commit message should be fixed from “Co-authoread-by:” to “Co-authored-by:”
1004+ double fee_filter_spacing)
1005 {
1006- CAmount minFeeLimit = std::max(CAmount(1), minIncrementalFee.GetFeePerK() / 2);
1007+ std::set<double> feeset;
1008+
1009+ CAmount min_fee_limit = std::max(CAmount(1), min_incremental_fee.GetFeePerK() / 2);
ecb5e10247a80356e1d22c2970e8c9b8fabff486 if you retouch
0 const CAmount min_fee_limit{std::max(CAmount(1), min_incremental_fee.GetFeePerK() / 2)};
1009@@ -1010,7 +1010,8 @@ FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee)
1010 CAmount FeeFilterRounder::round(CAmount currentMinFee)
1011 {
1012 std::set<double>::iterator it = feeset.lower_bound(currentMinFee);
1013- if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) {
1014+ const auto rand = WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32());
adfd07564d5544f327d7432faddcf9612a6a22b3
Just noting that now now rand32() is always called.
Could have
0 bool PreferLower() { LOCK(m_insecure_rand_mutex); return insecure_rand.rand32() % 3 != 0; }
1
2 if (it != m_fee_set.begin() && PreferLower() || it == m_fee_set.end()) --it;
1034+ std::set<double>::iterator it = m_fee_set.lower_bound(currentMinFee);
1035+ const auto PreferLower = [this]() -> bool {
1036+ LOCK(m_insecure_rand_mutex);
1037+ return insecure_rand.rand32() % 3 != 0;
1038+ };
1039+ if (it == m_fee_set.end() || (it != m_fee_set.begin() && PreferLower())) {
(This alternative is more straightforward to my eye. Feel free to ignore.)
0- const auto PreferLower = [this]() -> bool {
1- LOCK(m_insecure_rand_mutex);
2- return insecure_rand.rand32() % 3 != 0;
3- };
4- if (it == m_fee_set.end() || (it != m_fee_set.begin() && PreferLower())) {
5+ if (it == m_fee_set.end() || (it != m_fee_set.begin() && WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32()) % 3) {
if line easier to read IMO – the github comment already requires scrolling) shrug
Alright, dropped the function and used WITH_LOCK(). I wrapped the line coz long lines arehardtoreadandrequirescrolling in someenvironments (e.g.github) and also when a single piece of it is changed, then the whole line appears as changed in diffs - see the diff at the bottom of #15936 (review) for example.
In other words, I agree it is better to be purple, but I like the blueish tint.
Making review more difficult on the platform of a trusted third party could be considered a feature :smiling_imp:
(I find one line more readable but not a deal breaker, thanks for updating.)
So that its methods can be called concurrently by different threads on
the same object. Currently it has just one method (`round()`).
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
It is only set in the constructor, thus improve readability by marking
it as `const` and setting it from the initializer list using a helper
function to derive its value.
The idea was suggested by Anthony Towns <aj@erisian.com.au> in
https://github.com/bitcoin/bitcoin/pull/19268#discussion_r439929792
Rename the variables that were touched by the previous commit (split
logical from style changes).
minIncrementalFee -> min_incremental_fee
minFeeLimit -> min_fee_limit
bucketBoundary -> bucket_boundary
feeset -> fee_set
FeeFilterRounder::feeset -> FeeFilterRounder::m_fee_set
f69670bfc2...8173f160e0: rebase and address suggestion.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
CI currently fails on master, probably in connection with this:
0policy/fees.cpp:1042:10: error: calling function 'MaybeCheckNotHeld' requires negative capability '!m_insecure_rand_mutex' [-Werror,-Wthread-safety-analysis]
1 WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32()) % 3 != 0)) {