refactor: Mark CAddrMan::Select and GetAddr const by maflcko · Pull Request #21940 · bitcoin/bitcoin

maflcko · 2021-05-13T09:26:44Z

To clarify that a call to this only changes the random state and nothing else.

DrahtBot · 2021-05-13T15:24:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Assert in CAddrMan::Check() #22500 by MarcoFalke
Check that CAddrMan::nKey is not null after deserialize #22498 by MarcoFalke
fuzz: check that ser+unser produces the same AddrMan #21129 by vasild
addrman: Make consistency checks a runtime option #20233 by jnewbery

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag

ACK fa8e3f78751d80fa8bce957387bfc95802af2477.

Built locally, also tried to avoid mutable members, but all things considered, this approach seems preferable.

src/addrman.cpp

theStack

Code-review ACK fa8e3f78751d80fa8bce957387bfc95802af2477

maflcko · 2021-05-22T07:39:37Z

Rebased

theStack

re-ACK fa845812d94ec6adc13459dca708b0850a3059ca 🦉
Checked that changes since my last ACK are only rebase-related via git range-diff fa8e3f78...fa845812

theStack

re-re-ACK fa49ab3e4075f8402e30507091033a621a6941ac ⚓

Checked that changes since my last re-ACK are only rebase-related (caused by 5cd7f8a and 8caf60d) via git range-diff fa845812...fa49ab3

…only once faf7623 fuzz: Call const member functions in addrman fuzz test only once (MarcoFalke) Pull request description: Logically based on #21940 Currently the fuzz test may spend a long time generating random numbers: ![Screenshot from 2021-05-13 12-14-09](https://user-images.githubusercontent.com/6399679/118112238-06ecd880-b3e5-11eb-8013-6e0c20e6159f.png) Fix that by calling const member functions only once. Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34224 ACKs for top commit: practicalswift: cr ACK faf7623: touches only `src/test/fuzz/addrman.cpp` Tree-SHA512: 0fe9e0111eb1706fc39bd2f90d4b87a771882bada54c01e96d8e79c2afca2f1081139d5ab680285a81835cc5142e74ada422a181db34b01904975d1e167e64c2

maflcko · 2021-06-14T07:25:47Z

Rebased and added one commit

lsilva01

Code Review ACK and Tested ACK fa47bdd on Ubuntu 20.04.

theStack

re-ACK fa47bdddbc7fc153c36f8f700099656ec1ce9d5d

Checked again that changes since my last re-ACK are only rebase-related via git range-diff fa49ab3...fa47bddd (plus the fuzzing related extra commit)

maflcko · 2021-07-21T14:03:59Z

Force pushed to add comment

maflcko · 2021-07-21T14:05:33Z

Can GetAddr() also be made const?

It already is. (Adjusted title)

sipa · 2021-07-22T18:08:53Z

Concept ACK. Perhaps this deserves a comment in addrman.h though, to state that even const member functions cannot be used without synchronization?

maflcko · 2021-07-22T18:41:49Z

Added a lock annotation to the mutable member, which can be "read" by compilers and developers

src/addrman.cpp

src/addrman.h

Leaving it as-is would be annoying because some editor fix-up the spacing when opening a file or editing it.

jnewbery · 2021-07-23T10:05:45Z

Code review ACK fae108c

maflcko · 2021-07-29T11:06:46Z

@promag or @theStack interested in donating a re-ACK?

theStack

re-ACK fae108c 🍦

Checked via git range-diff fa47bddd...fae108ce that there were only minor changes (comment on vRandom, as suggested by jnewbery) and rebase on master since my last ACK

jonatack · 2021-08-02T11:07:48Z

Did any of the reviewers test this change with DEBUG_ADDRMAN defined? It blows up for me.

addrman.cpp:467:15: error: out-of-line definition of 'Check_' does not match any declaration in 'CAddrMan'
int CAddrMan::Check_()
              ^~~~~~
./addrman.h:746:9: note: member declaration does not match because it is const qualified
    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
        ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

maflcko · 2021-08-02T11:11:17Z

@jonatack Good catch. I didn't test this with the CPP flag set and CI doesn't either.

Luckily the code will be always compiled after #20233, so shouldn't happen again after that pull is merged.

jnewbery · 2021-08-02T11:44:41Z

Thanks @jonatack. I'm currently rebasing #20233 and will include a fix for this issue in that PR.

jonatack · 2021-08-02T11:51:54Z

Thanks @jonatack. I'm currently rebasing #20233 and will include a fix for this issue in that PR.

Thanks!

maflcko · 2021-08-02T12:29:01Z

I created a fix in #22601 , because I didn't see the comment here.

jonatack · 2021-08-02T12:34:04Z

Thanks, in the meantime will cherry-pick this commit locally.

theStack · 2021-08-02T16:10:56Z

Did any of the reviewers test this change with DEBUG_ADDRMAN defined? It blows up for me.

Oops, no, I didn't :( thanks for catching this.

Would it make sense to have a CI instance with DEBUG_ADDRMAN defined or is that overambitious? 🤔

jnewbery · 2021-08-02T16:49:32Z

@theStack please see #20233 which removes the DEBUG_ADDRMAN build option and makes consistency checks a runtime option.

