Page MenuHomePhabricator

Discourage selfish mining by favoring blocks with more accurate timestamps
Changes PlannedPublic

Authored by jasonbcox on May 23 2018, 06:01.

Details

Reviewers
deadalnix
schancel
Group Reviewers
Restricted Project
Summary

Prototype implementation, for reference: D1196

Implements changes for T207, T209

Depends on D1441
Depends on D1555

Test Plan

make check && test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1442
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3668
Build 5411: Bitcoin ABC Teamcity Staging
Build 5410: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jun 30 2018, 14:40
jasonbcox planned changes to this revision.Jul 3 2018, 20:40
jasonbcox added inline comments.
src/validation.cpp
116 ↗(On Diff #4149)

Looks like this entire function isn't tested anywhere! I'll write up some tests for it before continuing with this diff.

3945 ↗(On Diff #4149)

Sounds good. I'll go with chainTipShouldUpdate.

jasonbcox added inline comments.Jul 3 2018, 21:02
src/validation.cpp
4042 ↗(On Diff #4149)

Good catch. This was necessary in prior versions before you suggested I make modifications to the work comparator. It's no longer necessary.

jasonbcox added inline comments.Jul 3 2018, 21:10
src/net_processing.cpp
923 ↗(On Diff #4149)

Fair. I'll remove it.

jasonbcox added inline comments.Jul 10 2018, 18:43
src/validation.cpp
116 ↗(On Diff #4149)
jasonbcox marked 7 inline comments as done.Jul 10 2018, 18:56
jasonbcox marked 3 inline comments as done.Jul 10 2018, 21:44
jasonbcox added inline comments.
src/net_processing.cpp
923 ↗(On Diff #4149)
jasonbcox updated this revision to Diff 4264.Jul 10 2018, 21:45

Cleaned up according to feedback. Added more tests.

jasonbcox edited the summary of this revision. (Show Details)Jul 10 2018, 21:46
deadalnix requested changes to this revision.Jul 16 2018, 14:43
deadalnix added inline comments.
src/validation.cpp
3887 ↗(On Diff #4264)

Please keep fHasMoreWork and then you can add:

bool chainTipShouldUpdate = isSameHeightAndMoreHonestlyMined || fHasMoreWork;

Keeping names for intermediate variable makes the code more readable.

3922 ↗(On Diff #4264)

This comment is invalid now.

3944 ↗(On Diff #4264)

Considering this doesn't trigger when fHasMoreWork is true, then what ensures a reorg happens ? Is calling NewPoWValidBlock really the right thing to do ? After all, the chain we want to pick doesn't have more PoW behing it, just more accurate timestamps.

test/functional/sendheaders-selfish-mining.py
1 ↗(On Diff #4264)

The whole test is very confusing because way too much focused on selfish mining and not ont he behavior is needs to test, namely that more accurate timestamp are preferred. You should rewrite it in term of block with accurate/inaccurate timestamp. Tests cases:

1/

  • node receive a block with accurate timestamp.
  • node receive a second block with inaccurate timestamp.
  • check that node select the first one as tip.
  • inaccurate chain is extended.
  • node switch to the other chain.

2/

  • node receive a block with inaccurate timestamp.
  • node select that block as tip anyway.
  • node receive block with accurate timestamp.
  • node switch the the chain with accurate timestamp.

3/ Ensure that 1 and 2 also work when the fork is more than one level deep.

It relies on sleep which is generally a bad idea, especially since it's fairly easy to spoof timestamps and mock the timestamp of the node.

43 ↗(On Diff #4264)

Please give it a more descriptive name. You already boostrapped 2 nodes and this is the 3rd one, so surely, it's something more specific than just THE node.

75 ↗(On Diff #4264)

You cannot really rely on this. But you can setmocktime the node. You can also abuse the time difference (for instance 1h) to make the whole thing very reliable.

81 ↗(On Diff #4264)

Please assign this before using it. It makes the code easier to follow.

This revision now requires changes to proceed.Jul 16 2018, 14:43
jasonbcox planned changes to this revision.Sep 20 2018, 22:03
jasonbcox marked 3 inline comments as done and an inline comment as not done.

Still need to fix the test, but this is currently deep in my backlog. Will get to it whenever I can.

src/validation.cpp
3922 ↗(On Diff #4264)

It's still valid because less-work is < not <=. It would have been more accurate in the past if it said same-or-less work.

3944 ↗(On Diff #4264)

I think the naming here is confusing. This hooks into the net_processing code that announces new blocks. If this node receives a more honest block, I think it makes sense to announce that block ASAP or else we're encouraging the honest block to be orphaned by not sharing it with the network as fast as possible.

3948 ↗(On Diff #4149)

This doesn't seem right. Why show this log message for regular chaintip updates? This log message is only intended for when an apparent selfish mining operation is occurring.

jasonbcox updated this revision to Diff 4979.Sep 20 2018, 22:26
jasonbcox marked 2 inline comments as done and an inline comment as not done.

Rebased and updated according to code feedback. Test still needs rewriting.

jasonbcox planned changes to this revision.Sep 20 2018, 22:27
jasonbcox updated this revision to Diff 5450.Oct 20 2018, 00:44

Fixed according to feedback, except Test 1.b fails. It really should not or could encourage a chain split. I'm yet sure why the longest PoW on the previously less-accurate timestamped block is not overtaking the less PoW chain.

jasonbcox planned changes to this revision.Oct 20 2018, 02:24
jasonbcox updated this revision to Diff 5502.Oct 22 2018, 20:46

Fixed failing test

deadalnix requested changes to this revision.Oct 23 2018, 00:36
deadalnix added inline comments.
src/validation.cpp
3506 ↗(On Diff #5505)

Sounds like you should use the comparator itself.

3511 ↗(On Diff #5505)

The reorg will be in the log anyways.

3541 ↗(On Diff #5505)

My understanding is that this is a simple optimization. The code would work just as fine without it, but if there is no reason to consider the block for reorg, then we can stop here and reconsider later. While this is the correct course of action, however it is not clear to me what triggers the downloads and/or redownload of blocks. If the code doesn't work when this is left alone, then this is mostly likely indication that something is lurking deeper.

3569 ↗(On Diff #5505)

Why is that necessary ? It looks like it is changing the semantic completely.

This revision now requires changes to proceed.Oct 23 2018, 00:36
jasonbcox planned changes to this revision.Oct 23 2018, 17:16
jasonbcox marked 4 inline comments as done.

3/ Ensure that 1 and 2 also work when the fork is more than one level deep.

Addressing this since it isn't part of the tests: I did not intend for this to work beyond 1 block deep.

src/validation.cpp
3506 ↗(On Diff #5505)

Good idea.

3511 ↗(On Diff #5505)

While true, I thought it was important to indicate why, since this is a rarer case than normal.

3541 ↗(On Diff #5505)

iirc it does work without this change. I'll test again and confirm.

3569 ↗(On Diff #5505)

While not strictly necessary, I figured broadcasting selfish-looking blocks would not be useful. To be clear, it does change the semantic.

jasonbcox updated this revision to Diff 5516.Oct 23 2018, 21:23
jasonbcox marked an inline comment as done.

Using comparator

jasonbcox planned changes to this revision.Oct 23 2018, 21:25
jasonbcox marked 9 inline comments as done and 2 inline comments as done.
deadalnix added inline comments.Oct 23 2018, 23:41
src/validation.cpp
3541 ↗(On Diff #5505)

Which means there is a problem somewhere that needs to be identified.

deadalnix requested changes to this revision.Oct 23 2018, 23:42
deadalnix added inline comments.
src/validation.cpp
3569 ↗(On Diff #5505)

It will boradcast all block that do not immediately build on the tip now.

jasonbcox marked 4 inline comments as done.Oct 24 2018, 21:32
jasonbcox added inline comments.
src/validation.cpp
3541 ↗(On Diff #5505)

p2p-fullblocktest fails without this change, so I'll keep it as is.

jasonbcox edited the test plan for this revision. (Show Details)Oct 24 2018, 21:32
jasonbcox marked an inline comment as done.Oct 26 2018, 17:48
jasonbcox added inline comments.
src/validation.cpp
3569 ↗(On Diff #5505)

The comment above this suggests the opposite. NewPoWValidBlock(), which broadcasts compact blocks to peers, is only called for more timestamp-accurate or longer chainwork blocks.

Dealing with flaky tests... This change will likely depend on the follow in order to have high confidence:
T446
T447
T452
T454

Regarding this: I noticed in a recent reddit discussion that ProHashing does not use accurate timestamps: https://www.reddit.com/r/btc/comments/bqy05r/sensible_defaults_and_unintended_consequences_of/eo9f9fa/

If this is seriously planned to be landed, then it would be good to actually talk to miners and see what they think.