Page MenuHomePhabricator

[avalanche] refactor conditional processing in ProcessMessage
ClosedPublic

Authored by PiRK on Mon, Mar 22, 09:14.

Details

Summary

In ProcessMessage, when checking for avalanche message types, there
are additional checks made in the same if statement. This can cause
the Unknown command... log message to be printed at the end of
ProcessMessage when the message is not unknown.

This has the additional benefit of factoring some repeated checks.

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Mon, Mar 22, 09:14

I suspect that the if (!g_avalanche) test will never be true. In init.cpp, g_avalanche is set in step 7 before the node actually starts connecting to peers (step 12).

Unless there is a risk that we process late network messages while shutting down the node? But, if I understand the code correctly, the network processing is shut down before avalanche::Processor is destroyed, so this should not happen.

Fabien requested changes to this revision.Mon, Mar 22, 13:32
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/net_processing.cpp
2688 ↗(On Diff #27986)

drop the if:

return msg_type == NetMsgType::AVAHELLO || msg_type == NetMsgType::AVAPOLL ||
        msg_type == NetMsgType::AVARESPONSE ||
        msg_type == NetMsgType::AVAPROOF;
2731 ↗(On Diff #27986)

Out of scope, but I think it makes sense to use IsInitialBlockDownload() here. We cannot poll/answer during IBD, and this includes the actual checks.

This revision now requires changes to proceed.Mon, Mar 22, 13:32
This revision is now accepted and ready to land.Wed, Mar 24, 09:40

Tail of the build log:

[380/439] bitcoin: testing merkleblock_tests
[381/439] bitcoin: testing script_commitment_tests
[382/439] bitcoin: testing sighashtype_tests
[383/439] bitcoin: testing bip32_tests
[384/439] Running utility command for check-bitcoin-merkleblock_tests
[385/439] Running utility command for check-bitcoin-script_commitment_tests
[386/439] Running utility command for check-bitcoin-sighashtype_tests
[387/439] Running utility command for check-bitcoin-bip32_tests
[388/439] bitcoin: testing torcontrol_tests
[389/439] Running utility command for check-bitcoin-torcontrol_tests
[390/439] bitcoin: testing settings_tests
[391/439] Running utility command for check-bitcoin-settings_tests
[392/439] bitcoin: testing streams_tests
[393/439] bitcoin: testing timedata_tests
[394/439] bitcoin: testing addrman_tests
[395/439] Running utility command for check-bitcoin-timedata_tests
[396/439] Running utility command for check-bitcoin-streams_tests
[397/439] Running utility command for check-bitcoin-addrman_tests
[398/439] bitcoin: testing scriptpubkeyman_tests
[399/439] Running utility command for check-bitcoin-scriptpubkeyman_tests
[400/439] bitcoin: testing wallet_crypto_tests
[401/439] Running utility command for check-bitcoin-wallet_crypto_tests
[402/439] bitcoin: testing uint256_tests
[403/439] Running utility command for check-bitcoin-uint256_tests
[404/439] bitcoin: testing txvalidationcache_tests
[405/439] bitcoin: testing radix_tests
[406/439] bitcoin: testing walletdb_tests
[407/439] Running utility command for check-bitcoin-txvalidationcache_tests
[408/439] bitcoin: testing script_standard_tests
[409/439] bitcoin: testing serialize_tests
[410/439] Running utility command for check-bitcoin-radix_tests
[411/439] Running utility command for check-bitcoin-walletdb_tests
[412/439] bitcoin: testing schnorr_tests
[413/439] Running utility command for check-bitcoin-script_standard_tests
[414/439] Running utility command for check-bitcoin-serialize_tests
[415/439] Running utility command for check-bitcoin-schnorr_tests
[416/439] bitcoin: testing blockcheck_tests
[417/439] bitcoin: testing getarg_tests
[418/439] Running utility command for check-bitcoin-blockcheck_tests
[419/439] Running utility command for check-bitcoin-getarg_tests
[420/439] bitcoin: testing versionbits_tests
[421/439] Running utility command for check-bitcoin-versionbits_tests
[422/439] bitcoin: testing validation_block_tests
[423/439] Running utility command for check-bitcoin-validation_block_tests
[424/439] bitcoin: testing wallet_tests
[425/439] Running utility command for check-bitcoin-wallet_tests
[426/439] bitcoin: testing skiplist_tests
[427/439] Running utility command for check-bitcoin-skiplist_tests
[428/439] bitcoin: testing monolith_opcodes_tests
[429/439] Running utility command for check-bitcoin-monolith_opcodes_tests
[430/439] bitcoin: testing script_tests
[431/439] Running utility command for check-bitcoin-script_tests
[432/439] bitcoin: testing transaction_tests
[433/439] Running utility command for check-bitcoin-transaction_tests
[434/439] bitcoin: testing op_reversebytes_tests
[435/439] Running utility command for check-bitcoin-op_reversebytes_tests
[436/439] bitcoin: testing coins_tests
[437/439] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

rebase the commit onto master (drop the avaproof message)

This can be merged first, while D9327 is being worked on or replaced by another diff