Don't store or send blocks forked before last checkpoint.

ashleyholman commented at 1:17 pm on August 19, 2013: contributor

This is a proposed solution for the “Fingerprint via weak-chain block submission” issue (#2757).

Before storing a block, a check is done to see if it is at a height lower than last checkpoint. If so, it is not stored. (By definition, last checkpoint implies we have the whole chain up until last checkpoint, so we do not need to store any new blocks at those heights).

A similar check is done before serving a request for a block at a pre-checkpoint height. If it’s not in the main chain, it could be a fingerprint block. There is no point serving this block to anyone as it is a fork from the checkpointed chain, so the request is dropped.

Although fingerprinting would be possible by generating a block higher than last checkpoint, it would be economically prohibitive to do so.

ashleyholman commented at 1:33 pm on August 19, 2013: contributor

This is my first time doing a patch for bitcoin, so please scrutinise and tell me if I’ve done something wrong :)

Thanks

luke-jr commented at 2:43 pm on August 19, 2013: member

NACK, this makes Bitcoin centralized.

Edit: Nevermind, misread.

sipa commented at 2:48 pm on August 19, 2013: member

Isn’t there a rule already that prevents storing blocks before the last checkpoint?

Luke: if the rule was “Do not send blocks in a forked chain more than 2016 blocks behind the main chain”, would that also be ‘centralized’?

luke-jr commented at 2:55 pm on August 19, 2013: member

Nevermind, I misread this. Thought it was non-sending/storing pre-checkpoint blocks even in the main chain.

ashleyholman commented at 3:00 pm on August 19, 2013: contributor

There is no prevention to stop the storing of blocks prior to last checkpoint. The closest thing is a check to see if the block’s proof-of-work could possibly be valid given the time elapsed since last checkpoint, which allows for a worst case 4x dropoff in difficulty for every 2 weeks since the checkpoint. @luke-jr does this introduce any centralisation that’s not already there? See these lines that are already in the AcceptBlock() code:

0        // Check that the block chain matches the known block chain up to a checkpoint
1        if (!Checkpoints::CheckBlock(nHeight, hash))
2            return state.DoS(100, error("AcceptBlock() : rejected by checkpoint lock-in at %d", nHeight));

so any attempt to extend a fork prior to a checkpoint will hit up against that check anyway, and won’t be able to continue.

ashleyholman commented at 3:16 pm on August 19, 2013: contributor

I have done a test and confirmed that you can send a min difficulty block of height 1001 to a node who is on block 167000 and it will store it. For a fully synced node, it looks like you would need to generate a block of difficulty ~68000, and they would accept it at height 1001 also, but I haven’t generated a block that hard in order to test it. In a couple of weeks, it should only require 17000 difficulty to fingerprint an up-to-date node, so I could try to gen one on my GPU at that point and test.

ashleyholman commented at 3:19 pm on August 19, 2013: contributor

PS. another way to test this is to set your clock forward a few years, and then your client will happily store a min difficult block prior to checkpoints (if the block’s timestamp is also in the future).

sipa commented at 3:21 pm on August 19, 2013: member

Regarding storing blocks, that problem will disappear once we switch to headers-first syncing (where only blocks along the known best header chain are fetched anyway).

I have no objection to not serving pre-checkpoint sidechain blocks, though. I prefer decreasing the reliance on checkpoints, but that would mean we need to come up with a criterion that will eventually limit how deep reorgs the network supports; so for now, I think using the last checkpoint is fine.

I’ll review the code later.

jgarzik commented at 3:28 pm on August 19, 2013: contributor

+1 @sipa

ashleyholman commented at 3:54 pm on August 19, 2013: contributor

Something else that may be of relevance here: currently a node does not need to first request a block in order to store it. You can just connect to it and send a ‘block’ message and it will process it and store it as if it asked for it. Likewise, in response to a getdata request, you can send a different block and it will process it and store it as per normal, providing it passes the checks.

sipa commented at 6:22 pm on August 19, 2013: member

@ashleyholman It is relevant, and right now, that’s perhaps unwanted (though in case of a freshly-mined block, it is even distributed that way). But again headers-first sync will solve that, as any block received that is not part of the known best chain, is just dropped.

jgarzik commented at 2:36 am on August 25, 2013: contributor

ACK

in src/main.cpp: in 720d7a084d outdated

