fuzz: Rework FillNode #23575

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2111-fuzzFillNode changing 4 files +50 −16

MarcoFalke commented at 5:00 PM on November 22, 2021: member

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 (review) .

Fix all issues by sending a version/verack in FillNode and let net_processing figure out the internal details.
DrahtBot added the label Tests on Nov 22, 2021
DrahtBot commented at 6:36 AM on November 23, 2021: member


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.

MarcoFalke commented at 10:18 AM on November 23, 2021: member

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)

theStack commented at 3:08 PM on November 23, 2021: member

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.
MarcoFalke force-pushed on Nov 24, 2021
MarcoFalke commented at 7:09 PM on November 24, 2021: member

Thanks, done.
DrahtBot added the label Needs rebase on Dec 1, 2021
fuzz: Avoid negative NodeId in ConsumeNode fa3583f856

refactor: Set fSuccessfullyConnected in FillNode

Also, pass ConnmanTestMsg& and PeerManager& (needed for later commits).

fae6e31df7

fuzz: Rework FillNode fa19bab90a
MarcoFalke force-pushed on Dec 1, 2021
DrahtBot removed the label Needs rebase on Dec 1, 2021
MarcoFalke commented at 4:07 PM on December 6, 2021: member

cc @jnewbery
jnewbery commented at 11:10 AM on December 14, 2021: member

Strong concept ACK and light code review ACK fa19bab90a3ccc2f76c20aa805292d6a9c5d8071
fanquake merged this on Dec 14, 2021
fanquake closed this on Dec 14, 2021
MarcoFalke deleted the branch on Dec 14, 2021
sidhujag referenced this in commit 69bb346e4e on Dec 14, 2021
DrahtBot locked this on Dec 14, 2022

Contributors

Labels

Linked (view graph)

#21160 net/net processing: Move tx inventory into net_processing

fuzz: Rework FillNode #23575

Conflicts