10000 fuzz: Rework FillNode by maflcko · Pull Request #23575 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fuzz: Rework FillNode #23575

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 3 commits into from
Dec 14, 2021
Merged

fuzz: Rework FillNode #23575

merged 3 commits into from
Dec 14, 2021

Conversation

maflcko
Copy link
Member
@maflcko maflcko commented Nov 22, 2021 •
edited
Loading

Currently FillNode is a bit clumsy because it directly modifies memory of CNode. This gets in the way of moving that memory to Peer. Also, it isn't particularly consistent. See for example #21160 (comment) .

Fix all issues by sending a version/verack in FillNode and let net_processing figure out the internal details.

@maflcko maflcko mentioned this pull request Nov 22, 2021
Merged
@DrahtBot DrahtBot added the Tests label Nov 22, 2021
@DrahtBot
Copy link
Contributor
DrahtBot commented Nov 23, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23604 (Use Sock in CNode by vasild)
  • #21878 (Make all networking code mockable by vasild)

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 mentioned this pull request Nov 23, 2021
Merged
@maflcko
Copy link
Member Author
maflcko commented Nov 23, 2021

Pushed a commit to avoid newly uncovered error:

txrequest.cpp:123:95: runtime error: implicit conversion from type 'NodeId' (aka 'long') of value -1700107756478988831 (64-bit, signed) to type 'uint64_t' (aka 'unsigned long') changed the value to 16746636317230562785 (64-bit, unsigned)

This was referenced Nov 23, 2021
Merged
Closed
theStack
theStack reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor
@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK

Would it make sense to use CNetMsgMaker(...)::Make(..., ...) helper here for crafting the version message? Also, two yocto-nits: could replace magic strings "version" and "verack" by NetMsgType::VERSION / NetMsgType::VERACK.

@maflcko
Copy link
Member Author
maflcko commented Nov 24, 2021

Thanks, done.

@DrahtBot DrahtBot mentioned this pull request Nov 27, 2021
Merged
MarcoFalke added 3 commits December 1, 2021 12:14
@maflcko
Copy link
Member Author
maflcko commented Dec 6, 2021

cc @jnewbery

@jnewbery
Copy link
Contributor

Strong concept ACK and light code review ACK fa19bab

@fanquake fanquake merged commit 498fe4b into bitcoin:master Dec 14, 2021
@maflcko maflcko deleted the 2111-fuzzFillNode branch December 14, 2021 13:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 14, 2021
@fanquake
Merge bitcoin#23575: fuzz: Rework FillNode
69bb346
@bitcoin bitcoin locked and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@theStack theStack theStack left review comments

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @jnewbery @theStack @fanquake
0