Write fee estimate and peers files only when initialized #4942

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2014_09_dont_nuke_datadir changing 2 files +29 −18

laanwj commented at 12:21 PM on September 18, 2014: member

Fixes #4669 (see discussion there).

Also move the loading of addresses to StartNode() to make it more self-contained.

Write fee estimate and peers files only when initialized

Fixes #4669.

Move the loading of addresses to StartNode() to make it more
self-contained.

94064710b9

in src/net.cpp:None in 94064710b9
```
1739 | @@ -1739,6 +1740,18 @@ void static Discover(boost::thread_group& threadGroup)
1740 |  
1741 |  void StartNode(boost::thread_group& threadGroup)
1742 |  {
1743 | +    uiInterface.InitMessage(_("Loading addresses..."));
```
Diapolo commented at 12:26 PM on September 18, 2014:

Feels weird to have an init message in net.cpp, just saying.

laanwj commented at 12:28 PM on September 18, 2014:

Not strange at all - StartNode is part of the initialization process. This is bound to happen as we modularize. In time I expect all init messages to be in modules.
Diapolo commented at 12:31 PM on September 18, 2014: none

Are you sure this fixes the problem with peers.dat? StartNode() is still starting the DumpAddresses()-thread and inside that function there is still no check, like the one I proposed in #4784.
laanwj commented at 12:36 PM on September 18, 2014: member

StartNode is not called in the case that there is a lock file, as AppInit2 stops partway!
in src/init.cpp:None in 94064710b9
```
1062 | @@ -1056,6 +1063,7 @@ bool AppInit2(boost::thread_group& threadGroup)
1063 |      // Allowed to fail as this file IS missing on first startup.
1064 |      if (est_filein)
1065 |          mempool.ReadFeeEstimates(est_filein);
1066 | +    fFeeEstimatesInitialized = true;
```
Diapolo commented at 12:36 PM on September 18, 2014:

That fee file isn't created here, so that can't be the place where we should set init to true?

laanwj commented at 12:38 PM on September 18, 2014:

It doesn't need to be created here. We just need the flag to know whether we reached this stage in the initialization process. If we did, it is valid to write the file on shutdown.

Diapolo commented at 2:00 PM on September 18, 2014:

Alright, but at least the variable name is missleading? At this stage we could have a missing file, an up-version file or some serialisation error, which would make me think fFeeEstimatesInitialized shouldn't be true. Perhaps fWriteFeeEstimates is better.

laanwj commented at 2:07 PM on September 18, 2014:

The point is that it doesn't matter which of those are the case. We choose to ignore read errors. If the file is invalid or missing, it's fine to write an empty file. It's just not fine to write an empty file when initialization failed.
in src/init.cpp:None in 94064710b9
```
 142 | @@ -137,6 +143,7 @@ void Shutdown()
 143 |              mempool.WriteFeeEstimates(est_fileout);
```
Diapolo commented at 12:37 PM on September 18, 2014:

IMHO that call creates the file.

laanwj commented at 12:40 PM on September 18, 2014:

Which is inside if (fFeeEstimatesInitialized) now right? So if the fee estimates were never read (or deemed missing and initialized to empty), they won't be written either.

Diapolo commented at 2:00 PM on September 18, 2014:

Correct!
laanwj added the label Bug on Sep 18, 2014
Diapolo commented at 12:47 PM on September 18, 2014: none

@laanwj If StartNode() get's called we start the thread for DumpAddresses(), right? So we end up in the same situation in a case where the client is running for DUMP_ADDRESSES_INTERVAL * 1000, if addrman has 0 addresses.
laanwj commented at 12:50 PM on September 18, 2014: member

IF StartNode() was called, it is valid for the shutdown (or the DumpAddresses thread) to write the addresses file, no matter how many addresses are in it.

This is an initialization sequence fix: It makes sure that if the lock file is missing, or AppInit2() exits in some other way before addresses list is initialized, it won't write an empty file on shutdown (and StopNode),
BitcoinPullTester commented at 12:54 PM on September 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4942_94064710b9123dfb3df8cfd6c32efae349aec281/ 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.
Diapolo commented at 2:02 PM on September 18, 2014: none

@laanwj But wouldn't it also make sense to prevent overwriting with 0 addresses, even if the init stage was processed correctly?
laanwj commented at 2:04 PM on September 18, 2014: member

@Diapolo Writing 0 addresses is fine in principle. It's only an error if the previous addresses weren't read, or we're not allowed to write to the data directory at all.
sipa commented at 2:21 AM on October 2, 2014: member

utACK. I guess at some point we want the fee data to move from globals into an encapsulated state, which only exists when it was correctly loaded. But this is already an improvement.
laanwj commented at 7:19 AM on October 2, 2014: member

@sipa Agreed on that, eventually it should be an encapsulated structure that is instantiated/destroyed instead of having code directly in AppInit2()/ShutDown().
jgarzik commented at 3:11 PM on October 13, 2014: contributor

ut ACK
laanwj merged this on Oct 14, 2014
laanwj closed this on Oct 14, 2014
laanwj referenced this in commit 992ab87114 on Oct 14, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

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