Page MenuHomePhabricator

Defaults block index received time to 0 when init from a block header
ClosedPublic

Authored by Fabien on Nov 25 2018, 22:38.

Details

Summary

Current behavior is to set received time to the block header time. This
prevents any possibility to check if received time has been set due to
effective reception of a new block or because of a block loading from
file.

Test Plan
make check && ./test/functionnal/test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien added a child revision: Restricted Differential Revision.Nov 25 2018, 22:40
This revision is now accepted and ready to land.Nov 25 2018, 23:15
jasonbcox requested changes to this revision.Nov 26 2018, 21:11
jasonbcox added a subscriber: jasonbcox.

Why is knowing when the header was loaded from a file useful? The block time is the next-best accurate time we have when the actual received time is not available.

I'd also argue that if you're loading the header from a file, the received time is no longer useful, which is why it was never saved in the first place.

This revision now requires changes to proceed.Nov 26 2018, 21:11

Your second sentence is what this diff is about.

If loaded from file, the receive time is no longer needed ; but because it is set to the block header time there is no way to know if you need to care about the received time or not.
The block header time is actually a valid header receive time.

If you need to approximate the received time by the block header time it would be easy to check the later against 0 to determine which one is to be trusted.

src/chain.h
179 ↗(On Diff #6093)

Considering your latest comments, what do you think about this?

int64_t GetHeaderReceivedTime(bool guessUnknownTime=false) const {
if (guessUnknownTime && nTimeReceived == 0) {
    return GetBlockTime();
}

return nTimeReceived;
}

Updated as per suggestion

@jasonbcox I defaulted guessUnknownTime to true, so it requires no change on existing code to get the same behavior as prior

jasonbcox added inline comments.
src/test/blockindex_tests.cpp
126 ↗(On Diff #6116)

guessUnkwnowTime -> guessUnknownTime

This revision is now accepted and ready to land.Nov 27 2018, 00:20
deadalnix added inline comments.
src/chain.h
175 ↗(On Diff #6116)

Is there any callsite that rely on time being non zero ?

deadalnix requested changes to this revision.Nov 27 2018, 00:30
deadalnix added inline comments.
src/chain.h
175 ↗(On Diff #6116)

If none rely on this then just refactor. I don't think there are many callsites so it is worth investigating.

This revision now requires changes to proceed.Nov 27 2018, 00:30
src/chain.h
175 ↗(On Diff #6116)

Not currently. Fabien and Shae were experimenting with it in other diffs. I figure making this change allows us to continue experiments until the matter of penalizing blocks based on received time is settled.

Fabien marked an inline comment as done.Nov 27 2018, 10:46
Fabien added inline comments.
src/chain.h
175 ↗(On Diff #6116)

I found a single case where the behaviour was changed when returning 0: in the GetReceivedTimeDiff function below which is called in AcceptBlock for logging only.
Intended that it has a very low impact, I found that the solution proposed by Jason was costless and better than just refactoring.
As mentionned, the ability to return 0 is used in another diff.

deadalnix requested changes to this revision.Nov 28 2018, 17:20
deadalnix added inline comments.
src/chain.h
175 ↗(On Diff #6116)

Adding bool parameter is an antipattern so clearly we are not talking about anything costless here, but actually making the code worse. If there is no reason to make the code worse, then just refactor this and the one callsite.

This revision now requires changes to proceed.Nov 28 2018, 17:20

Remove the boolean parameter as per review

This revision is now accepted and ready to land.Nov 29 2018, 00:14
This revision was automatically updated to reflect the committed changes.