Page MenuHomePhabricator

net: fix GetListenPort() to derive the proper port
ClosedPublic

Authored by PiRK on Oct 26 2023, 18:07.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC50401c4ab752: net: fix GetListenPort() to derive the proper port
Summary

timedata: make it possible to reset the state

Add a new function TestOnlyResetTimeData() which would reset the
internal state used by GetTimeOffset(), GetAdjustedTime() and
AddTimeData().

This is needed so that unit tests that call AddTimeData() can restore
the state in order not to confuse other tests that rely on it.

Currently timedata_tests/addtimedata is the only test that modifies
the state (via AddTimeData()) and also the only test that relies on
that state.

timedata: rename variables to match the coding style

Rename the local variables in src/timedata.cpp:
setKnown -> g_sources
vTimeOffsets -> g_time_offsets
fDone -> g_warning_emitted

net: make CaptureMessage() mockable

Rename CaptureMessage() to CaptureMessageToFile() and introduce a
std::function variable called CaptureMessage whose value can be
changed by unit tests, should they need to inspect message contents.

net: pass Span by value to CaptureMessage()

Span is lightweight and need not be passed by const reference.

net: fix GetListenPort() to derive the proper port

GetListenPort() uses a simple logic: "if -port=P is given, then we
must be listening on P, otherwise we must be listening on 8333".
This is however not true if -bind= has been provided with :port part
or if -whitebind= has been provided. Thus, extend GetListenPort() to
return the port from -bind= or -whitebind=, if any.

Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)

TestChainState is from core#20332

net: only assume all local addresses if listening on any

If -bind= is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.

Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)

This is a backport of core#20196

Depends on D14689

Test Plan

ninja all check-all

run new functional tests as per instructions

$ sudo su
$ ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up
$ exit
$ ifconfig | grep LOOPBACK
$ test/functional/test_runner.py feature_bind_port_discover.py --ihave1111and2222
$ test/functional/test_runner.py feature_bind_port_externalip.py --ihave1111
$ sudo su
$ ifconfig lo:0 down && ifconfig lo:1 down

Event Timeline

Tail of the build log:

