Page MenuHomePhabricator
Differential D17131

net: use Sock in CNode
ClosedPublic
Actions

Authored by PiRK on Nov 13 2024, 17:40.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC48e3344f9f94: net: use Sock in CNode
Summary

Change CNode to use a pointer to Sock instead of a bare SOCKET.
This will help mocking / testing / fuzzing more code.

Rename CNode::cs_hSocket to CNode::m_sock_mutex

This is a partial backport of core#23604
https://github.com/bitcoin/bitcoin/pull/23604/commits/c5dd72e146dd8fa77d29c8689a42322a4d1ec780
https://github.com/bitcoin/bitcoin/pull/23604/commits/c41a1162ac4da437c5d755e8fe2bf636bed22b0f
https://github.com/bitcoin/bitcoin/pull/23604/commits/b68349164827f14c472201cad54c4e19a3321261

We also need to split the FuzzedSock interface and implementation (https://github.com/bitcoin/bitcoin/pull/21630/commits/29ae1c13a59187119f5b2a38b54dbbec936d8f87) to "make move FuzzedSock earlier in src/test/fuzz/util.h" work.

Depends on D17130

Test Plan

ninja all check-all

Diff Detail

Event Timeline

PiRK created this revision.Nov 13 2024, 17:40
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 13 2024, 17:40
PiRK requested review of this revision.Nov 13 2024, 17:40
teamcity edited the summary of this revision. (Show Details)Nov 13 2024, 17:40
PiRK added a child revision: D17132: net: also wait for exceptional events in Sock::Wait().Nov 13 2024, 17:48
Harbormaster completed remote builds in B31126: Diff 50840.Nov 13 2024, 18:04
teamcity added a comment.Nov 13 2024, 18:04
Build Bitcoin ABC Diffs / Diff Testing (build-fuzzer) failed.

Tail of the build log:

[389/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o
FAILED: src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-fuzzer/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-fuzzer/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIE -fvisibility=hidden -fsanitize=fuzzer -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o -MF src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o.d -o src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o -c /work/src/test/fuzz/strprintf.cpp
In file included from /work/src/test/fuzz/strprintf.cpp:10:
/work/src/./test/fuzz/util.h:316:40: error: use of undeclared identifier 'FuzzedSock'
    const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
                                       ^
1 error generated.
[390/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[391/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o
FAILED: src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-fuzzer/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-fuzzer/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIE -fvisibility=hidden -fsanitize=fuzzer -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o -MF src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o.d -o src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o -c /work/src/test/fuzz/system.cpp
In file included from /work/src/test/fuzz/system.cpp:9:
/work/src/./test/fuzz/util.h:316:40: error: use of undeclared identifier 'FuzzedSock'
    const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
                                       ^
1 error generated.
[392/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[393/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[394/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o
FAILED: src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-fuzzer/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-fuzzer/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIE -fvisibility=hidden -fsanitize=fuzzer -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o -MF src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o.d -o src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o -c /work/src/test/fuzz/timedata.cpp
In file included from /work/src/test/fuzz/timedata.cpp:7:
/work/src/./test/fuzz/util.h:316:40: error: use of undeclared identifier 'FuzzedSock'
    const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
                                       ^
1 error generated.
[395/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/transaction.cpp.o
[396/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[397/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o
FAILED: src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I/work/src/. -I/work/abc-ci-builds/build-fuzzer/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-fuzzer/src/crypto/.. -I/work/src/secp256k1/include -I/work/src/leveldb/include -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIE -fvisibility=hidden -fsanitize=fuzzer -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wformat-security -Wredundant-move -Woverloaded-virtual -Wshadow -Wshadow-field -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-psabi -std=gnu++17 -MD -MT src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o -MF src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o.d -o src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o -c /work/src/test/fuzz/validation_load_mempool.cpp
In file included from /work/src/test/fuzz/validation_load_mempool.cpp:13:
/work/src/./test/fuzz/util.h:316:40: error: use of undeclared identifier 'FuzzedSock'
    const auto sock = std::make_shared<FuzzedSock>(fuzzed_data_provider);
                                       ^
1 error generated.
[398/419] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txrequest.cpp.o
[399/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[400/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[401/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[402/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[403/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[404/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[405/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[406/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[407/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[408/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[409/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[410/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[411/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[412/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[413/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[414/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[415/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[416/419] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[417/419] Linking CXX static library src/wallet/libwallet.a
[418/419] Linking CXX static library src/libserver.a
ninja: build stopped: cannot make progress due to previous errors.
Build build-fuzzer failed with exit code 1
PiRK planned changes to this revision.Nov 13 2024, 21:29

needs a fuzzer fix and test plan

PiRK updated this revision to Diff 50856.Nov 14 2024, 11:36
PiRK edited the summary of this revision. (Show Details)

rebase onto D17137 which fixes the fuzzer

Harbormaster completed remote builds in B31140: Diff 50856.Nov 14 2024, 11:58
Fabien added a subscriber: Fabien.Nov 14 2024, 13:18
In D17131#388836, @PiRK wrote:

rebase onto D17137 which fixes the fuzzer

Please add the dependency in phab then

PiRK added a comment.Nov 14 2024, 13:35
In D17131#388860, @Fabien wrote:
In D17131#388836, @PiRK wrote:

rebase onto D17137 which fixes the fuzzer

Please add the dependency in phab then

I also rebased D17130 because it touches stuff added in D17137. So the dependencies are now correct:
D17131 -> D17130 -> D17137

Fabien accepted this revision.Nov 14 2024, 14:07
This revision is now accepted and ready to land.Nov 14 2024, 14:07
Closed by commit rABC48e3344f9f94: net: use Sock in CNode (authored by Vasil Dimov <vd@FreeBSD.org>, committed by PiRK). · Explain WhyNov 18 2024, 06:42
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC48e3344f9f94: net: use Sock in CNode.

Revision Contents
Changeset List

PathSize
src/
avalanche/
test/
2 lines
21 lines
54 lines
test/
10 lines
fuzz/
19 lines
29 lines

Diff 50840

View Options

src/avalanche/test/processor_tests.cpp

Loading...
View Options

src/net.h

Loading...
View Options

src/net.cpp

Loading...
View Options

src/test/denialofservice_tests.cpp

Loading...
View Options

src/test/fuzz/util.h

Loading...
View Options

src/test/net_tests.cpp

Loading...