p2p, rpc: don't allow past absolute timestamp in `setban` #26822

brunoerg commented at 6:45 PM on January 5, 2023: contributor

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.

DrahtBot commented at 6:45 PM on January 5, 2023: contributor

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	1440000bytes
Concept ACK	andrewtoth

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:

#19825 (rpc: simpler setban and new ban manipulation commands by dhruv)

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.

in test/functional/p2p_disconnect_ban.py:51 in b5db26060c outdated

  45 | @@ -46,6 +46,9 @@ def run_test(self):
  46 |          assert_raises_rpc_error(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add")
  47 |          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
  48 |  
  49 | +        self.log.info("setban: fail to ban with past absolute timestamp")
  50 | +        assert_raises_rpc_error(-8, "Error: Past absolute timestamp is not allowed", self.nodes[1].setban, "127.27.0.1", "add", 123, True)
  51 | +

andrewtoth commented at 2:49 AM on January 6, 2023:

There doesn't appear to be a test for successfully banning with absolute time. Maybe that test should be added as well?

brunoerg commented at 11:53 AM on January 6, 2023:

Yes, I'm gonna add it!

in src/rpc/net.cpp:745 in b5db26060c outdated

 740 | @@ -741,6 +741,10 @@ static RPCHelpMan setban()
 741 |  
 742 |          const bool absolute{request.params[3].isNull() ? false : request.params[3].get_bool()};
 743 |  
 744 | +        if (absolute && banTime < GetTime()) {
 745 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Past absolute timestamp is not allowed");

andrewtoth commented at 2:50 AM on January 6, 2023:

            throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Absolute timestamp is in the past");

andrewtoth commented at 2:53 AM on January 6, 2023: contributor

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?

in test/functional/p2p_disconnect_ban.py:50 in b5db26060c outdated

  45 | @@ -46,6 +46,9 @@ def run_test(self):
  46 |          assert_raises_rpc_error(-30, "Error: Invalid IP/Subnet", self.nodes[1].setban, "127.0.0.1/42", "add")
  47 |          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
  48 |  
  49 | +        self.log.info("setban: fail to ban with past absolute timestamp")
  50 | +        assert_raises_rpc_error(-8, "Error: Past absolute timestamp is not allowed", self.nodes[1].setban, "127.27.0.1", "add", 123, True)

unknown commented at 3:54 AM on January 6, 2023:

        assert_raises_rpc_error(-8, "Error: Past absolute timestamp is not allowed", self.nodes[1].setban, "127.0.0.1", "add", 123, True)

brunoerg commented at 12:21 PM on January 6, 2023:

I can't use "127.0.0.1" because it will throw another error (ip/subnet already banned) instead of the "absolute timestamp" one. That's why I'm using a random ip, it doesn't impact the purpose of the test.

unknown commented at 3:54 AM on January 6, 2023: none

Concept ACK

brunoerg force-pushed on Jan 6, 2023

brunoerg commented at 12:22 PM on January 6, 2023: contributor

Thanks @andrewtoth and @1440000bytes. Force-pushed addressing #26822 (review) and #26822 (review)

brunoerg commented at 12:31 PM on January 6, 2023: contributor

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?

I think so, specially part of them are testing same thing (e.g. the persistence across node restart).

maflcko commented at 1:01 PM on January 6, 2023: member

What is the point of adding this check if relative times still accept -1?

brunoerg commented at 1:20 PM on January 6, 2023: contributor

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 BanMan::Ban, not sure why: https://github.com/bitcoin/bitcoin/blob/911a40ead256b8849166cff1b745b9c9898e2da8/src/banman.cpp#L134-L137

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

p2p, rpc: don't allow past absolute timestamp in `setban` b99f1f20f7

test: add coverage for absolute timestamp in `setban` abccb27466

brunoerg force-pushed on Jan 6, 2023

unknown approved

unknown commented at 2:26 AM on January 7, 2023: none

ACK https://github.com/bitcoin/bitcoin/pull/26822/commits/abccb27466c5a240a59f3f7c86f2bc60d51e9fee

andrewtoth commented at 2:26 AM on January 7, 2023: contributor

What is the point of adding this check if relative times still accept -1?

A relative time of -1 will still ban the peer for the default time, so it will succeed. Banning with an absolute time in the past will not ban the peer, so it will fail silently.

maflcko referenced this in commit 39363a4b94 on Jan 9, 2023

maflcko closed this on Jan 9, 2023

sidhujag referenced this in commit 213f80fcbf on Jan 9, 2023

bitcoin locked this on Jan 9, 2024