Please keep fHasMoreWork and then you can add:

bool chainTipShouldUpdate = isSameHeightAndMoreHonestlyMined || fHasMoreWork;

Keeping names for intermediate variable makes the code more readable.

This comment is invalid now.

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.

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.

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.

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.

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

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.

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.

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.

Sounds like you should use the comparator itself.

The reorg will be in the log anyways.

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.

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

Good idea.

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

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

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

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