fees: make the class FeeFilterRounder thread-safe #24407

vasild · 2022-02-21T16:21:17Z

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.

vasild · 2022-02-21T16:23:40Z

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.

maflcko · 2022-02-22T08:57:00Z

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:

This pull makes a utility class thread safe, so that it may be called in a concurrent context without running into UB
The other pull aims to reduce the use of the 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.

jonatack

ACK ecb5e10

The first commit message should be fixed from "Co-authoread-by:" to "Co-authored-by:"

src/policy/fees.cpp

vasild · 2022-03-15T16:02:17Z

ecb5e10247...b2311fe154: address suggestions

Invalidates ACK from @jonatack

jonatack · 2022-03-15T23:56:59Z

ACK b2311fe

promag

Code review ACK b2311fe

src/policy/fees.cpp

vasild · 2022-04-15T07:04:26Z

b2311fe154...f69670bfc2: call rand32() only if necessary

Invalidates ACKs from @jonatack, @promag

jonatack

ACK f69670b

src/policy/fees.cpp

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 bitcoin#19268 (comment)

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

vasild · 2022-04-18T08:50:13Z

f69670bfc2...8173f160e0: rebase and address suggestion.

jonatack · 2022-04-18T09:45:56Z

re-ACK 8173f16

promag

Code review ACK 8173f16

laanwj · 2022-04-19T13:47:07Z

Code review ACK 8173f16

DrahtBot · 2022-09-23T12:59:40Z

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

Conflicts

No conflicts as of last run.

mzumsande · 2022-10-13T18:08:17Z

CI currently fails on master, probably in connection with this:

policy/fees.cpp:1042:10: error: calling function 'MaybeCheckNotHeld' requires negative capability '!m_insecure_rand_mutex' [-Werror,-Wthread-safety-analysis]
         WITH_LOCK(m_insecure_rand_mutex, return insecure_rand.rand32()) % 3 != 0)) {

(https://github.com/bitcoin/bitcoin/runs/8876014446)

cbb2da8 add lock annotation for FeeFilterRounder::round() (glozow) Pull request description: CI failure from #24407: https://github.com/bitcoin/bitcoin/runs/8876014446 Calling `WITH_LOCK()` on a non-recursive mutex requires not holding it beforehand. ACKs for top commit: achow101: ACK cbb2da8 dergoegge: ACK cbb2da8 hebasto: ACK cbb2da8, tested on Ubuntu 22.04 with clang 14.0. Tree-SHA512: d6782ee48442b9d64d58a54c1ec7c53822ab051bf9728b44d6a0e05f1953e90f16420d349379345845db203fbad4e1f5750d9070adcb7daa18f12359a29488ca

8173f16 style: rename variables to match coding style (Vasil Dimov) 8b4ad20 fees: make FeeFilterRounder::feeset const (Vasil Dimov) e7a5bf6 fees: make the class FeeFilterRounder thread-safe (Vasil Dimov) Pull request description: 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. ACKs for top commit: jonatack: re-ACK 8173f16 laanwj: Code review ACK 8173f16 promag: Code review ACK 8173f16 Tree-SHA512: 94b809997c485c0d114fa702d0406b980be8eaaebcfefa56808ed670aa943959c2f16cfd0ef72b4752fe2a409a23af1b4b7f2f236e51212957759569e3bbbefd

cbb2da8 add lock annotation for FeeFilterRounder::round() (glozow) Pull request description: CI failure from bitcoin#24407: https://github.com/bitcoin/bitcoin/runs/8876014446 Calling `WITH_LOCK()` on a non-recursive mutex requires not holding it beforehand. ACKs for top commit: achow101: ACK cbb2da8 dergoegge: ACK cbb2da8 hebasto: ACK cbb2da8, tested on Ubuntu 22.04 with clang 14.0. Tree-SHA512: d6782ee48442b9d64d58a54c1ec7c53822ab051bf9728b44d6a0e05f1953e90f16420d349379345845db203fbad4e1f5750d9070adcb7daa18f12359a29488ca

DrahtBot added the TX fees and policy label Feb 21, 2022

jonatack reviewed Mar 14, 2022

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

jonatack reviewed Mar 14, 2022

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

vasild force-pushed the FeeFilterRounder_thread_safe branch from ecb5e10 to b2311fe Compare March 15, 2022 16:02

promag approved these changes Apr 8, 2022

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

vasild force-pushed the FeeFilterRounder_thread_safe branch from b2311fe to f69670b Compare April 15, 2022 07:03

jonatack reviewed Apr 15, 2022

View reviewed changes

src/policy/fees.cpp Outdated Show resolved Hide resolved

vasild and others added 3 commits April 18, 2022 10:40

fees: make the class FeeFilterRounder thread-safe

e7a5bf6

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>

vasild force-pushed the FeeFilterRounder_thread_safe branch from f69670b to 8173f16 Compare April 18, 2022 08:49

promag approved these changes Apr 19, 2022

View reviewed changes

achow101 merged commit 0bac04b into bitcoin:master Oct 13, 2022

achow101 mentioned this pull request Oct 13, 2022

Verify PSBT inputs rather than check for fields being empty #25595

Merged

glozow mentioned this pull request Oct 13, 2022

add lock annotation for FeeFilterRounder::round() #26306

Merged

vasild deleted the FeeFilterRounder_thread_safe branch October 14, 2022 07:02

bitcoin locked and limited conversation to collaborators Oct 14, 2023

fees: make the class FeeFilterRounder thread-safe #24407

fees: make the class FeeFilterRounder thread-safe #24407

Uh oh!

Conversation

vasild commented Feb 21, 2022

Uh oh!

vasild commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 22, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vasild commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Mar 15, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Apr 15, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Apr 18, 2022

Uh oh!

jonatack commented Apr 18, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 19, 2022

Uh oh!

DrahtBot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

mzumsande commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

vasild commented Feb 21, 2022 •

edited

Loading

vasild commented Mar 15, 2022 •

edited

Loading

DrahtBot commented Sep 23, 2022 •

edited

Loading