Drop only invalid entries when reading banlist.json #22362
Conversation
|
Concept ACK |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
0075737 to
77335ff
Compare
|
Concept ACK. Still, a mistake in one entry of user input should not prevent the whole ban thing from working. |
8023243 to
fa7f08d
Compare
|
|
||
| BOOST_AUTO_TEST_CASE(file) | ||
| { | ||
| SetMockTime(777s); |
There was a problem hiding this comment.
Dropping this seems to break the test, but I don't understand why. Perhaps a comment?
There was a problem hiding this comment.
Ban entries from the past (banned_until=778) will be dropped, unless the time is less than that.
|
ACK fa7f08d |
|
Rebased to fix silent and explicit merge conflict. Should be trivial to re-ACK |
|
ACK eeee738 |
src/net_types.cpp
Outdated
| CSubNet subnet; | ||
| const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str(); | ||
| if (!LookupSubNet(subnet_str, subnet)) { | ||
| throw std::runtime_error( | ||
| strprintf("Cannot parse banned address or subnet: %s", subnet_str)); | ||
| LogPrintf("Dropping entry with unparseable address or subnet: %s\n", subnet_str); |
There was a problem hiding this comment.
I think this error message needs to include the context (maybe "ban list entry" instead of "entry").
|
Code review ACK eeee738 |
Currently all entries in the file are dropped. Fix that by only dropping the invalid ones
|
Thanks, should be trivial to re-review with: git range-diff bitcoin-core/master eeee738d1c faa6c3d44c --word-diff-regex=. |
|
Re-ACK faa6c3d |
faa6c3d net: Drop only invalid entries when reading banlist.json (MarcoFalke) Pull request description: 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 bitcoin#20966 (comment) ACKs for top commit: laanwj: Re-ACK faa6c3d Tree-SHA512: 5a58e7f1dcabf78d0c65d8c6d5d997063af1efeaa50ca7730fc00056fda7e0061b6f7a38907ea045fe667c9f61d392e01e556b425a95e6b126e3c41cd33deb83
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)