Skip to content

refactor: P2P transport without serialize version and type #28892

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 4 commits into from
Nov 28, 2023
Merged

refactor: P2P transport without serialize version and type #28892

merged 4 commits into from
Nov 28, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 16, 2023

Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.

This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2023 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ajtowns
Stale ACK sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
  • #27826 (validation: log which peer sent us a header by Sjors)
  • #26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
  • #21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
  • #10102 (Multiprocess bitcoin by ryanofsky)

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.

@DrahtBot DrahtBot changed the title refactor: P2P transport without serialize version and type refactor: P2P transport without serialize version and type Nov 16, 2023
This was referenced Nov 16, 2023
Merged
Merged
This was referenced Nov 16, 2023
Merged
Merged
Closed
ajtowns
ajtowns reviewed Nov 17, 2023
View reviewed changes
src/net_processing.cpp Outdated
@@ -1985,7 +1985,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr<const CBlock> &blo
void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock)
{
auto pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs>(*pblock);
const CNetMsgMaker msgMaker(PROTOCOL_VERSION);
const NetMsgMaker msgMaker{};
Copy link
Contributor

@ajtowns ajtowns Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why not const NetMsgMaker msgMaker; with no curly brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like {}, because it has many benefits:

  • No need for code readers to waste brain cycles on thinking whether an initialization was missed
  • A smaller --word-diff-regex=., when in the future a designated initializer or constructor argument is added
  • I looks nice

MarcoFalke added 2 commits November 17, 2023 14:38
refactor: VectorWriter without nVersion
fa0ed07 
The field is unused, so remove it.

This is also required for future commits.
Remove unused Make() overload in netmessagemaker.h
66669da
@maflcko maflcko force-pushed the 2311-p2p-no-nVersion- branch from fa96619 to fade05b Compare November 17, 2023 13:44
@sipa
Copy link
Member

sipa commented Nov 17, 2023

utACK fade05b

I'm also ok with a commit in this PR to drop NetMsgMaker entirely.

@ajtowns
Copy link
Contributor

ajtowns commented Nov 17, 2023

ACK fade05b

@fanquake fanquake requested a review from theuni November 17, 2023 15:42
@maflcko maflcko force-pushed the 2311-p2p-no-nVersion- branch 2 times, most recently from 3333fdf to fa3e1ff Compare November 17, 2023 16:11
@maflcko
Copy link
Member Author

maflcko commented Nov 17, 2023

Removed the class and replaced it with a template function in a namespace to address the feedback of both reviewers (Thanks!)

sipa
sipa reviewed Nov 17, 2023
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Nov 17, 2023
Open
@maflcko maflcko force-pushed the 2311-p2p-no-nVersion- branch from fa3e1ff to dddd999 Compare November 19, 2023 13:12
@DrahtBot DrahtBot mentioned this pull request Nov 19, 2023
Merged
ajtowns
ajtowns reviewed Nov 20, 2023
View reviewed changes
@ajtowns
refactor: NetMsg::Make() without nVersion
fa9b5f4 
The nVersion field is unused, so remove it.

This is also required for future commits.

Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.

Co-Authored-By: Anthony Towns <aj@erisian.com.au>
@maflcko maflcko force-pushed the 2311-p2p-no-nVersion- branch from dddd999 to fafd5e5 Compare November 20, 2023 13:06
ajtowns
ajtowns reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

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

ACK fafd5e5

@DrahtBot DrahtBot requested a review from sipa November 22, 2023 00:58
@DrahtBot DrahtBot mentioned this pull request Nov 22, 2023
Draft
@maflcko maflcko force-pushed the 2311-p2p-no-nVersion- branch from fafd5e5 to fa79a88 Compare November 23, 2023 12:44
@ajtowns
Copy link
Contributor

ajtowns commented Nov 27, 2023

reACK fa79a88

@fanquake fanquake merged commit c252a0f into bitcoin:master Nov 28, 2023
@maflcko maflcko deleted the 2311-p2p-no-nVersion- branch November 28, 2023 11:39
@bitcoin bitcoin locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@theuni theuni Awaiting requested review from theuni

@sipa sipa Awaiting requested review from sipa

1 more reviewer

@ajtowns ajtowns ajtowns left review comments

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

Successfully merging this pull request may close these issues.
6 participants
@maflcko @DrahtBot @sipa @ajtowns @theuni @fanquake