fix race that could fail to persist a ban #7959
Conversation
DumpBanList currently does this: - with lock: take a copy of the banmap - perform I/O (write out the banmap) - with lock: mark the banmap non-dirty If a new ban is added during the I/O operation, it may never be persisted to disk. Reorder operations so that the data to be persisted cannot be older than the time at which the banmap was marked non-dirty.
|
ut ACK f4ac02e |
| @@ -2634,9 +2634,10 @@ void DumpBanlist() | |||
|
|
|||
| CBanDB bandb; | |||
| banmap_t banmap; | |||
| CNode::SetBannedSetDirty(false); | |||
There was a problem hiding this comment.
What about doing the whole bandb.Write(ban map) & CNode::SetBannedSetDirty(false); while holding LOCK(cs_setBanned);?
Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).
There was a problem hiding this comment.
Otherwise there will always be a possible race condition that someone is adding a ban after CNode::SetBannedSetDirty(false); and before bandb.Write(banmap) or CNode::GetBanned(banmap).
That wouldn't be a race condition, but the way it's structured on purpose: because the change to the ban map will re-mark the map as dirty, the next write will catch it (there is a very slight window between CNode::SetBannedSetDirty and CNode::GetBanned in which a false-positive dirty could happen, but unlike the other way around that's harmless). This is not the case right now.
Putting it in a huge lock would hold up the entire P2P for the time of the I/O, which would be unfortunate.
|
utACK f4ac02e |
|
utACK f4ac02e |
f4ac02e fix race that could fail to persist a ban (Kaz Wesley)
banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 Part of #2074.
banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 Part of #2074.
banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 - bitcoin/bitcoin#13061 - Wasn't needed when I first made this backport in 2017, but it is now! Part of #2074.
banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 - bitcoin/bitcoin#13061 - Wasn't needed when I first made this backport in 2017, but it is now! Part of #2074.
DumpBanList currently does this:
If a new ban is added during the I/O operation, it may never be persisted to disk.
Reorder operations so that the data to be persisted cannot be older than the time at which the banmap was marked non-dirty.