p2p, rpc: don't allow past absolute timestamp in setban
#26822
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
andrewtoth
left a comment
andrewtoth
left a comment
There was a problem hiding this comment.
Concept ACK
Banning with an absolute timestamp in the past is surely a user error, so it should not just fail to ban silently.
Perhaps out of scope of this PR but I see there is a rpc_setban.py functional test file that largely overlaps with p2p_disconnect_ban.py. Should they be merged into a single file?
brunoerg
force-pushed
the
2023-01-fix-ban-time
branch
from
b5db260 to
ff52aaf
Compare
|
Thanks @andrewtoth and @1440000bytes. Force-pushed addressing #26822 (comment) and #26822 (comment) |
I think so, specially part of them are testing same thing (e.g. the persistence across node restart). |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
What is the point of adding this check if relative times still accept -1?
I think both accepts -1 but sets the ban to the default time (86400), it's the behavior of the Lines 134 to 137 in 911a40e However, there is no check for past absolute timestamp, and I think it's more appropriate throws an error than setting the ban to the default time (if it would be an option). Maybe the RPC should not accept negative values for any time (relative or absolute). |
brunoerg
force-pushed
the
2023-01-fix-ban-time
branch
from
ff52aaf to
abccb27
Compare
A relative time of |
…stamp in `setban` abccb27 test: add coverage for absolute timestamp in `setban` (brunoerg) b99f1f2 p2p, rpc: don't allow past absolute timestamp in `setban` (brunoerg) Pull request description: We shouldn't allow call `setban` with past absolute timestamp. First, because doesn't make sense to ban a node until ~ past ~. Besides that, it could make an unnecessary write to the DB since `BanMan::Ban` calls `DumpBanlist` and it calls `SweepBanned` which will remove this new ban (because of the timestamp) of the array. ACKs for top commit: 1440000bytes: ACK bitcoin/bitcoin@abccb27 Tree-SHA512: 6d0cadf99fc4f78d77d3bafd6f5c85ac56e243ebc376210fdb2bee751e7b139ec7d6f5f346317fd0b10051b685f2d0ee1d8e40f4bc10f4dbdbbddd5e1ee84de5
… `setban` abccb27 test: add coverage for absolute timestamp in `setban` (brunoerg) b99f1f2 p2p, rpc: don't allow past absolute timestamp in `setban` (brunoerg) Pull request description: We shouldn't allow call `setban` with past absolute timestamp. First, because doesn't make sense to ban a node until ~ past ~. Besides that, it could make an unnecessary write to the DB since `BanMan::Ban` calls `DumpBanlist` and it calls `SweepBanned` which will remove this new ban (because of the timestamp) of the array. ACKs for top commit: 1440000bytes: ACK bitcoin@abccb27 Tree-SHA512: 6d0cadf99fc4f78d77d3bafd6f5c85ac56e243ebc376210fdb2bee751e7b139ec7d6f5f346317fd0b10051b685f2d0ee1d8e40f4bc10f4dbdbbddd5e1ee84de5
4 participants
We shouldn't allow call
setbanwith past absolute timestamp. First, because doesn't make sense to ban a node until ~ past ~. Besides that, it could make an unnecessary write to the DB sinceBanMan::BancallsDumpBanlistand it callsSweepBannedwhich will remove this new ban (because of the timestamp) of the array.