doc: Add non-thread-safe note to FeeFilterRounder::round()

hebasto commented at 3:10 pm on June 13, 2020: member

The FastRandomContext class is documented as not thread-safe. This PR adds a relevant note to the FeeFilterRounder::round() function declaration.

Close #19254

DrahtBot added the label P2P on Jun 13, 2020

DrahtBot added the label Refactoring on Jun 13, 2020

DrahtBot added the label TX fees and policy on Jun 13, 2020

in src/policy/fees.h:283 in 877cca538b outdated

280@@ -281,11 +281,12 @@ class FeeFilterRounder
281     explicit FeeFilterRounder(const CFeeRate& minIncrementalFee);
282 
283     /** Quantize a minimum fee for privacy purpose before broadcast **/

MarcoFalke commented at 3:53 pm on June 13, 2020:

0    /** Quantize a minimum fee for privacy purpose before broadcast . Not thread-safe due to use of FastRandomContext **/

sipa commented at 3:57 pm on June 13, 2020:

If this is not going to be changed, it’s probably better to make it an explicit global.

MarcoFalke commented at 4:05 pm on June 13, 2020:

Oh, I am not suggestion to never change this. This can be changed when net_processing is multi-threaded. Until then documentation and sanitizers are good enough to leave the code as is for now.

ajtowns commented at 5:17 am on June 15, 2020:

Isn’t it as thread safe as a class A { int a = 0; public: int increment() { return ++a; } }, which we’d solve by saying static A my_a GUARDED_BY(cs_main); ? And both could be made parallelisable by saying static thread_local A my_a; – it’s just that GUARDED_BY doesn’t work with block-scope statics?

Just adding // protected by cs_main at the declaration of filterRounder, or moving it to be a module-level static and adding a GUARDED_BY(cs_main) seems better to me?

hebasto commented at 11:01 am on July 5, 2020:

Updated.

MarcoFalke commented at 3:55 pm on June 13, 2020: member

My preference would be to just document it. Unless this is the only random context without a lock held in the net_processing loop.

As soon as the net_processing loop is run in parallel, our sanitizers should point out any unsafe code, so there should be no risk in leaving the code as-is.

MarcoFalke removed the label TX fees and policy on Jun 13, 2020

DrahtBot commented at 4:19 am on June 14, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

naumenkogs commented at 6:30 am on June 14, 2020: member

Concept ACK.

I don’t have a strong opinion on taking either approach (mutex vs documentation-only)

in src/policy/fees.h:289 in 877cca538b outdated

286 
287 private:
288     std::set<double> feeset;
289-    FastRandomContext insecure_rand;
290+    mutable Mutex m_random_context_mutex;
291+    mutable FastRandomContext m_insecure_rand GUARDED_BY(m_random_context_mutex);

ajtowns commented at 4:54 am on June 15, 2020:

I think it’d make more sense to mark feeset as const, than marking the whole object as const, and the parts that change as mutable (given const X x;, then a = x.foo(); b = x.foo(); a == b ought to be true imo).

Adding a static std::set<double> make_feeset(const CFeeRate&) member function and using that to initialise feeset via the initializer list looks like it’s enough?

in src/net_processing.cpp:4398 in 877cca538b outdated

4395@@ -4396,7 +4396,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4396             int64_t timeNow = GetTimeMicros();
4397             if (timeNow > pto->m_tx_relay->nextSendTimeFeeFilter) {
4398                 static CFeeRate default_feerate(DEFAULT_MIN_RELAY_TX_FEE);

ajtowns commented at 5:19 am on June 15, 2020:

Can make default_feerate const too – could make it constexpr even, so long as the constructors were also marked constexpr.

DrahtBot added the label Needs rebase on Jun 29, 2020

doc: Add non-thread-safe note to FeeFilterRounder::round()

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

d842e6ac96

hebasto force-pushed on Jul 5, 2020

hebasto commented at 10:57 am on July 5, 2020: member

Updated 877cca538b29bbb7420601e7972fb5635523f507 -> d842e6ac965b528f0d704f54aceb91eae84085fb (pr19268.01 -> pr19268.02):

rebased
implemented @MarcoFalke’s suggestion

hebasto renamed this:
~~refactor: Make FeeFilterRounder thread safe~~
doc: Add non-thread-safe note to FeeFilterRounder::round()
on Jul 5, 2020

MarcoFalke commented at 11:30 am on July 5, 2020: member

self ACK d842e6ac965b528f0d704f54aceb91eae84085fb

DrahtBot removed the label Needs rebase on Jul 5, 2020

practicalswift commented at 8:59 pm on July 10, 2020: contributor

ACK d842e6ac965b528f0d704f54aceb91eae84085fb: explicit is better than implicit

naumenkogs commented at 7:24 am on July 14, 2020: member

ACK d842e6a

MarcoFalke merged this on Jul 14, 2020

MarcoFalke closed this on Jul 14, 2020

hebasto deleted the branch on Jul 14, 2020

sidhujag referenced this in commit 9081bb4469 on Jul 14, 2020

Fabcien referenced this in commit 5492420267 on Aug 31, 2021

DrahtBot locked this on Feb 15, 2022

doc: Add non-thread-safe note to FeeFilterRounder::round() #19268

Conflicts