Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't generate a block template with invalid timestamp #118

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

jagerman
Copy link
Contributor

If some other node has submitted enough blocks with forged future
timestamps, legitimate nodes end up providing their pools with a block
template that cannot be accepted--it fails the requirement that a block
timestamp be greater than the median of the recent block window.

This fixes the node to increase the timestamp to the median (i.e. the
minimum required) if the timestamp would be rejected so that it doesn't
end up handing out impossible-to-accept block templates.

Most importantly, this prohibits an attacker from stalling all
legitimate pools by submitting fake timestamps to the network.

Fixes #116

If some other node has submitted enough blocks with forged future
timestamps, legitimate nodes end up providing their pools with a block
template that cannot be accepted--it fails the requirement that a block
timestamp be greater than the median of the recent block window.

This fixes the node to increase the timestamp to the median (i.e. the
minimum required) if the timestamp would be rejected so that it doesn't
end up handing out impossible-to-accept block templates.

Most importantly, this prohibits an attacker from stalling all
legitimate pools by submitting fake timestamps to the network.

Fixes graft-project#116
@jagerman
Copy link
Contributor Author

I should also point out that I applied this patch on my own pool and was successfully able to mine 8 blocks with a small nicehash order. Unfortunately, 3 blocks in the attacker noticed, mined on an alt chain and orphaned 5 of those, but the main point is that this works.

@jagerman
Copy link
Contributor Author

Also worth pointing out: this doesn't require a fork, but merely an update of the nodes pool are attached to.

@DevEidem
Copy link

Not reccomending this! The suggested code adds to offset to latest block, which, after an attack makes everyone using it an attacker on the network, maximizing the 7200 FTL, look at blockchain explorer, we are maximizing the FTL limit, blocks are 2 hours in the future, with this fix we are maximizing future time
and then legitimate nodes will never be able to submit any blocks, after fork limit is 500, not 7200
so graft will then be broken with this "fix". Think about it. @zawy12

@jagerman
Copy link
Contributor Author

Not reccomending this! The suggested code adds to offset to latest block

You should look at the code more carefully. It only updates the offset if an offset is needed to meet the median requirements, and even then, it only uses the median time, not 7200. It also handles the v9 fork code just fine, using an 11 block median instead of 60 block median.

@DevEidem
Copy link

Regardless offset needed or not, at the moment pools are maximizing FTL 7200, all blocks are in the future.
Once we hit block 68000, we will have FTL 500, with blocks already 2 hours in the future.

@jagerman
Copy link
Contributor Author

Regardless offset needed or not, at the moment pools are maximizing FTL 7200, all blocks are in the future.
Once we hit block 68000, we will have FTL 500, with blocks already 2 hours in the future.

Yes. There will be a 2 hour period immediately after the fork where no pools anywhere can find a block. That happens with or without my patch.

@zawy12
Copy link

zawy12 commented Apr 23, 2018

I should mention Zcash added 1 second to the MTP for those timestamps not meeting it.

@DevEidem I don't think making honest timestamps lie like this in order to be accepted will do damage. He already owns the MTP. It does not make FTL go into the future, so all the bad timestamps will eventually be limited by FTL. This does not seem to change the attacker's motivations. If he keeps pushing the FTL limit, difficulty will catch up to him, so he'll be paying a fair, expensive price for the blocks. He can own the MTP forever, and push the FTL forever, and the chain might work fine, if nodes are close enough in agreement on time....but if he starts running a bunch of nodes to approve his own blocks.....I guess there could be chain splits and then I guess you need trusted nodes. Staying close to the FTL may also cause chain splits if nodes are not close on agreement in time....and I don't know how propagation delays figure into this. Selfish mining opens up another can of worms, but I don't think this change will affect that. It seems to simply allow honest miners to get a few blocks in when they can and should. What'll happen is that he'll come and go even N or N/2 blocks to get about N/3 blocks at an average FTL/N/2 reduction in difficulty.

But in all this, I can't see how jagerman's idea will hurt, and it seems like Zcash was smart in doing it I found out about it because I had noticed a bunch of blocks with solvetimes of "1" on the zcash chain during an attack

