Page MenuHomePhabricator

Discourage selfish mining by favoring blocks with more accurate timestamps
AbandonedPublic

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
sm3
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/net_processing.cpp:1CFMTCode style violation
Auto-Fixsrc/validation.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 2618
Build 3350: Bitcoin ABC Buildbot (legacy)
Build 3349: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

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.

src/net_processing.cpp
923 ↗(On Diff #4149)

Fair. I'll remove it.

src/validation.cpp
116 ↗(On Diff #4149)
jasonbcox added inline comments.
src/net_processing.cpp
923 ↗(On Diff #4149)

Cleaned up according to feedback. Added more tests.

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 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 marked 2 inline comments as done and an inline comment as not done.

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

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.

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 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 marked an inline comment as done.

Using comparator

jasonbcox marked 9 inline comments as done and 2 inline comments as done.
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 added inline comments.
src/validation.cpp
3541 ↗(On Diff #5505)

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

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.

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.

With regards to this comment and others I've heard from miners, it's clear to me that a change like this has much more risk than initially thought and does not provide enough benefit to offset that risk. This change is also not being maintained at all, so I think it's time to abandon it.