Page MenuHomePhabricator

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

Authored by Fabien on Sun, Nov 25, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Sun, Nov 25, 22:38
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Nov 25, 22:38
Fabien added a child revision: Restricted Differential Revision.Sun, Nov 25, 22:40
schancel accepted this revision.Sun, Nov 25, 23:15
This revision is now accepted and ready to land.Sun, Nov 25, 23:15
jasonbcox requested changes to this revision.Mon, Nov 26, 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.Mon, Nov 26, 21:11
Fabien added a comment.Mon, Nov 26, 21:39

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.

jasonbcox added inline comments.Mon, Nov 26, 21:51
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;
}
Fabien updated this revision to Diff 6116.Mon, Nov 26, 22:52

Updated as per suggestion

Fabien added a comment.Mon, Nov 26, 22:54

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

jasonbcox accepted this revision.Tue, Nov 27, 00:20
jasonbcox added inline comments.
src/test/blockindex_tests.cpp
126 ↗(On Diff #6116)

guessUnkwnowTime -> guessUnknownTime

This revision is now accepted and ready to land.Tue, Nov 27, 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.Tue, Nov 27, 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.Tue, Nov 27, 00:30
jasonbcox added inline comments.Tue, Nov 27, 01:07
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.Tue, Nov 27, 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.

Fabien updated this revision to Diff 6126.Tue, Nov 27, 10:47

Fix typo in comment

deadalnix requested changes to this revision.Wed, Nov 28, 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.Wed, Nov 28, 17:20
Fabien updated this revision to Diff 6155.Wed, Nov 28, 19:02

Remove the boolean parameter as per review

schancel accepted this revision.Wed, Nov 28, 21:11
deadalnix accepted this revision.Thu, Nov 29, 00:14
This revision is now accepted and ready to land.Thu, Nov 29, 00:14
This revision was automatically updated to reflect the committed changes.