fae108c Fix incorrect whitespace in addrman (MarcoFalke) fa32024 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke) fab755b fuzz: Actually use const addrman (MarcoFalke) fae0c79 refactor: Mark CAddrMan::GetAddr const (MarcoFalke) fa02934 refactor: Mark CAddrMan::Select const (MarcoFalke) Pull request description: To clarify that a call to this only changes the random state and nothing else. ACKs for top commit: jnewbery: Code review ACK fae108c theStack: re-ACK fae108c 🍦 Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a

maflcko mentioned this pull request May 13, 2021

fuzz: Call const member functions in addrman fuzz test only once #21941

Merged

DrahtBot added the Refactoring label May 13, 2021

This was referenced May 13, 2021

p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network #21843

Merged

addrman: Make consistency checks a runtime option #20233

Closed

promag reviewed May 14, 2021

View reviewed changes

jonatack reviewed May 14, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

theStack approved these changes May 16, 2021

View reviewed changes

DrahtBot added the Needs rebase label May 20, 2021

maflcko force-pushed the 2105-addrmanConstSelect branch from fa8e3f7 to fa84581 Compare May 22, 2021 07:38

DrahtBot removed the Needs rebase label May 22, 2021

This was referenced May 23, 2021

refactor: Group and re-order CAddrMan members by access type #22025

Merged

refactor: Make CAddrMan::cs non-recursive #19238

Merged

theStack approved these changes May 24, 2021

View reviewed changes

DrahtBot added the Needs rebase label May 27, 2021

maflcko force-pushed the 2105-addrmanConstSelect branch from fa84581 to fa49ab3 Compare May 31, 2021 11:59

DrahtBot removed the Needs rebase label May 31, 2021

theStack approved these changes May 31, 2021

View reviewed changes

maflcko force-pushed the 2105-addrmanConstSelect branch from fa49ab3 to fa35236 Compare June 14, 2021 07:25

DrahtBot added the Needs rebase label Jun 14, 2021

maflcko force-pushed the 2105-addrmanConstSelect branch from fa35236 to faecfad Compare June 14, 2021 15:59

DrahtBot removed the Needs rebase label Jun 14, 2021

DrahtBot mentioned this pull request Jul 19, 2021

Assert in CAddrMan::Check() #22500

Closed

maflcko force-pushed the 2105-addrmanConstSelect branch from faecfad to fa47bdd Compare July 19, 2021 19:39

lsilva01 approved these changes Jul 19, 2021

View reviewed changes

theStack approved these changes Jul 20, 2021

View reviewed changes

fanquake requested a review from jnewbery July 20, 2021 02:56

MarcoFalke added 2 commits July 21, 2021 16:01

refactor: Mark CAddrMan::GetAddr const

fae0c79

fuzz: Actually use const addrman

fab755b

maflcko force-pushed the 2105-addrmanConstSelect branch from fa47bdd to fab755b Compare July 21, 2021 14:02

maflcko changed the title ~~refactor: Mark CAddrMan::Select const~~ refactor: Mark CAddrMan::Select and GetAddr const Jul 21, 2021

maflcko force-pushed the 2105-addrmanConstSelect branch from a04e835 to 5555a1d Compare July 22, 2021 18:39

DrahtBot mentioned this pull request Jul 22, 2021

Check that CAddrMan::nKey is not null after deserialize #22498

Closed

jnewbery reviewed Jul 23, 2021

View reviewed changes

src/addrman.cpp Outdated Show resolved Hide resolved

src/addrman.h Outdated Show resolved Hide resolved

MarcoFalke added 2 commits July 23, 2021 11:31

Add missing GUARDED_BY to CAddrMan::insecure_rand

fa32024

Fix incorrect whitespace in addrman

fae108c

Leaving it as-is would be annoying because some editor fix-up the spacing when opening a file or editing it.

maflcko force-pushed the 2105-addrmanConstSelect branch from 5555a1d to fae108c Compare July 23, 2021 09:31

DrahtBot mentioned this pull request Jul 27, 2021

fuzz: check that ser+unser produces the same AddrMan #21129

Merged

theStack approved these changes Jul 29, 2021

View reviewed changes

maflcko merged commit 2f60d9f into bitcoin:master Aug 2, 2021

maflcko deleted the 2105-addrmanConstSelect branch August 2, 2021 10:15

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

maflcko commented May 13, 2021

Uh oh!

DrahtBot commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 22, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 14, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 21, 2021

Uh oh!

maflcko commented Jul 21, 2021

Uh oh!

sipa commented Jul 22, 2021

Uh oh!

maflcko commented Jul 22, 2021

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Jul 23, 2021

Uh oh!

maflcko commented Jul 29, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 2, 2021

Uh oh!

jnewbery commented Aug 2, 2021

Uh oh!

jonatack commented Aug 2, 2021

Uh oh!

maflcko commented Aug 2, 2021

Uh oh!

jonatack commented Aug 2, 2021

Uh oh!

theStack commented Aug 2, 2021

Uh oh!

jnewbery commented Aug 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented May 13, 2021 •

edited

Loading

jonatack commented Aug 2, 2021 •

edited

Loading