rpc: add additional ban time fields to listbanned #21602

This PR adds a ban_duration and time_remaining field to the listbanned RPC command. Thanks to jonatack, this PR also expands the listbanned test coverage to include these new fields

It's useful to keep track of ban_duration as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI bantablemodel as part of a follow-up PR. As suggested by jonatack, time_remaining is another useful user-centric data point.

Since a ban always expires after its created, the ban_created field is now placed before the banned_until field. This new ordering is more logical.

This PR also improves the help listbanned output by providing additional context to the descriptions of the address, ban_created, and banned_until fields.

Master: listbanned

[
  {
    "address": "1.2.3.4/32",
    "banned_until": 1617691101,
    "ban_created": 1617604701
  },
  {
    "address": "135.181.41.129/32",
    "banned_until": 1649140716,
    "ban_created": 1617604716
  }
]

PR: listbanned

[
  {
    "address": "1.2.3.4/32",
    "ban_created": 1617775773,
    "banned_until": 1617862173,
    "ban_duration": 86400,
    "time_remaining": 86392
  },
  {
    "address": "3.114.211.172/32",
    "ban_created": 1617753165,
    "banned_until": 1618357965,
    "ban_duration": 604800,
    "time_remaining": 582184
  }
]

I'm not sure about this.

Would this change cause versioning/compatibility issues with previous/existing ban entries? (test coverage needed). It may be better to avoid changing CBanEntry, and have the RPC handle the calculation.

As a human user, I've found myself wishing for a "time_remaining" field in listbanned.

@jonatack

It may be better to avoid changing CBanEntry, and have the RPC handle the calculation.

You're right, taking into consideration compatibility guarantees, performing the calculation is the simplest path. I will revise this PR to perform the calculation and avoid changing CBanEntry and Banman.

As a human user, I've found myself wishing for a "time_remaining" field in listbanned.

I will look into adding this field, thanks for the review!

updated from 9c896134b12f012d0736742693df4b50c133e506 to 8e34b5ed06f3ca0865b9b7dca6e31f2e95c6f5c8, addressed @jonatack suggestions:

• perform the calculation inside the RPC command instead of changing CBanEntry and Banman. This avoids any compatibility issues.
• Add additional ban_time_remaining field to listbanned

updated from 8e34b5e to 833bebdece0f6fac5fecf602e88ef7c114f2416c, addressed @sipa suggestion:

• GetTime() was being called inside of a loop due to an oversight. This is unnecessary and can produce inconsistent results. This moves GetTime() out of the loop so it is only called once.

utACK 833bebdece0f6fac5fecf602e88ef7c114f2416c

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Tested approach ACK

Various ideas and suggestions below grouped into one comment. If you retouch, perhaps update the relevant help in each commit that adds a field, so each commit is self-contained. Also, needs a release note.

@jarolrod feel free to pull in the last commit in this branch https://github.com/jonatack/bitcoin/commits/pr21602-suggestions to add unit test coverage for these changes

updated from 833bebd to c5c920f, addressed @jonatack suggestions

• swap positions of ban_created and banned_until
• rename ban_time to ban_duration
• rename ban_time_remaining to time_remaining
• drop #include util/time.h, this is already included by something else.
  • Other commands already use time.h functions in this part of the code.
• Add test coverage for new listbanned fields, thanks to jon
• Add release notes for new fields + banned_until reposition

Tested ACK c5c920f394891f1d6958913330da564bf24a71a9

Updated from c5c920f to d3b0b08

Changes:

• Include PR# into release notes

re-ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec

There's a small potential race in the added unit tests. I didn't hit while running the test, even with valgrind to slow it down, but maybe something like this would provide a margin:

@@ -277,6 +277,7 @@ BOOST_AUTO_TEST_CASE(rpc_ban)
 
     BOOST_CHECK_NO_THROW(r = CallRPC(std::string("setban 127.0.0.0/24 add 200")));
     BOOST_CHECK_NO_THROW(r = CallRPC(std::string("listbanned")));
+    const int64_t now{GetTime()};
     ar = r.get_array();
     o1 = ar[0].get_obj();
     adr = find_value(o1, "address");
@@ -284,12 +285,13 @@ BOOST_AUTO_TEST_CASE(rpc_ban)
     const int64_t ban_created{find_value(o1, "ban_created").get_int64()};
     const int64_t ban_duration{find_value(o1, "ban_duration").get_int64()};
     const int64_t time_remaining{find_value(o1, "time_remaining").get_int64()};
-    const int64_t now{GetTime()};
     BOOST_CHECK_EQUAL(adr.get_str(), "127.0.0.0/24");
     BOOST_CHECK(banned_until > now);
     BOOST_CHECK(banned_until - now <= 200);
-    BOOST_CHECK_EQUAL(ban_duration, banned_until - ban_created);
-    BOOST_CHECK_EQUAL(time_remaining, banned_until - now);
+    BOOST_CHECK(ban_duration >= banned_until - ban_created - 1);
+    BOOST_CHECK(ban_duration <= banned_until - ban_created + 1);
+    BOOST_CHECK(time_remaining >= banned_until - now - 1);
+    BOOST_CHECK(time_remaining <= banned_until - now + 1);

Code review d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec

I'd drop dd3c8eaa3399b28dc78a883ff78cbe7cc5c31b5b, but if kept then mark it move-only or squash with c95c61657afd058b46549fb3d65633d7c736f5fc.

I'd also squash 5456b345312857981cb426712f0665800c682e09 with 3e978d1a5dbd43f85bd03e759984ab1f209d6e34.

Concept ACK on:

I found this helpful in adding additional context columns to the GUI bantablemodel as part of a follow-up PR.

Approach ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec.

I found this helpful in adding additional context columns to the GUI bantablemodel as part of a follow-up PR.

It seems, that adding additional time-related fields to the "Banned peers" table in the GUI does not depend on this PR :)

ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec, tested on Linux Mint 20.1 (x86_64).

review ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec 🕙

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec 🕙
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgCnQv+Pt8SJP4wssUe0R41DmjzJ0d0nEqEiWA38vaipjeD6zNgy5yBhuOmtsm/
vdlSvsOogPjoXM7nrCEaODFRdM8Sj9cfFtPi94U6dmU8lIv92urpREeUm92Zf1PI
2z0mMJwktzszLVK1t3p6Qh90iV3RDy/OE/kkKYOjs41wLMtt1akHchkGVVMuA6P9
w3L8rVoG/6dlWsWd9H9bWvJ/0QEm12zK9hlHZONGBCHC4NmM/QEYLFqULmsAVmuw
7h9XysqcQVCVTFcUD4Qk7y2WOM3vtW3OI2N+DzLK9tOcYvJ0LlGByNe5RSGnLzgl
wewt7k2piUr8qSQNKIvCSIFoy3K3BuVAGWpvFmMgR6tEidhrMhMxFc+rCcvPQbDI
gHypLP4J3DwU55PQ/vAO4IpsSudiVmgUbO9gkrZCJ5r9qMTgJhWtN7epkTgcVLUn
aXbS1eLlMrEcnRt1VdLc42VOo4P755/lNg/QmZcQTub3Z46KxhOhf+gSTOut3KL6
3lJzmjRC
=qw8I
-----END PGP SIGNATURE-----

Timestamp of file with hash 4d1380dd6579b539625e1fcb8f85f0b5ec4b795d7be43d7e91a24d790b488c20 -

</details>

(edited OP to remove @)