3237@@ -3233,10 +3238,27 @@ void static ProcessGetData(CNode* pfrom)
3238 
3239             if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK)
3240             {
3241-                // Send block from disk

sipa commented at 3:06 pm on August 25, 2013:

Any reason to remove this comment? EDIT: nevermind, it just moved.

in src/main.cpp: in 720d7a084d outdated

3246+                    // If the requested block is at a height below our last
3247+                    // checkpoint, only serve it if it's in the checkpointed chain
3248+                    int nHeight = ((*mi).second)->nHeight;
3249+                    CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(mapBlockIndex);
3250+                    if (pcheckpoint && nHeight < pcheckpoint->nHeight) {
3251+                       if (vBlockIndexByHeight[nHeight]->GetBlockHash() != ((*mi).second)->GetBlockHash())

sipa commented at 3:07 pm on August 25, 2013:

Use CBlockIndex::IsInMainChain() instead.

ashleyholman commented at 2:19 am on August 26, 2013: contributor

@sipa: patch now amended to use CBlockIndex::IsInMainChain()

ashleyholman commented at 4:35 pm on September 15, 2013: contributor

@sipa anything else you need from me on this one?

sipa commented at 11:30 pm on September 23, 2013: member

I’d like to postpone this until after headers-first is merged, as:

The problem with storing blocks will be solved in a very natural way then already (by only fetching blocks we already know we want)
My current headers-first patch (#2964) removes GetLastCheckpoint, and I’d like to keep it that way; we should reduce relying on checkpoints to the extent possible. However, there’s no need to use the checkpoints in this case. In headers-first, the best chain is always known, and we can simply refuse to serve non-main-chain blocks as soon as the main chain has N blocks more than the fork (with N=144, a day, for example).

wtogami commented at 9:25 pm on January 14, 2014: contributor

@sipa @laanwj If headers first isn’t going into 0.9, wouldn’t this PR be a good idea?

in src/main.cpp: in 898d96670b outdated

3237@@ -3233,10 +3238,27 @@ void static ProcessGetData(CNode* pfrom)
3238 
3239             if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK)
3240             {
3241-                // Send block from disk
3242+                bool send = true;

sipa commented at 9:30 pm on January 14, 2014:

Initialize as false, and drop the else case?

in src/main.cpp: in 898d96670b outdated

3243                 map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.find(inv.hash);
3244                 if (mi != mapBlockIndex.end())
3245                 {
3246+                    // If the requested block is at a height below our last
3247+                    // checkpoint, only serve it if it's in the checkpointed chain
3248+                    int nHeight = ((*mi).second)->nHeight;

sipa commented at 9:31 pm on January 14, 2014:

int nHeight = mi->second->nHeight;

in src/main.cpp: in 898d96670b outdated

3248+                    int nHeight = ((*mi).second)->nHeight;
3249+                    CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(mapBlockIndex);
3250+                    if (pcheckpoint && nHeight < pcheckpoint->nHeight) {
3251+                       if (!((*mi).second)->IsInMainChain())
3252+                       {
3253+                         printf("ProcessGetData(): ignoring request for old block that isn't in the main chain\n");

sipa commented at 9:31 pm on January 14, 2014:

Use LogPrintf() now, printf() has been demagicified.

in src/main.cpp: in 898d96670b outdated

3246+                    // If the requested block is at a height below our last
3247+                    // checkpoint, only serve it if it's in the checkpointed chain
3248+                    int nHeight = ((*mi).second)->nHeight;
3249+                    CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(mapBlockIndex);
3250+                    if (pcheckpoint && nHeight < pcheckpoint->nHeight) {
3251+                       if (!((*mi).second)->IsInMainChain())

sipa commented at 9:32 pm on January 14, 2014:

if (!chainActive.Contains(mi->second))

sipa commented at 9:33 pm on January 14, 2014: member

Sounds like a good idea; despite what github says, this needs rebasing. I’ve left some comments.

wtogami commented at 1:31 pm on January 17, 2014: contributor

@ashleyholman Are you still around?

jgarzik commented at 1:45 pm on January 17, 2014: contributor

+1, this remains interesting

ashleyholman commented at 6:48 pm on January 17, 2014: contributor

I am still around, but I no longer have my dev / test environment that I used to make this patch.

What needs to be done here - just to incorporate the feedback from sipa and provide a rebased patch?

I might be able to do that.

sipa commented at 6:53 pm on January 17, 2014: member

Yes, I’m fine with the changes here. It’s just that the code is outdated by now…

ashleyholman commented at 6:58 pm on January 17, 2014: contributor

@sipa when is it needed by? If you give me a few days I’ll get my VM set up again and properly test the new patch.

sipa commented at 6:58 pm on January 17, 2014: member

That’s fine. Thanks!

Don't store or send side-chain blocks lower than last checkpoint. d8b4b49667

ashleyholman commented at 0:25 am on January 20, 2014: contributor

Here is a new commit. It is updated to contain the changes requested by @sipa including initialising the send variable as false. This required adding 2 new else cases but I think it’s still better like that.

ashleyholman commented at 0:32 am on January 20, 2014: contributor

plus I tested it with the following tests: Test 1: Synced blockchain to height 1325. Sent a difficulty-1 fingerprint block at height 1001. Result: Both the patched and unpatched clients accepted and stored the block, and would send it upon request (since there is no last checkpoint) Test 2: Synced blockchain to height 28157 (past first checkpoint at 11111). Sent a difficulty-1 block at height 1001. Result: The unpatched client accepts and stores the block, and serves it upon request. The patched client rejects the block and eventually bans the peer if they keep trying this. Test 3: Synced the blockchain to height 12905 (past first checkpoint at 11111). Sent a difficulty-1 block to the unpatched client which was accepted as per Test 2. I then patched the client and requested the block. Result: Client ignores the request, and logs a debug message: ProcessGetData(): ignoring request for old block that isn’t in the main chain.

BitcoinPullTester commented at 0:55 am on January 20, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d8b4b49667f3eaf5ac16c218aaba2136ece907d8 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

ashleyholman commented at 2:30 am on February 1, 2014: contributor

Hi. What’s happening with this? Still not merged?

jgarzik commented at 2:32 am on February 1, 2014: contributor

ACK

sipa commented at 10:43 am on February 1, 2014: member

ACK

laanwj referenced this in commit 76a77059f3 on Feb 20, 2014

laanwj merged this on Feb 20, 2014

laanwj closed this on Feb 20, 2014

IntegralTeam referenced this in commit 040abafe3a on Jun 4, 2019

DrahtBot locked this on Sep 8, 2021

Don’t store or send blocks forked before last checkpoint. #2910