Skip to content

p2p: Rename fBlocksOnly, Add test #22955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 16, 2021
Merged

p2p: Rename fBlocksOnly, Add test #22955

merged 2 commits into from
Sep 16, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 12, 2021

fBlocksOnly has several issues:

  • The name is confusing
  • It is untested

Fix both.

@fanquake fanquake added the P2P label Sep 12, 2021
@maflcko
Copy link
Member Author

maflcko commented Sep 12, 2021

Fun fact: This is covered by the fuzz tests, but obviously they don't check the logic level of the code. https://marcofalke.github.io/btc_cov/fuzz.coverage/src/net_processing.cpp.gcov.html

Functional coverage: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html

@maflcko
Copy link
Member Author

maflcko commented Sep 12, 2021

There are some other issues with this code, which will be fixed in follow-ups, because they change the P2P protocol.

p2p: Rename fBlocksOnly, Add test
fa66a7d 
The new name describes better what the bool does and also limits the confusion of the three different concepts:
* fBlocksOnly (This bool to skip tx invs)
* -blocksonly (A setting to ignore incoming txs)
* block-relay-only (A connection type in the block-relay-only P2P graph)
@maflcko maflcko force-pushed the 2109-testInvBlocksonly branch from fa59cf6 to fa66a7d Compare September 12, 2021 10:54
@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22850 (net: Make m_mempool optional in PeerManager by sriramdvt)
  • #21954 (Some minor code changes to improve efficiency of processing TXs by rebroad)

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.

This was referenced Sep 12, 2021
Closed
Closed
naumenkogs
naumenkogs reviewed Sep 13, 2021
View reviewed changes
vincenzopalazzo
vincenzopalazzo approved these changes Sep 13, 2021
View reviewed changes
Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack fac66d0

@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

Code review ACK fa66a7d

@laanwj laanwj merged commit 58e0239 into bitcoin:master Sep 16, 2021
@maflcko maflcko deleted the 2109-testInvBlocksonly branch September 16, 2021 14:49
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
@laanwj
Merge bitcoin#22955: p2p: Rename fBlocksOnly, Add test
de79e58 
fa66a7d p2p: Rename fBlocksOnly, Add test (MarcoFalke)
fac66d0 test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method (MarcoFalke)

Pull request description:

  `fBlocksOnly` has several issues:
  * The name is confusing
  * It is untested

  Fix both.

ACKs for top commit:
  laanwj:
    Code review ACK fa66a7d

Tree-SHA512: 4218f455eeb37297f74603d7d44895288605844ae828a40dfb7a70215f1a058ac5ad945a22732f5ebcad3ad375d54ba360bea69ea79639a30d4c88b042448f0f
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@naumenkogs naumenkogs naumenkogs left review comments

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maflcko @DrahtBot @laanwj @naumenkogs @vincenzopalazzo @fanquake