Drop only invalid entries when reading banlist.json #22362

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

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

   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:

    0git 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
MarcoFalke practicalswift DrahtBot naumenkogs laanwj

Labels
P2P