@jagerman
Copy link
Contributor Author

which, after an attack makes everyone using it an attacker on the network, maximizing the 7200 FTL

It sets the timestamp in the block template to the minimum value required for the node itself to validate the block. This means that in the presence of an attack it will generally be considerably less in the future than the attacker's blocks and adding that block to the chain will reduce the amount by which other (legitimate) pools have to upwardly adjust FTL.

@jagerman
Copy link
Contributor Author

@zawy12 - do you think the +1 is important to add? For the 60-block window it won't matter (the median is already going to be halfway between the 30th and 31st highest), but for the 11-block window it's going to be an identical timestamp to the 6th highest.

@jagerman
Copy link
Contributor Author

Important for the difficulty adjustment, I mean. It's not needed for the mined block to be accepted by the node (rejection is a strict inequality: https://github.com/graft-project/GraftNetwork/blob/master/src/cryptonote_core/blockchain.cpp#L3080).

@DevEidem
Copy link

@zawy12 Wouldn't that cause a logic issue?

@zawy12
Copy link

zawy12 commented Apr 23, 2018

I can't see any reason for sticking with my MTP=11. If you see my equation in the issue I came up with this morning, you can see honest timestamps will not need to be changed nearly as much, especially if it's only a 3x miner instead of this 10x miner. We added MTP=11 arbitrarily, without good reason other than 60 seemed unreasonable and 'bitcoin did it" i.e., 11xT. But in real time MTP 11xT in BTC is 60xT here. As far as the LWMA goes, the method Graft is using makes the MTP irrelevant because negatives times are forbidden (in a way I got that comes from Neil kyuupichan that prevents an exploit that is allowed when people try to use if ST < 1 then ST=1 ). BTW, if graft is a monero clone that's sorting timestamps somewhere it needs to be unsorted before it enters LWMA calculation, otherwise difficulty will go lower than it should.

I do not know of a reason to make it "1". solvetimes in the LWMA are "0" whenever there is an out-of-sequence timestamp, so the DA at least does not seem to have a problem with 0.

@jagerman
Copy link
Contributor Author

Putting MTP back to 60 (combined with the FTL) would make this attack harder, but not impossible. It would require yet another fork, however, which I think graft should avoid.

With the v9 parameters of 11/500, an attacker can still pull off an attack by getting 6 out of 11 blocks (or 31 out of 60) on the chain with future timestamps. As soon as that happens, this currently prevents all pools (without this patch) from finding any blocks because the pools are trying to solve the last block the node issued them, which has an invalid timestamp. So the attacker pulls this off by jamming up the blockchain with the 6 or 31 forged timestamps, then goes off the mine leisurely on a private chain for a while, reintroducing that later into the network.

The attack can be foiled by restarting a pool after the median has fallen, since that will reissue the block template with the updated, post-median timestamp. That still requires waiting for the restarted pool to find a block (which will then kick other pools into getting a new block template). But this isn't the sort of thing that big pools usually do.

@zawy12
Copy link

zawy12 commented Apr 23, 2018

Crap I didn't realize they did another fork already.

@jagerman
Copy link
Contributor Author

Crap I didn't realize they did another fork already.

It's about an hour away :)

@zawy12
Copy link

zawy12 commented Apr 23, 2018

There's another potential issue. If graft is a monero clone, there might be a sort occurring to the timestamps before they get to the LWMA. With the +10xT inside LWMA (it's supposed to be +7xT) this....well actually I shouldn't disclose here. I'll email a dev.

@jagerman
Copy link
Contributor Author

If graft is a monero clone, there might be a sort occurring to the tiomestamps before they get to the LWMA.

The sort was in the v7 (i.e. pre difficulty change), but isn't in v8 (or v9). It's here in v7:

sort(timestamps.begin(), timestamps.end());

and isn't in the v8 or v9 difficulty code below that.

@zawy12
Copy link

zawy12 commented Apr 23, 2018

I think they should postpone. Changing FTL to 7200/500 might still allow 50% daamge due to changing MTP 60/11 at the same time. We have 3 good things to change. Keep MTP=60, change +10 to +7, and
your idea. I would not assume your change is trivial. I don't even know how that's done because I thought timestamp is in the hash. And it would be good if someone can check to see if there'a timestamp sort.

