prevent overwriting peers.dat if we have 0 addresses #4784

pull Diapolo wants to merge 2 commits into bitcoin:master from Diapolo:fix_dumpaddresses changing 3 files +10 −5

Diapolo commented at 9:32 PM on August 28, 2014: none
- fixes #4669
jgarzik commented at 9:35 PM on August 28, 2014: contributor
1. This is only a partial fix for #4669 because as noted everything is flushed in the case where the lockfile is present. This solves one symptom of the larger problem.
2. whitespace after "if
jgarzik commented at 9:37 PM on August 28, 2014: contributor
1. Your solution is generally OK
2. addrman.size() will never be negative
3. Hey, that would make a great addrman cleanup, wouldn't it? :)
Diapolo commented at 9:47 PM on August 28, 2014: none

@jgarzik 1./3. Yeah, I intended to not fix the larger problem with the lockfile, but the specific one mentioned. 2. Will update, thanks for catching it :). 4. But we return an int for the size so isn't it better to leave it that way? 5. Do you want me to make size an unsigned integer or change the commit-msg ^^, don't understand.
jgarzik commented at 10:46 PM on August 28, 2014: contributor

@Diapolo addrman's size() should return 'unsigned int' (technically size_type, what std::vector<>::size() returns) And in doing so, that makes testing addrman.size() less-than-zero incorrect, enabling removal of that case (becomes not-zero test).
prevent overwriting peers.dat if we have 0 addresses
- fixes #4669
f56aa076ff
change addrman::size() to return size_t 4985aaa43a
Diapolo commented at 9:51 AM on August 29, 2014: none

@jgarzik Can you re-check this pull?
BitcoinPullTester commented at 10:05 AM on August 29, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4784_4985aaa43a5a4762db3f16037c2da43aad3ff21f/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
in src/net.cpp:None in 4985aaa43a
```
1262 | @@ -1263,13 +1263,17 @@ void ThreadDNSAddressSeed()
1263 |  
1264 |  void DumpAddresses()
1265 |  {
1266 | +    // Prevent overwriting peers.dat in case addrman is empty
```
laanwj commented at 3:28 AM on August 30, 2014:

Now you use addrman.size()==0 implicitly to determine whether to write the list at exit. It works, but could lead to strange corner case if we actually need to write an empty list for some reason in the future.

It would lead to more understandable code to explicitly keep track of whether we reached the stage of initialization where addrman was initialized.

A more general solution would be to keep track (in net.cpp) whether StartNode was called. If not, don't do any cleanup in StopNode (which includes the call to DumpAddresses that is the culprit here).

Diapolo commented at 12:33 PM on September 18, 2014:

@laanwj I think DumpAddresses could carry a flag bool fForce for example, to force re-creation of that file instead.
laanwj commented at 12:23 PM on September 18, 2014: member

Closing in favor of #4942.
laanwj closed this on Sep 18, 2014
Diapolo deleted the branch on Mar 21, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4669 peers.dat easily wiped out after unclean shutdown/hasty relaunch