Skip to content
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.

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

test: check disconnection when sending sendaddrv2 after verack #29699

Merged

Conversation

brunoerg
Copy link
Contributor

This PR adds test coverage for:

bitcoin/src/net_processing.cpp

Lines 3796 to 3807 in 71b6319

// BIP155 defines feature negotiation of addrv2 and sendaddrv2, which must happen
// between VERSION and VERACK.
if (msg_type == NetMsgType::SENDADDRV2) {
if (pfrom.fSuccessfullyConnected) {
// Disconnect peers that send a SENDADDRV2 message after VERACK.
LogPrint(BCLog::NET, "sendaddrv2 received after verack from peer=%d; disconnecting\n", pfrom.GetId());
pfrom.fDisconnect = true;
return;
}
peer->m_wants_addrv2 = true;
return;
}

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 21, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK byaye, maflcko
Stale ACK BrandonOdiwuor, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member

maflcko commented Mar 22, 2024

lgtm ACK 8517ee9

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

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

crACK 8517ee9

@kevkevinpal
Copy link
Contributor

ACK 8517ee9

@brunoerg brunoerg force-pushed the 2024-03-test-sendaddrv2-after-verack branch from 8517ee9 to 6a37493 Compare March 28, 2024 10:20
@brunoerg brunoerg force-pushed the 2024-03-test-sendaddrv2-after-verack branch from 6a37493 to b4c9ace Compare March 28, 2024 10:25
@brunoerg
Copy link
Contributor Author

Force-pushed addressing #29699 (comment)

Copy link

@byaye byaye left a comment

Choose a reason for hiding this comment

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

Tested ACK b4c9ace

Screenshot 2024-04-15 at 09 03 30

@maflcko
Copy link
Member

maflcko commented Apr 15, 2024

lgtm ACK b4c9ace

Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -92 to +100
f'received: addrv2 ({msg_size} bytes) peer=0',
f'sending addrv2 ({msg_size} bytes) peer=1',
f'received: addrv2 ({msg_size} bytes) peer=1',
f'sending addrv2 ({msg_size} bytes) peer=2',
Copy link
Member

Choose a reason for hiding this comment

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

(I think this is a good example of why we shouldn't use assert_debug_log when we're able to check the logic otherwise)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :)

@glozow glozow merged commit df609a3 into bitcoin:master Apr 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants