Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Dec 15, 2017
Merged

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

merged 2 commits into from Dec 15, 2017

Conversation

jonasschnelli
Copy link
Contributor

We do currently not update the UI during periodic ban list sweeps (via dump banlist).
Fixes #11612

@ghost
Copy link

ghost commented Nov 6, 2017

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();
}

?

laanwj
laanwj reviewed Nov 6, 2017
View reviewed changes
src/net.cpp Outdated
@@ -468,6 +468,10 @@ void CConnman::DumpBanlist()
SetBannedSetDirty(false);
}

// update UI
Copy link
Member

@laanwj laanwj Nov 6, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

@jonasschnelli
Copy link
Contributor Author

Moved the signal call to SweepBanned().

@jonasschnelli
Copy link
Contributor Author

Travis is passing now (random issue)

theuni
theuni reviewed Nov 15, 2017
View reviewed changes
src/net.cpp Outdated

// update UI
if(setBannedIsDirty && clientInterface) {
clientInterface->BannedListChanged();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be called without cs_setBanned.

@theuni
Copy link
Member

theuni commented Nov 15, 2017

looks good after scoping to avoid holding the lock during the callback.

@jonasschnelli
Copy link
Contributor Author

Thanks @theuni for the review.
lock-de-scoped the signal call part, switched setBannedIsDirty to an atomic.
Best reviewed with ?w=1

@theuni
Copy link
Member

theuni commented Nov 19, 2017 •
edited

@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.

@jonasschnelli
Copy link
Contributor Author

@theuni. Thanks. Your right. Just change the approach.

theuni
theuni approved these changes Nov 20, 2017
View reviewed changes
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! utACK 57ac471

@laanwj
Copy link
Member

laanwj commented Dec 15, 2017

utACK 57ac471

@laanwj laanwj merged commit 57ac471 into bitcoin:master Dec 15, 2017
laanwj added a commit that referenced this pull request Dec 15, 2017
@laanwj
Merge #11616: Update ban-state in case of dirty-state during periodic…
8585bb8 
… sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes #11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
3526ef9 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
b117c2f 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
127fef4 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
e2ba114 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
a648022 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
e1cd0f2 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
b9e3ae6 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 25, 2021
@laanwj
Merge bitcoin#11616: Update ban-state in case of dirty-state during p…
dac7106 
…eriodic sweep

57ac471 Call BannedListChanged outside of cs_setBanned lock (Jonas Schnelli)
c853812 Update ban-state in case of dirty-state during periodic sweep (Jonas Schnelli)

Pull request description:

  We do currently not update the UI during periodic ban list sweeps (via dump banlist).
  Fixes bitcoin#11612

Tree-SHA512: bffbdcc03c63042177bdd511b0a9187c211c2b5011178481e8ee3e43a71eef1e4cd6b72f73672babab142b644f62f8b56f0aac1d26d3f19372b1f8644fec9395
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@theuni theuni theuni approved these changes

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Peer is still banned, allthough ban endtime has been reached
3 participants
@jonasschnelli @theuni @laanwj