Quote from: goatpig on November 22, 2018, 02:17:24 PM

As seen in the code you linked to, the style is actually confusing: the semantics do not outright suggest ReadLastBlockFile takes a reference, as it is not being checked for a status return. This kind of semantics suggest you are setting some internal value for object pblocktree instead. The following snippet would be a little easier to read:

Code:

int result = pblocktree->ReadLastBlockFile(nLastBlockFile)
if(result != 0)
{
//error handling
}

//do something with nLastBlockFile

Right. Even better would be to create a local variable to indicate things.

Code:

int storedLastBlockFile;
int result = pblocktree->ReadLastBlockFile(storedLastBlockFile)
if(result != 0)
{
//error handling
}

nLastBlockFile = storedLastBlockFile;

He kind of did that, since the new variable is called loadedblockfile, it just isn't used to update the global variable.

Quote

With that in mind, the change in BU could very well be a mistake that led to a benign change in behavior, assuming the author of the change did not bother to acquire a broad understanding of this code before modifying it. He possibly saw a global variable changed by a local object and figured that's bad style, correcting for style without caring for the substance.

I agree that this was possibly overlooked. Having a pointer to the last file doesn't really do much if it isn't persisted over restarts.

He may have decided that it was more efficient to fill in the free spaces. This change would make pruning less efficient too. Ideally, each block should have a set of files from approx the same time, so they can be deleted roughly in order.

I wonder if the BU node writes to blk00000.dat on each restart if pruning is enabled.