Skip to content

Commit 39363a4

Browse files
author
MarcoFalke
committed
Merge bitcoin/bitcoin#26822: p2p, rpc: don't allow past absolute timestamp 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
2 parents 49aefc2 + abccb27 commit 39363a4

File tree

2 files changed

+17
-2
lines changed

2 files changed

+17
-2
lines changed

src/rpc/net.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,10 @@ static RPCHelpMan setban()
741741

742742
const bool absolute{request.params[3].isNull() ? false : request.params[3].get_bool()};
743743

744+
if (absolute && banTime < GetTime()) {
745+
throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Absolute timestamp is in the past");
746+
}
747+
744748
if (isSubnet) {
745749
node.banman->Ban(subNet, banTime, absolute);
746750
if (node.connman) {

test/functional/p2p_disconnect_ban.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ def run_test(self):
4646
assert_raises_rpc_error(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add")
4747
assert_equal(len(self.nodes[1].listbanned()), 1) # still only one banned ip because 127.0.0.1 is within the range of 127.0.0.0/24
4848

49+
self.log.info("setban: fail to ban with past absolute timestamp")
50+
assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True)
51+
4952
self.log.info("setban remove: fail to unban a non-banned subnet")
5053
assert_raises_rpc_error(-30, "Error: Unban failed", self.nodes[1].setban, "127.0.0.1", "remove")
5154
assert_equal(len(self.nodes[1].listbanned()), 1)
@@ -66,9 +69,13 @@ def run_test(self):
6669
self.nodes[1].setban("2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19", "add", 1000) # ban for 1000 seconds
6770
listBeforeShutdown = self.nodes[1].listbanned()
6871
assert_equal("192.168.0.1/32", listBeforeShutdown[2]['address'])
72+
73+
self.log.info("setban: test banning with absolute timestamp")
74+
self.nodes[1].setban("192.168.0.2", "add", old_time + 120, True)
75+
6976
# Move time forward by 3 seconds so the third ban has expired
7077
self.nodes[1].setmocktime(old_time + 3)
71-
assert_equal(len(self.nodes[1].listbanned()), 3)
78+
assert_equal(len(self.nodes[1].listbanned()), 4)
7279

7380
self.log.info("Test ban_duration and time_remaining")
7481
for ban in self.nodes[1].listbanned():
@@ -78,13 +85,17 @@ def run_test(self):
7885
elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19":
7986
assert_equal(ban["ban_duration"], 1000)
8087
assert_equal(ban["time_remaining"], 997)
88+
elif ban["address"] == "192.168.0.2/32":
89+
assert_equal(ban["ban_duration"], 120)
90+
assert_equal(ban["time_remaining"], 117)
8191

8292
self.restart_node(1)
8393

8494
listAfterShutdown = self.nodes[1].listbanned()
8595
assert_equal("127.0.0.0/24", listAfterShutdown[0]['address'])
8696
assert_equal("127.0.0.0/32", listAfterShutdown[1]['address'])
87-
assert_equal("/19" in listAfterShutdown[2]['address'], True)
97+
assert_equal("192.168.0.2/32", listAfterShutdown[2]['address'])
98+
assert_equal("/19" in listAfterShutdown[3]['address'], True)
8899

89100
# Clear ban lists
90101
self.nodes[1].clearbanned()

0 commit comments

Comments
 (0)