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: add coverage for -bantime #26604

Merged
merged 1 commit into from
May 1, 2023

Conversation

brunoerg
Copy link
Contributor

This PR adds test coverage for -bantime. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set bantime when using setban).

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 29, 2022

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

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.

@DrahtBot DrahtBot added the Tests label Nov 29, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Concept ACK

I was testing ChatGPT to see if it can review pull requests and it suggested 4 improvements that make sense:

@brunoerg
Copy link
Contributor Author

brunoerg commented Dec 1, 2022

Thanks, @1440000bytes for your review. Interesting experiment using an AI, however, I believe it doesn't deal so well with some specific stuff from our framework. E.g. It doesn't know 100% what our assert functions do (perhaps the reason it suggested a text message on the assertion).

@brunoerg brunoerg force-pushed the 2022-11-setban-bantime-test branch from 9be1a6c to 9c18992 Compare March 15, 2023 15:32
self.log.info("Test -bantime")
self.restart_node(1, ["-bantime=1234"])
self.nodes[1].setban("127.0.0.1", "add")
banned = self.nodes[1].listbanned()[0]
Copy link
Member

Choose a reason for hiding this comment

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

tAck but how about adding another coverage to test the ban remaining time after the bantime elapsed.

time.sleep(secs)
banned = self.nodes[1].listbanned()[0]
assert_equal(banned["time_remaining"],0)

@maflcko
Copy link
Member

maflcko commented Apr 30, 2023

lgtm ACK 9c18992

@fanquake fanquake merged commit ab99b95 into bitcoin:master May 1, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2023
9c18992 test: add coverage for `-bantime` (brunoerg)

Pull request description:

  This PR adds test coverage for `-bantime`. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set `bantime` when using `setban`).

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 9c18992

Tree-SHA512: e95f8608aa5df9b09cc5577daae662ed79ef5d5c69ee5e704d7c69520b9b51cc142e9e6be69d80356eda25a5215c4770b1a208638560c48cd3bc8f6d195a371f
@bitcoin bitcoin locked and limited conversation to collaborators Apr 30, 2024
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.

5 participants