Page MenuHomePhabricator

Add logging for block header receive time
ClosedPublic

Authored by jasonbcox on May 23 2018, 05:44.

Details

Summary

Prototype implementation, for reference: D1196

Finishes implementation for T208

Some stats from my experiment with selfish mining over a period 753
blocks:

  • min time received compared to header timestamp: -26 sec (so the header timestamp was 26 seconds in the future. lol)
  • max time received compared to header timestamp: 166 sec
  • median: 0
  • mean: 3.34661
  • stddev: 11.896

No apparently selfish blocks were captured during this time period.

The selfish mining threshold is slightly under one standard deviation.
Let me know if you think this value should be larger, just keep in mind
that it's only used for breaking ties.

Test Plan

make check
and
./test_runner.py reindex.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sm1
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/net_processing.cpp:1CFMTCode style violation
Auto-Fixsrc/rpc/mining.cpp:1CFMTCode style violation
Auto-Fixsrc/test/miner_tests.cpp:1CFMTCode style violation
Auto-Fixsrc/validation.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 2617
Build 3348: Bitcoin ABC Buildbot (legacy)
Build 3347: arc lint + arc unit

Event Timeline

src/net_processing.cpp
2805

wildnewlineappeared

Mengerian added inline comments.
src/validation.cpp
3866

It seems like this calculation assumes that TimeDiffs will be positive. What about the case where chainTipTimeDiff is negative, this seems like a way the Selfish Miner could produce blocks that are always considered more honestly mined, just by future-dating blocks.

Shouldn't the calculation just compare the absolute values of the time differences?

Also, can you explain why chain work is considered? It's unclear to me if that makes sense.

deadalnix requested changes to this revision.May 24 2018, 16:45
deadalnix added inline comments.
src/net_processing.cpp
2510

blocktxn is send in response to a getblocktxn message, so this doesn't indicate the time at which the block was discovered.

2801

Samewise, block command are sent in response to a getblock, usually triggered by an inv or a header message.

src/validation.cpp
3226

Looks like the GetTime should happen here. This would simplify most of that patch and it is not like this piece of code is supposed to be stateless anyway.

This revision now requires changes to proceed.May 24 2018, 16:45
src/validation.cpp
3226

This is called by AcceptBlock, where the logic based on the time is done. It should probably be called there, and passed in here.

schancel requested changes to this revision.May 24 2018, 21:41
jasonbcox added inline comments.
src/chain.h
40

Note to self based on recent meeting: This threshold may be completely unnecessary. Think about it some more.

src/validation.cpp
3866

As discussed in the meeting, I'll think about the negative time diff vs abs()-ing it. You are correct that this could incentivize future timestamps for potentially selfish miners.

Comparing chainwork here is necessary because you can't compare receive times of blocks at different heights. Otherwise miners could be tricked into mining on chains with less work by manipulating timestamps. I can make that more clear in the comments.

jasonbcox marked 8 inline comments as done.

Cleaned up code according to feedback:

  • Removed the THRESHOLD as it was unnecessary.
  • Moved GetTime() call, as putting in net_processing was overkill and not implemented correctly anyway.
src/validation.cpp
3226

While I agree with adding it in AcceptBlock conceptually, this function is called in two places and so is AcceptBlockHeader which calls this function directly. To prevent implementing this on multiple code paths (with no apparent gain for doing so), I think it's best to implement it here for now. If future code is cleaner and there is a demonstrable reason for moving it into AcceptBlock (performance reasons, for example), then I'm fine doing that later.

Fixed segfault during reindexing and used llabs() instead of abs() to prevent overflows.

deadalnix added inline comments.
src/validation.cpp
3906 ↗(On Diff #4085)

This would benefit from being a member function of CBlockIndex and have a test for it.

Added member function to compute time diff

jasonbcox added inline comments.
src/chain.h
400 ↗(On Diff #4094)

I plan to write a test for this and other functions in this class as part of T355.

Mengerian added inline comments.
src/validation.cpp
3904 ↗(On Diff #4094)

Looks good to me, this algorithm makes more sense than the previous version.

matiu added inline comments.
src/validation.cpp
3906 ↗(On Diff #4094)

Maybe wrap 3905 to 3924 in

if (chainActive.Tip() != nullptr) {
   ...
}

for clarity?

Also, at 3926 we check chainActive.Tip with: if (chainActive.Tip()) and here with if (chainActive.Tip() != nullptr) we may want to unify for consistency.

This revision is now accepted and ready to land.Jun 14 2018, 00:46
src/validation.cpp
3906 ↗(On Diff #4094)

Good catch.

Simplified chainActive.Tip() checks

This revision was automatically updated to reflect the committed changes.