From e7a5bf6be79e341e037305a4c2d8a1a510a8d709 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 21 Feb 2022 17:03:27 +0100 Subject: fees: 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()`). Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- src/policy/fees.cpp | 6 ++++-- src/policy/fees.h | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) (limited to 'src/policy') diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 6499dbd97f..9f576e738a 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -1010,8 +1010,10 @@ FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) CAmount FeeFilterRounder::round(CAmount currentMinFee) { std::set::iterator it = feeset.lower_bound(currentMinFee); - if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) { - it--; + if (it == feeset.end() || + (it != feeset.begin() && + WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32()) % 3 != 0)) { + --it; } return static_cast(*it); } diff --git a/src/policy/fees.h b/src/policy/fees.h index 6e25bb42b8..c7fa1a0b77 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -299,12 +299,13 @@ public: /** Create new FeeFilterRounder */ explicit FeeFilterRounder(const CFeeRate& minIncrementalFee); - /** Quantize a minimum fee for privacy purpose before broadcast. Not thread-safe due to use of FastRandomContext */ + /** Quantize a minimum fee for privacy purpose before broadcast. */ CAmount round(CAmount currentMinFee); private: std::set feeset; - FastRandomContext insecure_rand; + Mutex m_insecure_rand_mutex; + FastRandomContext insecure_rand GUARDED_BY(m_insecure_rand_mutex); }; #endif // BITCOIN_POLICY_FEES_H -- cgit v1.2.3 From 8b4ad203d06c5ded6ecebbd7277b29a442d88bcf Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 21 Feb 2022 17:11:59 +0100 Subject: fees: make FeeFilterRounder::feeset const 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 in https://github.com/bitcoin/bitcoin/pull/19268#discussion_r439929792 --- src/policy/fees.cpp | 17 ++++++++++++++--- src/policy/fees.h | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) (limited to 'src/policy') diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 9f576e738a..d64360e82d 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -998,13 +998,24 @@ void CBlockPolicyEstimator::FlushUnconfirmed() { LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, (endclear - startclear)*0.000001); } -FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) +static std::set MakeFeeSet(const CFeeRate& minIncrementalFee, + double max_filter_fee_rate, + double fee_filter_spacing) { - CAmount minFeeLimit = std::max(CAmount(1), minIncrementalFee.GetFeePerK() / 2); + std::set feeset; + + const CAmount minFeeLimit{std::max(CAmount(1), minIncrementalFee.GetFeePerK() / 2)}; feeset.insert(0); - for (double bucketBoundary = minFeeLimit; bucketBoundary <= MAX_FILTER_FEERATE; bucketBoundary *= FEE_FILTER_SPACING) { + for (double bucketBoundary = minFeeLimit; bucketBoundary <= max_filter_fee_rate; bucketBoundary *= fee_filter_spacing) { feeset.insert(bucketBoundary); } + + return feeset; +} + +FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) + : feeset{MakeFeeSet(minIncrementalFee, MAX_FILTER_FEERATE, FEE_FILTER_SPACING)} +{ } CAmount FeeFilterRounder::round(CAmount currentMinFee) diff --git a/src/policy/fees.h b/src/policy/fees.h index c7fa1a0b77..e7f45c3151 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -303,7 +303,7 @@ public: CAmount round(CAmount currentMinFee); private: - std::set feeset; + const std::set feeset; Mutex m_insecure_rand_mutex; FastRandomContext insecure_rand GUARDED_BY(m_insecure_rand_mutex); }; -- cgit v1.2.3 From 8173f160e085186c9bcc7f3506205c309ee66af6 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 15 Mar 2022 16:57:05 +0100 Subject: style: rename variables to match coding style 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 --- src/policy/fees.cpp | 25 ++++++++++++++----------- src/policy/fees.h | 4 ++-- 2 files changed, 16 insertions(+), 13 deletions(-) (limited to 'src/policy') diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index d64360e82d..44024ce7ac 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -998,31 +998,34 @@ void CBlockPolicyEstimator::FlushUnconfirmed() { LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, (endclear - startclear)*0.000001); } -static std::set MakeFeeSet(const CFeeRate& minIncrementalFee, +static std::set MakeFeeSet(const CFeeRate& min_incremental_fee, double max_filter_fee_rate, double fee_filter_spacing) { - std::set feeset; + std::set fee_set; - const CAmount minFeeLimit{std::max(CAmount(1), minIncrementalFee.GetFeePerK() / 2)}; - feeset.insert(0); - for (double bucketBoundary = minFeeLimit; bucketBoundary <= max_filter_fee_rate; bucketBoundary *= fee_filter_spacing) { - feeset.insert(bucketBoundary); + const CAmount min_fee_limit{std::max(CAmount(1), min_incremental_fee.GetFeePerK() / 2)}; + fee_set.insert(0); + for (double bucket_boundary = min_fee_limit; + bucket_boundary <= max_filter_fee_rate; + bucket_boundary *= fee_filter_spacing) { + + fee_set.insert(bucket_boundary); } - return feeset; + return fee_set; } FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) - : feeset{MakeFeeSet(minIncrementalFee, MAX_FILTER_FEERATE, FEE_FILTER_SPACING)} + : m_fee_set{MakeFeeSet(minIncrementalFee, MAX_FILTER_FEERATE, FEE_FILTER_SPACING)} { } CAmount FeeFilterRounder::round(CAmount currentMinFee) { - std::set::iterator it = feeset.lower_bound(currentMinFee); - if (it == feeset.end() || - (it != feeset.begin() && + std::set::iterator it = m_fee_set.lower_bound(currentMinFee); + if (it == m_fee_set.end() || + (it != m_fee_set.begin() && WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32()) % 3 != 0)) { --it; } diff --git a/src/policy/fees.h b/src/policy/fees.h index e7f45c3151..b44ba4dc7a 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -297,13 +297,13 @@ private: public: /** Create new FeeFilterRounder */ - explicit FeeFilterRounder(const CFeeRate& minIncrementalFee); + explicit FeeFilterRounder(const CFeeRate& min_incremental_fee); /** Quantize a minimum fee for privacy purpose before broadcast. */ CAmount round(CAmount currentMinFee); private: - const std::set feeset; + const std::set m_fee_set; Mutex m_insecure_rand_mutex; FastRandomContext insecure_rand GUARDED_BY(m_insecure_rand_mutex); }; -- cgit v1.2.3