Skip to content

p2p, rpc, test: address rate-limiting follow-ups #22604

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 1 commit into from
Aug 14, 2021
Merged

p2p, rpc, test: address rate-limiting follow-ups #22604

fanquake merged 1 commit into from
Aug 14, 2021

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Aug 2, 2021 •
edited
Loading

Incorporates review feedback in #22387.

Edit, could be considered separately: should a release note (or two) be added for 22.0? e.g. the new getpeerinfo fields in Updated RPCs and the rate-limiting itself in P2P and network changes.

@jonatack jonatack mentioned this pull request Aug 2, 2021
Merged
@theStack
Copy link
Contributor

theStack commented Aug 2, 2021

Concept ACK

Will code-review tomorrow. At a first glance, most parts of the diff look straight-forward, the change in the rate-limiting logic (swapping the if conditions) needs probably most attention.
To speed up review, adding links to each of the review comments in #22387 that are tackled in this PR could be a good idea.

(the AppVeyor CI build fail seems to be not caused by this PR's changes, by the way)

jonatack
jonatack commented Aug 2, 2021
View reviewed changes
Copy link
Member Author

@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.

To speed up review, adding links to each of the review comments in #22387 that are tackled in this PR could be a good idea.

Good idea--added!

@sipa
Copy link
Member

sipa commented Aug 2, 2021

Concept ACK

@tryphe
Copy link
Contributor

tryphe commented Aug 3, 2021

Concept ACK

Nice fixups! Looks like there's essentially only one functional change here with if (rate_limited) {....

@maflcko
Copy link
Member

maflcko commented Aug 3, 2021

review ACK b7c4b058ba2625a39666636253620bc08a41c501

Thanks!

Also reset appveyor run.

@maflcko
Copy link
Member

maflcko commented Aug 3, 2021

Looks like there's essentially only one functional change here with if (rate_limited) {....

I think it still counts as refactoring because the change shouldn't be observable by a user

@DrahtBot DrahtBot mentioned this pull request Aug 3, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 3, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

theStack
theStack approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK b7c4b058ba2625a39666636253620bc08a41c501

@jonatack jonatack force-pushed the rate_limit_addr_follow-ups branch from b7c4b05 to d930c7f Compare August 4, 2021 17:13
@jonatack
Copy link
Member Author

jonatack commented Aug 4, 2021

Rebased per git range-diff 2b06af1 b7c4b05 d930c7f

theStack
theStack reviewed Aug 5, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK d930c7f 🌮

Checked via git range-diff b7c4b058...d930c7f5 that the changes since my last ACK were only rebase-related.

@maflcko
Copy link
Member

maflcko commented Aug 5, 2021

review ACK d930c7f

Zero-1729
Zero-1729 reviewed Aug 13, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

crACK d930c7f

LGTM 🧉

@fanquake fanquake merged commit 4c87665 into bitcoin:master Aug 14, 2021
@jonatack jonatack deleted the rate_limit_addr_follow-ups branch August 14, 2021 07:38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 15, 2021
@fanquake
Merge bitcoin#22604: p2p, rpc, test: address rate-limiting follow-ups
df1a987
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 1, 2022
@jonatack @PiRK
p2p, rpc, test: address rate-limiting follow-ups
b9ceb82 
Summary:
Incorporates review feedback in [[bitcoin/bitcoin#22387 | core#22387]]

This is a backport of [[bitcoin/bitcoin#22604 | core#22604]]
Depends on D10941

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10942
@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.

Reviewers

2 more reviewers

@theStack theStack theStack approved these changes

@Zero-1729 Zero-1729 Zero-1729 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jonatack @theStack @sipa @tryphe @maflcko @DrahtBot @Zero-1729 @fanquake