Update ban-state in case of dirty-state during periodic sweep #11616

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2017/11/qt_ban changing 1 files +19 −12

jonasschnelli commented at 2:45 AM on November 6, 2017: contributor

We do currently not update the UI during periodic ban list sweeps (via dump banlist). Fixes #11612
jonasschnelli added the label GUI on Nov 6, 2017
ghost commented at 3:14 AM on November 6, 2017: none
Great! Just a nit: According to the Developer Notes

If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.

shouldn't it be written like
```
if(clientInterface) clientInterface->BannedListChanged();
```
or
```
if(clientInterface) {
    clientInterface->BannedListChanged();
}
```
?
Update ban-state in case of dirty-state during periodic sweep c8538123a7
in src/net.cpp:471 in 12c9401a61 outdated
```
 467 | @@ -468,6 +468,10 @@ void CConnman::DumpBanlist()
 468 |          SetBannedSetDirty(false);
 469 |      }
 470 |  
 471 | +    // update UI
```
laanwj commented at 9:22 AM on November 6, 2017:

I think an appropriate place for this would be in SweepBanned() itself (at the end, if anything changed), as the list is changed there. It just happens to be called from here, but there are other places.

theuni commented at 7:03 PM on November 7, 2017:

Agree.
jonasschnelli force-pushed on Nov 9, 2017
jonasschnelli commented at 7:08 PM on November 9, 2017: contributor

Moved the signal call to SweepBanned().
jonasschnelli commented at 8:52 PM on November 15, 2017: contributor

Travis is passing now (random issue)

in src/net.cpp:618 in c8538123a7 outdated

 611 | @@ -612,6 +612,11 @@ void CConnman::SweepBanned()
 612 |          else
 613 |              ++it;
 614 |      }
 615 | +
 616 | +    // update UI
 617 | +    if(setBannedIsDirty && clientInterface) {
 618 | +        clientInterface->BannedListChanged();

theuni commented at 11:17 PM on November 15, 2017:

This needs to be called without cs_setBanned.

theuni commented at 11:19 PM on November 15, 2017: member

looks good after scoping to avoid holding the lock during the callback.
jonasschnelli commented at 7:41 AM on November 19, 2017: contributor

Thanks @theuni for the review. lock-de-scoped the signal call part, switched setBannedIsDirty to an atomic. Best reviewed with ?w=1
theuni commented at 3:39 PM on November 19, 2017: member

@jonasschnelli making setBannedIsDirty atomic doesn't work, i'm afraid, as setBanned/setBannedIsDirty must remain in sync. See CConnman::Unban for an example of where an atomic setBannedIsDirty could go out of sync with setBanned if it's unset elsewhere without cs_setBanned.
Call BannedListChanged outside of cs_setBanned lock 57ac471a29
jonasschnelli force-pushed on Nov 19, 2017
jonasschnelli commented at 10:52 PM on November 19, 2017: contributor

@theuni. Thanks. Your right. Just change the approach.
theuni approved
theuni commented at 6:37 PM on November 20, 2017: member

Looks good, thanks! utACK 57ac471a294fc7039140eed91d217ad1af7fa7af
laanwj commented at 9:43 AM on December 15, 2017: member

utACK 57ac471
laanwj merged this on Dec 15, 2017
laanwj closed this on Dec 15, 2017
laanwj referenced this in commit 8585bb8f05 on Dec 15, 2017
PastaPastaPasta referenced this in commit 3526ef9fda on Jan 17, 2020
PastaPastaPasta referenced this in commit b117c2f418 on Jan 22, 2020
PastaPastaPasta referenced this in commit 127fef465c on Jan 22, 2020
PastaPastaPasta referenced this in commit e2ba114f87 on Jan 29, 2020
PastaPastaPasta referenced this in commit a648022eaf on Jan 29, 2020
PastaPastaPasta referenced this in commit e1cd0f2a85 on Jan 29, 2020
PastaPastaPasta referenced this in commit b9e3ae689a on Jan 31, 2020
ckti referenced this in commit f79f3d9b99 on Mar 28, 2021
gades referenced this in commit dac710680f on Jun 25, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11612 Peer is still banned, allthough ban endtime has been reached