@imperdin
Copy link

imperdin commented Apr 23, 2018

@jagerman, Using your code causes unexpected issues when appending transactions.

2018-04-23 14:13:29.603 [RPC1] ERROR cn src/cryptonote_core/cryptonote_core.cpp:1017 Transaction not found in pool
2018-04-23 14:14:07.213 [RPC1] ERROR cn src/cryptonote_core/cryptonote_core.cpp:1017 Transaction not found in pool

@jagerman
Copy link
Contributor Author

Using your code causes unexpected issues when appending transactions.

I took another look, but I don't see how that's possible from this code: the only value the code alters is the b.timestamp value. (And for the last right now, it's not altering it at all since the median is ).

Are you sure this wasn't happening before?

@jagerman
Copy link
Contributor Author

Also, for at least the last hour, and perhaps longer, the median block timestamp has been in the past, in which case this code doesn't change anything.

@jagerman
Copy link
Contributor Author

I don't even know how that's done because I thought timestamp is in the hash.

It is; but it's in the block template that the pool gets from the node. Miners mine it, and when a result is found, the pool passes it back to the node, which then rejects it because the timestamp value that the node itself put in the block template is wrong.

@mbg033
Copy link
Contributor

mbg033 commented Apr 23, 2018

Using your code causes unexpected issues when appending transactions.

@imperdin, It's very unlikely the reason is this patch

@imperdin
Copy link

@mbg033 We Seb tried to apply it to his pool and the pool was enable to find blocks due to the error, then ~~we~ Seb mitigated it using empty blocks as a temporary mitigation until the fork

@jagerman
Copy link
Contributor Author

Perhaps related: does anyone have any idea why the 5 transactions with age > 85 hours in the mempool aren't getting put into a block?

@jagerman
Copy link
Contributor Author

We Seb tried to apply it to his pool and the pool was enable to find blocks due to the error, then we Seb mitigated it using empty blocks as a temporary mitigation until the fork

I can also report that my pool found at least one block (67882) which included transactions and had a forged timestamp to meet the median requirements (looking at the two surrounding blocks, it was forged around +1h to meet the median boundary requirement, which makes sense since the previous 60 blocks were all +2h forgeries, so the median would have been right around +1h).

@iamamyth
Copy link

I do not know of a reason to make it "1". solvetimes in the LWMA are "0" whenever there is an out-of-sequence timestamp, so the DA at least does not seem to have a problem with 0.

One reason for incrementing the median is simply so that time always moves forward. So, it could be unrelated to difficulty. Moving time forward when it's already too far forward seems perhaps counter-productive, but that depends, in part, on how one resolves the broader question of whether to use subjective node data (wall clock time) rather than solely checking the blockchain for internal timestamp consistency, which seems out of scope for this PR, in which case no increment would be reasonable.

A secondary concern, which does relate to difficulty, would be that if the last N blocks all have identical timestamps, you could end up with an astronomical difficulty, depending on the algorithm (i.e. if zero time has elapsed but N blocks have been solved, difficulty skyrockets, in some implementations). I imagine someone else could speak to the that scenario better than I.

@zawy12
Copy link

zawy12 commented Apr 23, 2018

Good point. It's an interesting idea for a malicious hack. LWMA includes !<= 0 protection.

@mbg033
Copy link
Contributor

mbg033 commented Apr 25, 2018

Perhaps related: does anyone have any idea why the 5 transactions with age > 85 hours in the mempool aren't getting put into a block?

These transactions were mined into the blocks which became alternative chain and currently gone. Transactions will be removed from pool once one week (CRYPTONOTE_MEMPOOL_TX_FROM_ALT_BLOCK_LIVETIME) passed from creation time

@trestylez
Copy link

It looks like the attacks are back and things have stalled again.

@jagerman
Copy link
Contributor Author

Yes. This is being exploited again. All you need to freeze the chain now is 6 out of the last 11 blocks to be in the future to freeze the chain. Then the attacker mines on his own private chain and dumps that onto the network after a couple hundred blocks.

That is exactly what is happening, again.

@mbg033 - this is being exploited again, this patch fixes it, doesn't require a fork, but really, really needs to go out to pools.

@jagerman
Copy link
Contributor Author

In the meantime, any pool owners paying attention: a pool restart as soon as the block explorer stops counting down (i.e. as soon as the ages of the last few blocks stop counting down and start going up again) will unstick things.

@jagerman
Copy link
Contributor Author

Here's how this worked: the blocks up to 69950 had forged timestamps, and since they came within 8 minutes, they satisfied the 6-timestamps-of-the-last-11-are-in-the-future condition. That froze all pools on the network because as soon as that happens, they start trying to find a solution to a block that the node will never accept. (It won't accept it because the block has a timestamp--generated from the current time--that was earlier than the median timestamp of the last 11 blocks).