[192/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[193/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[194/489] pow: testing aserti32d_tests
[195/489] Running utility command for check-pow-aserti32d_tests
[196/489] Running pow test suite
PASSED: pow test suite
[197/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[198/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[199/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/psbt_wallet_tests.cpp.o
[200/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/ismine_tests.cpp.o
[201/489] avalanche: testing peermanager_tests
[202/489] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[203/489] Running utility command for check-avalanche-peermanager_tests
[204/489] avalanche: testing processor_tests
[205/489] Running utility command for check-avalanche-processor_tests
[206/489] Running avalanche test suite
PASSED: avalanche test suite
[207/489] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[208/489] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[209/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/walletdb_tests.cpp.o
[210/489] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[211/489] Linking CXX executable src/seeder/test/test-seeder
[212/489] seeder: testing options_tests
[213/489] seeder: testing message_writer_tests
[214/489] seeder: testing parse_name_tests
[215/489] seeder: testing p2p_messaging_tests
[216/489] Running utility command for check-seeder-options_tests
[217/489] Running utility command for check-seeder-message_writer_tests
[218/489] Running utility command for check-seeder-parse_name_tests
[219/489] Running utility command for check-seeder-p2p_messaging_tests
[220/489] seeder: testing write_name_tests
[221/489] Running utility command for check-seeder-write_name_tests
[222/489] Running seeder test suite
PASSED: seeder test suite
[223/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/coinselector_tests.cpp.o
[224/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[225/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[226/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[227/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[228/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[229/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[230/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[231/489] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[232/489] Linking CXX executable src/qt/test/test_bitcoin-qt
[233/489] bitcoin-qt: testing test_bitcoin-qt
[234/489] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[235/489] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

Tail of the build log:

[184/482] pow: testing eda_tests
[185/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/options_tests.cpp.o
[186/482] Running utility command for check-pow-eda_tests
[187/482] pow: testing grasberg_tests
[188/482] Running utility command for check-pow-grasberg_tests
[189/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[190/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
[191/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[192/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[193/482] pow: testing aserti32d_tests
[194/482] Running utility command for check-pow-aserti32d_tests
[195/482] Running pow test suite
PASSED: pow test suite
[196/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[197/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[198/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[199/482] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[200/482] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[201/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[202/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[203/482] Test Bitcoin utilities...
[204/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[205/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[206/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[207/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[208/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[209/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[210/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[211/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/fixture.cpp.o
[212/482] Linking CXX executable src/seeder/test/test-seeder
[213/482] seeder: testing message_writer_tests
[214/482] seeder: testing p2p_messaging_tests
[215/482] seeder: testing options_tests
[216/482] seeder: testing parse_name_tests
[217/482] Running utility command for check-seeder-message_writer_tests
[218/482] seeder: testing write_name_tests
[219/482] Running utility command for check-seeder-p2p_messaging_tests
[220/482] Running utility command for check-seeder-options_tests
[221/482] Running utility command for check-seeder-parse_name_tests
[222/482] Running utility command for check-seeder-write_name_tests
[223/482] Running seeder test suite
PASSED: seeder test suite
[224/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[225/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[226/482] Linking CXX executable src/qt/test/test_bitcoin-qt
[227/482] bitcoin-qt: testing test_bitcoin-qt
[228/482] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
PiRK edited the test plan for this revision. (Show Details)

remove debugging prints, fix var shadowing

PiRK published this revision for review.Oct 27 2023, 08:07
PiRK added inline comments.
src/test/net_tests.cpp
1322 ↗(On Diff #42834)

I had to adjust this line to account for differences in the version message, to make it work for Bitcoin ABC (prevent a failure when deserializing the message)

src/validation.h
653 ↗(On Diff #42834)

Tail of the build log:

[179/482] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/grasberg_tests.cpp.o
[180/482] Linking CXX executable src/pow/test/test-pow
[181/482] pow: testing daa_tests
[182/482] Running utility command for check-pow-daa_tests
[183/482] Test Bitcoin utilities...
[184/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/parse_name_tests.cpp.o
[185/482] pow: testing eda_tests
[186/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
[187/482] Running utility command for check-pow-eda_tests
[188/482] pow: testing grasberg_tests
[189/482] Running utility command for check-pow-grasberg_tests
[190/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/options_tests.cpp.o
[191/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/write_name_tests.cpp.o
[192/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[193/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[194/482] pow: testing aserti32d_tests
[195/482] Running utility command for check-pow-aserti32d_tests
[196/482] Running pow test suite
PASSED: pow test suite
[197/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[198/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[199/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[200/482] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[201/482] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[202/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[203/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[204/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[205/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[206/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[207/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[208/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[209/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[210/482] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/fixture.cpp.o
[211/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[212/482] Linking CXX executable src/seeder/test/test-seeder
[213/482] seeder: testing message_writer_tests
[214/482] seeder: testing p2p_messaging_tests
[215/482] seeder: testing options_tests
[216/482] seeder: testing parse_name_tests
[217/482] seeder: testing write_name_tests
[218/482] Running utility command for check-seeder-options_tests
[219/482] Running utility command for check-seeder-message_writer_tests
[220/482] Running utility command for check-seeder-p2p_messaging_tests
[221/482] Running utility command for check-seeder-parse_name_tests
[222/482] Running utility command for check-seeder-write_name_tests
[223/482] Running seeder test suite
PASSED: seeder test suite
[224/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[225/482] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[226/482] Linking CXX executable src/qt/test/test_bitcoin-qt
[227/482] bitcoin-qt: testing test_bitcoin-qt
[228/482] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
PiRK planned changes to this revision.Oct 27 2023, 11:37

fix argument name in comment

Fabien requested changes to this revision.Oct 27 2023, 13:41
Fabien added a subscriber: Fabien.

This is unfortunate but these tests will always be skipped on CI

src/init.cpp
2730

You can derive params from config here

src/test/util/validation.cpp
26

dito

src/test/util/validation.h
24

This has nothing to do with this diff

src/validation.h
653

dito

This revision now requires changes to proceed.Oct 27 2023, 13:41
src/test/util/validation.h
24

The new uses this. It is from https://github.com/bitcoin/bitcoin/pull/20332 (but the rest of the PR is not applicable due to missing fuzzer backports)

24

*The new test

use config to derive chainparams

This revision is now accepted and ready to land.Oct 27 2023, 18:30