Drop only invalid entries when reading banlist.json #22362

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2106-addrdb changing 3 files +57 −4
  1. MarcoFalke commented at 2:10 PM on June 28, 2021: member

    All entries will be dropped when there is at least one invalid one in banlist.json. Fix this by only dropping invalid ones.

    Also suggested in #20966 (comment)

  2. MarcoFalke force-pushed on Jun 28, 2021
  3. fanquake added the label P2P on Jun 28, 2021
  4. MarcoFalke force-pushed on Jun 28, 2021
  5. MarcoFalke marked this as a draft on Jun 28, 2021
  6. practicalswift commented at 11:01 PM on June 29, 2021: contributor

    Concept ACK

  7. MarcoFalke force-pushed on Jun 30, 2021
  8. DrahtBot commented at 12:14 PM on June 30, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  9. MarcoFalke force-pushed on Aug 2, 2021
  10. MarcoFalke renamed this:
    Drop (only) invalid entries when reading banlist
    Drop only invalid entries when reading banlist.json
    on Aug 2, 2021
  11. MarcoFalke force-pushed on Aug 2, 2021
  12. MarcoFalke force-pushed on Aug 7, 2021
  13. MarcoFalke force-pushed on Aug 7, 2021
  14. MarcoFalke force-pushed on Sep 6, 2021
  15. naumenkogs commented at 7:20 AM on September 22, 2021: member

    Concept ACK. I don't think this can be exploited, because the checks are static, so the only way to use it is to persuade people in banning an invalid address.

    Still, a mistake in one entry of user input should not prevent the whole ban thing from working.

  16. MarcoFalke marked this as ready for review on Sep 22, 2021
  17. MarcoFalke force-pushed on Sep 22, 2021
  18. in src/test/banman_tests.cpp:20 in fa7f08ddad outdated 
      15 | +
  16 | +BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
  17 | +
  18 | +BOOST_AUTO_TEST_CASE(file)
  19 | +{
  20 | +    SetMockTime(777s);
    naumenkogs commented at 10:52 AM on September 22, 2021:

    Dropping this seems to break the test, but I don't understand why. Perhaps a comment?

    MarcoFalke commented at 11:08 AM on September 22, 2021:

    Ban entries from the past (banned_until=778) will be dropped, unless the time is less than that.

  19. naumenkogs commented at 10:52 AM on September 22, 2021: member

    ACK fa7f08ddad1c7741ead1589730b56262ee9aeb9c

  20. DrahtBot added the label Needs rebase on Nov 16, 2021
  21. MarcoFalke force-pushed on Nov 16, 2021
  22. DrahtBot removed the label Needs rebase on Nov 16, 2021
  23. MarcoFalke force-pushed on Nov 16, 2021
  24. MarcoFalke commented at 4:14 PM on November 16, 2021: member

    Rebased to fix silent and explicit merge conflict. Should be trivial to re-ACK

  25. naumenkogs commented at 1:39 PM on November 17, 2021: member

    ACK eeee738d1c4b584968b816309dff78c8011d34bf

  26. in src/net_types.cpp:69 in eeee738d1c outdated 
      66 |          CSubNet subnet;
  67 |          const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str();
  68 |          if (!LookupSubNet(subnet_str, subnet)) {
  69 | -            throw std::runtime_error(
  70 | -                strprintf("Cannot parse banned address or subnet: %s", subnet_str));
  71 | +            LogPrintf("Dropping entry with unparseable address or subnet: %s\n", subnet_str);
    laanwj commented at 11:51 AM on December 14, 2021:

    I think this error message needs to include the context (maybe "ban list entry" instead of "entry").

  27. laanwj commented at 11:52 AM on December 14, 2021: member

    Code review ACK eeee738d1c4b584968b816309dff78c8011d34bf Small nit about error message.

  28. net: Drop only invalid entries when reading banlist.json
    Currently all entries in the file are dropped. Fix that by only dropping the invalid ones
    faa6c3d44c
  29. MarcoFalke force-pushed on Dec 14, 2021
  30. MarcoFalke commented at 6:01 PM on December 14, 2021: member

    Thanks, should be trivial to re-review with:

    git range-diff bitcoin-core/master eeee738d1c faa6c3d44c --word-diff-regex=.
  31. laanwj commented at 11:02 AM on December 15, 2021: member

    Re-ACK faa6c3d44c861c0486c1369e1d098b7645ab07cd

  32. laanwj merged this on Dec 15, 2021
  33. laanwj closed this on Dec 15, 2021
  34. MarcoFalke deleted the branch on Dec 15, 2021
  35. sidhujag referenced this in commit a210f0c43e on Dec 15, 2021
  36. DrahtBot locked this on Dec 15, 2022
Contributors
MarcoFalkepracticalswiftDrahtBotnaumenkogslaanwj
Labels
P2P
Linked (view graph)
#20966 banman: save the banlist in a JSON format on disk
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me