The attacker then disconnected his pool but kept mining on a private chain for 2 and a half hours while the rest of the network's pools were frozen (they were finding valid solutions, but their nodes were rejecting them). Looking back, that gap doesn't show up on the blockchain explorer; more on that below. Since he was the only one mining, he was able to do so incredibly profitably: diff along that chain (now our chain) dropped to just 441,768,993 at its lowest (I've added block difficulty to my copy of the block explorer at https://blocks.imaginary.ca to better see/investigate this.)

The network finally came unstuck because some pool operator somewhere restarted a pool (or a node) at some point, causing that pool to get a new block template from its node which had a new block timestamp, and since the forged timestamps at this point were in the past, that new block template was perfectly valid. As soon as that got mined and released onto the network, every other pool requested a new block template at the new height, at which point all pools came unstuck.

So why don't we see any such gap in the blockchain explorer now? Because once things came unstuck for a few blocks, the attacker noticed and reconnected his node--at this point, the rest of the network as about 10 blocks in. The 10 blocks got orphaned because in the 2.5 hours the attacker had built up a chain of 80 blocks; difficulty on those 80 was far too low, but 80small number is bigger than 10large number, so his chain won, and along his privately mined chain there was no long gap in blocks being found.

@zawy12
Copy link

zawy12 commented Apr 26, 2018

Are all cryptonote coins subject to this attack? It seems like any >50% miner could do this, even if the MTP were 60.

@jagerman
Copy link
Contributor Author

jagerman commented Apr 26, 2018

Yes, I see no reason why this wouldn't apply to other CN coins with the same median requirement.

That said, with MTP of 60 and FTL of 500 I think this would be very difficult to pull off: you'd have to be able to find 31 valid blocks with forged timestamps within 500 seconds, and unless the difficulty response is extremely slow, that's going to be very costly because of the difficulty ramp-up. That said, it's not impossible, just very expensive.

As for graft, in v8 you would need 31 of the last 60 within 7200; graft v9 needed 6 in the last 11, both significantly easier to accomplish. It seems likely that timestamp forgery was being used to alter the diff (which this patch does nothing about at all) and the attacker stumbled upon pools freezing up. This fix is rolling out to graft pools now, though; I'd be quite surprised if this isn't attempted against other coins.

@zawy12
Copy link

zawy12 commented Apr 26, 2018

Luckily the FTL = 500 is already on LWMA coins and MTP = 11 was not generally being done. But everyone needs this patch. It would be interesting to see when that change from BTC was made and which non-CN coins are making this mistake.

@zawy12
Copy link

zawy12 commented Apr 27, 2018

Is the only fix for miners and pools to follow the "MTP+1" rule so their blocks do not get rejected? I'll define the MTP here to be the actual MTP, not the constant in the MTP variable. "MTP" in this post means 6 or 30.5 here instead of my previous posts saying MTP=11 or 60. FTL here will also be in terms of T, so previous FTL=500 with T=120 is FTL = 4.2. An attacker needs to be > MTP/FTL+0.5 times the baseline hashrate to succeed if the coin uses the Cryptonote default algorithm (which does not hardly respond in 30 blocks). For T = 120, MTP = 30.5, and FTL = 4.2 this is 8x. But the above ratio assumes he gets all the blocks. He missed MTP/8 =~ 4 of them, so he needs to be > (MTP+4)/FTL + 0.5 = 9x With LWMA, he needs to be about 20x because the difficulty rises faster. 20x sometimes occurs. Since this exploit is now known, it's more likely to occur.

With FTL =7200, Monero seems vulnerable. Repeating the calculation gives 1x, but he did not get 1/2 the blocks, so 2nd iteration is
[ MTP + MTP/1] / FTL + 0.5 = 1.5x
3rd iteration:
[ MTP + MTP/1.5] / FTL + 0.5 = 1.34x
And so on gives something about 1.4x which is 1.4/(1.4+1) = 58% of the network hash rate.

@zawy12 zawy12 mentioned this pull request Apr 27, 2018
@zawy12
Copy link

zawy12 commented Apr 27, 2018

Does this make a selfish mining attack any easier? Is it already easy to do a selfish mining attack if a miner has 55%? I mean, is the point moot?

@iamamyth
Copy link

Absent a change which makes network nodes generate valid block templates, this flaw improves the economics of selfish mining because the selfish chain has a higher likelihood of becoming the highest difficulty chain, due to nodes rejecting proofs of work from miners using node-generated block templates. As seen in past attacks, once the MTP set gets too far in the future, honest provers effectively drop from the network until time catches up with the MTP. In other words, one need not control a majority of hash power for the duration of the attack, because the honest provers exclude themselves, hence the improved returns.

@zawy12
Copy link

zawy12 commented Apr 27, 2018

@jagerman re-iterated something he already said in one of his posts here: it's not just that they can't get a block in until the MTP has gone into the past, but that maybe they do not request a new block template from the node even after the MTP has gone into the past, at least not until they have attempted to submit a block. Will they request a new template if their block gets rejected?

The amount of hash power I calculate above is at the edge: he may only block out miners for 1 block with that amount. That may be enough to cause havoc. But if miners are getting new templates after they get rejected, he will only be able to mine selfishly for about 2 blocks more (if he was only barely able to own the MTP before the FTL window ran out on him). FTL = 60 in CN, so if he can get 31 blocks to own the MTP in 30xT, then he has 30xT to get more blocks selfishly. Re-doing my iterative equation above with FTL replaced 30:

MTP / (30) + 0.5 = 1.5
(MTP + MTP/1.5)/30 + 0.5 = 2.2
(MTP + MTP/2.2)/30 + 0.5 = 2.0

So he needs about 2.1x HR => 2.1/(2.1+1) = 68%

OK, so if he was already present and the difficulty was correct for his presence at the moment MTP ownership began, the network loses 68% hash rate. The next block will come ~3x slower to them, while he will be selfish mining at 2/3 the normal solvetime, or twice as fast as them. There are 30 blocks remaining in the FTL buffer, so he can get 60 even if people are asking for the new block template, but he still owns the MTP.

@jagerman
Copy link
Contributor Author

jagerman commented Aug 1, 2018

Is there any reason this branch hasn't been included in the 1.2.2 release and 1.3.0 (develop) branch? This patch or an alternative implementation of it is present in upstream Monero and most other CN coins now.

@mbg033 mbg033 merged commit 772db03 into graft-project:master Aug 7, 2018
aviator-app bot pushed a commit to tari-project/tari that referenced this pull request Sep 16, 2021
Description
---
This changes the timestamp of a new block template to be always greater than the median time past (MTP) of the past blocks as per consensus. 

Motivation and Context
---
It is possible for an attack to forward the median time to be greater than now, but less than the future time limit(FTL). This means that all valid blocks with the current timestamp will be rejected. This will ensure that ll newly created blocks always have a timestamp greater than the MTP. Due to consensus we also know that MTP will always be less than FTL. This ensures that all new block timestamps are `MTP < Timestamp < FTL`

For a description see: [Graft network](graft-project/GraftNetwork#118 (comment))
and [MTP Attack (Jagernan)](zawy12/difficulty-algorithms#30)


How Has This Been Tested?
---

All current unit tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"mined block failed verification"
7 participants