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.
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.
2633 | @@ -2634,9 +2634,10 @@ void DumpBanlist() 2634 | 2635 | CBanDB bandb; 2636 | banmap_t banmap; 2637 | + CNode::SetBannedSetDirty(false);
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).
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 f4ac02ee7c6530c273503d8575a492e9b2ac1f13