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: make sure non-IP peers get discouraged and disconnected (vasild) #21571

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 2, 2021

Split up from #20966, so that it can be backported easier. Merging this ahead of #20966 will also reduce the number of conflicts for that pull.

@maflcko maflcko changed the title test: make sure non-IP peers get discouraged and disconnected test: make sure non-IP peers get discouraged and disconnected (vasild) Apr 2, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 2, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 3d5eac6

A few optional suggestions follow.

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved
src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved
src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.

The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.

If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
Use `CConnmanTest` instead of `CConnman` and add the nodes to it
so that their `fDisconnect` flag is set during disconnection.
@jonatack
Copy link
Contributor

jonatack commented Apr 2, 2021

ACK 81747b2

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

81747b2 looks good

I verified that the 3 commits in this PR are the same as the ones in #20966 (except conflict resolution and minor change in comment).

I think it may be misleading if I ACK this PR, coz im the author of the commitz.

@maflcko maflcko merged commit 1a7dec7 into bitcoin:master Apr 6, 2021
@maflcko maflcko deleted the 2104-testPointers branch April 6, 2021 08:30
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 6, 2021
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.

The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.

If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.

Github-Pull: bitcoin#21571
Rebased-From: 4d6e246
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 6, 2021
Use `CConnmanTest` instead of `CConnman` and add the nodes to it
so that their `fDisconnect` flag is set during disconnection.

Github-Pull: bitcoin#21571
Rebased-From: 637bb6d
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 6, 2021
@maflcko
Copy link
Member Author

maflcko commented Apr 6, 2021

Backported in #21614

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 6, 2021
… disconnected (vasild)

81747b2 test: make sure non-IP peers get discouraged and disconnected (Vasil Dimov)
637bb6d test: also check disconnect in denialofservice_tests/peer_discouragement (Vasil Dimov)
4d6e246 test: use pointers in denialofservice_tests/peer_discouragement (Vasil Dimov)

Pull request description:

  Split up from bitcoin#20966, so that it can be backported easier. Merging this ahead of bitcoin#20966 will also reduce the number of conflicts for that pull.

ACKs for top commit:
  jonatack:
    ACK 81747b2

Tree-SHA512: 8f0e30b95baba7f056920d7fc3b37bd49ee13e69392fe80e2d333c6bb09fd25f4603249301b8795cca26a2f2d15b9f8904798a55cd9c04fd28afb316e95c551c
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 16, 2021
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.

The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.

If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.

Github-Pull: bitcoin#21571
Rebased-From: 4d6e246
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 16, 2021
Use `CConnmanTest` instead of `CConnman` and add the nodes to it
so that their `fDisconnect` flag is set during disconnection.

Github-Pull: bitcoin#21571
Rebased-From: 637bb6d
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 16, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants