block validate height

[zgfzgf]

fix #4090
Node evil
lotus chain list
... ...
7: (Sep 29 15:26:54) [ bafy2bzaceaphw3y7mv557lhpf2k3wmmbnfvr52r7zsc2qogitokfafpxq6jnw: t01000, ]
8: (Sep 29 15:26:58) [ bafy2bzaced577iipshzvvwpyn2kmpaqo3jrjxrkfauylq7wavhrqpkwdnu5e4: t01000, ]
9: (Sep 29 15:27:02) [ bafy2bzacecvw64pu527dh7554chhid2bmklffefvxu77dklt4aafgdghwm45i: t01000, ]
10: (Sep 29 15:27:06) [ bafy2bzacebscylcsnsecx7ct2ivzr6wkzd732o6tmdmukmtyf6xb7ewlxy63k: t01000, ]
10: (Sep 29 15:27:06) [ bafy2bzacebyt52vq3nqoo4pbd2xovdwutojqtthqtefbwemmmezbm4csvjlhk: t01000, ]
10: (Sep 29 15:27:06) [ bafy2bzacecgf3sfhjylbe7jp6n47jes7xb67clpeuj6im5rfevg6szsl2vkni: t01000, ]

Normal node
lotus chain list
... ...
7: (Sep 29 15:26:54) [ bafy2bzaceaphw3y7mv557lhpf2k3wmmbnfvr52r7zsc2qogitokfafpxq6jnw: t01000, ]
8: (Sep 29 15:26:58) [ bafy2bzaced577iipshzvvwpyn2kmpaqo3jrjxrkfauylq7wavhrqpkwdnu5e4: t01000, ]
9: (Sep 29 15:27:02) [ bafy2bzacecvw64pu527dh7554chhid2bmklffefvxu77dklt4aafgdghwm45i: t01000, ]
10: (Sep 29 15:27:06) [ bafy2bzacebscylcsnsecx7ct2ivzr6wkzd732o6tmdmukmtyf6xb7ewlxy63k: t01000, ]

[schomatis]

Hey @zgfzgf, thanks for submitting this. Could you add a test case that triggers this please? Or explain what would be the code flow that leads to this case?

We normally check that the incoming blocks are in decreasing order when fetching the chain so one block always has a higher height than its parent. If we were to receive a block with a lower height that the heaviest one we have then that would mean a possible fork that would trigger a different logic (syncFork).

[schomatis]

Also, the injectNulls case you mention in #4090 is a problem only for the same miner that generates that block since we (as a special case) bypass all the logic above mentioned (which an external node would not bypass, rejecting the incoming block).

lotus/chain/sync.go

Line 241 in e4e3a03

if from == syncer.self {

[zgfzgf]

When I was learning mining synchronization code, I found no place to verify block height information
The other blockchain projects are auto added 1

So I changed some of the code (keeping the height unchanged) as an attack node

Attack nodes can synchronize blocks to normal nodes.
As the example above, I have done experiments again just now, or as above

The normal node is compiled by GitHub source code
The attack node is compiled by the modified code

Therefore, it is better to increase block height verification, and at the same time, it is better to increase the judgment of height in mining code. Now, it mainly verifies weight

[zgfzgf]

hello @schomatis
The attack node can be implemented without changing a lot of code. I changed it locally a week ago, but I didn't save it. Now I only keep the executable program

If you don't quite understand what I mean, I'll submit the modified code to implement the attack node

[zgfzgf]

https://github.com/zgfzgf/lotus/tree/height-unchanged
@schomatis
Test steps:

As shown above

1. The first 10 blocks of attacking nodes run with normal node program
2. Then close the program
3. The attack node uses the above code compiler to run

Of course, there are other attack programs

[schomatis]

Hey @zgfzgf, thanks for the information. I understand and agree with what you're saying, what I was trying to say in #4091 (comment) is that I think that change only affects the miner who generated the fake block, other nodes will verify the height difference and reject it. If you have observed otherwise please let me know.

[schomatis]

As a demonstration of what I'm saying, try to rebase your commit zgfzgf@3f1cfa0 on top of #4192 (which removes the bypass of the checks for the miner that produces the block) to see if you can reproduce the error.

[zgfzgf]

@schomatis
NOT ”other nodes will verify the height difference and reject it“

The normal node accept it
see #4090

[zgfzgf]

other nodes accept it

[zgfzgf]

lotus/chain/sync.go

Lines 1385 to 1388 in 8f35a5c

	if types.CidArrsEqual(base.Parents().Cids(), known.Cids()) {
	// common case: receiving blocks that are building on top of our best tipset
	return blockSet, nil
	}

Node Online
Block by block synchronization
repeat the test

[arajasek]

Would prefer the check phrased as if h.Height <= baseTs.Height..., but this LGTM.