Details

Reviewers

deadalnix
Mengerian

Group Reviewers

Restricted Project

Commits

rSTAGINGebff5f83feac: Add logging for block header receive time
rABCebff5f83feac: Add logging for block header receive time

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.May 23 2018, 05:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 23 2018, 05:44

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

jasonbcox added a child revision: D1442: Discourage selfish mining by favoring blocks with more accurate timestamps.May 23 2018, 06:01

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2617: Diff 3940!May 23 2018, 06:21

schancel added inline comments.May 23 2018, 16:05

src/net_processing.cpp
2805 ↗	(On Diff #3940)

Mengerian added a subscriber: Mengerian.May 23 2018, 17:22

Mengerian added inline comments.

src/validation.cpp
3866 ↗	(On Diff #3940)	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 ↗	(On Diff #3940)	blocktxn is send in response to a getblocktxn message, so this doesn't indicate the time at which the block was discovered.
2801 ↗	(On Diff #3940)	Samewise, block command are sent in response to a getblock, usually triggered by an inv or a header message.
src/validation.cpp
3226 ↗	(On Diff #3940)	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

schancel added inline comments.May 24 2018, 21:41

src/validation.cpp
3226 ↗	(On Diff #3940)	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 planned changes to this revision.May 30 2018, 16:30

jasonbcox added inline comments.

src/chain.h
40 ↗	(On Diff #3940)	Note to self based on recent meeting: This threshold may be completely unnecessary. Think about it some more.
src/validation.cpp
3866 ↗	(On Diff #3940)	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.

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.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

jasonbcox added inline comments.Jun 6 2018, 22:53

src/validation.cpp
3226 ↗	(On Diff #3940)	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.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2710: Diff 4077!Jun 6 2018, 23:26

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

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2718: Diff 4085.Jun 7 2018, 19:14

deadalnix accepted this revision.Jun 8 2018, 14:52

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 marked 2 inline comments as done.Jun 8 2018, 15:17

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.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2721: Diff 4094.Jun 8 2018, 15:55

Mengerian accepted this revision.Jun 8 2018, 19:35

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 a subscriber: matiu.Jun 12 2018, 14:28

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.