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

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:200613-fee changing 1 files +1 −1

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:

/** 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

Contributors

Labels

Linked (view graph)

#19254 static FeeFilterRounder in net_processing needs a mutex (or documentation that concurrent access is not allowed)

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

Conflicts