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

hebasto · 2020-06-13T15:10:07Z

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

Close #19254

maflcko

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.

maflcko · 2020-06-13T15:53:36Z

src/policy/fees.h

    /** Create new FeeFilterRounder */
    explicit FeeFilterRounder(const CFeeRate& minIncrementalFee);

    /** Quantize a minimum fee for privacy purpose before broadcast **/


Suggested change

/** Quantize a minimum fee for privacy purpose before broadcast **/

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

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

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.

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?

DrahtBot · 2020-06-14T04:19:45Z

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

Conflicts

No conflicts as of last run.

naumenkogs · 2020-06-14T06:30:12Z

Concept ACK.

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

ajtowns · 2020-06-15T04:54:43Z

src/policy/fees.h

    std::set<double> feeset;
-    FastRandomContext insecure_rand;
+    mutable Mutex m_random_context_mutex;
+    mutable FastRandomContext m_insecure_rand GUARDED_BY(m_random_context_mutex);


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?

ajtowns · 2020-06-15T05:17:05Z

src/policy/fees.h

    /** Create new FeeFilterRounder */
    explicit FeeFilterRounder(const CFeeRate& minIncrementalFee);

    /** Quantize a minimum fee for privacy purpose before broadcast **/


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?

ajtowns · 2020-06-15T05:19:53Z

src/net_processing.cpp

            CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
            int64_t timeNow = GetTimeMicros();
            if (timeNow > pto->m_tx_relay->nextSendTimeFeeFilter) {
                static CFeeRate default_feerate(DEFAULT_MIN_RELAY_TX_FEE);


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

Co-authored-by: MarcoFalke <[email protected]>

hebasto · 2020-07-05T10:57:02Z

Updated 877cca5 -> d842e6a (pr19268.01 -> pr19268.02):

rebased
implemented @MarcoFalke's suggestion

maflcko · 2020-07-05T11:30:18Z

self ACK d842e6a

practicalswift · 2020-07-10T20:59:16Z

ACK d842e6a: explicit is better than implicit

naumenkogs · 2020-07-14T07:24:43Z

ACK d842e6a

…r::round() d842e6a doc: Add non-thread-safe note to FeeFilterRounder::round() (Hennadii Stepanov) Pull request description: The `FastRandomContext` class is documented as not thread-safe. This PR adds a relevant note to the `FeeFilterRounder::round()` function declaration. Close bitcoin#19254 ACKs for top commit: MarcoFalke: self ACK d842e6a practicalswift: ACK d842e6a: explicit is better than implicit naumenkogs: ACK d842e6a Tree-SHA512: 538508f24b9cb29baece6a64108e2c5fc3960768c6475c4f2baf48a4a7bdb96dcef1a74d21a4822e1f8635e1375362986da4e3a20f5644129046a354c4b0a8a0

Summary: Co-authored-by: MarcoFalke <[email protected]> This is a backport of [[bitcoin/bitcoin#19268 | core#19268]] Test Plan: Proof-reading, check that our implementations of `FeeFilterRounder::round` are similar enough that the comment applies. Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9995

hebasto mentioned this pull request Jun 13, 2020

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

Closed

DrahtBot added P2P Refactoring TX fees and policy labels Jun 13, 2020

maflcko reviewed Jun 13, 2020

View reviewed changes

maflcko removed the TX fees and policy label Jun 13, 2020

DrahtBot mentioned this pull request Jun 14, 2020

p2p: Reduce inv traffic during IBD #19204

Merged

ajtowns reviewed Jun 15, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jun 29, 2020

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

d842e6a

Co-authored-by: MarcoFalke <[email protected]>

hebasto force-pushed the 200613-fee branch from 877cca5 to d842e6a Compare July 5, 2020 10:52

hebasto changed the title ~~refactor: Make FeeFilterRounder thread safe~~ doc: Add non-thread-safe note to FeeFilterRounder::round() Jul 5, 2020

DrahtBot removed the Needs rebase label Jul 5, 2020

maflcko merged commit 07c83ce into bitcoin:master Jul 14, 2020

hebasto deleted the 200613-fee branch July 14, 2020 07:34

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

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

Uh oh!

Conversation

hebasto commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Jun 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 5, 2020

Uh oh!

maflcko commented Jul 5, 2020

Uh oh!

practicalswift commented Jul 10, 2020

Uh oh!

naumenkogs commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Jun 13, 2020 •

edited

Loading

DrahtBot commented Jun 14, 2020 •

edited

Loading