fix: stop MTP attack #3357
fix: stop MTP attack #3357
Conversation
| @@ -643,7 +650,24 @@ where B: BlockchainBackend | |||
| pub fn prepare_block_merkle_roots(&self, template: NewBlockTemplate) -> Result<Block, ChainStorageError> { | |||
| let NewBlockTemplate { header, mut body, .. } = template; | |||
| body.sort(); | |||
| let header = BlockHeader::from(header); | |||
| let mut header = BlockHeader::from(header); | |||
There was a problem hiding this comment.
This method now does more than it says it does. It should be renamed or this logic should be moved elsewhere
There was a problem hiding this comment.
I renamed the method to prepare_new_block. The timestamp was already set by this method, it was done inside of let header = BlockHeader::from(header);. All it does not is add an extra check to what it should set it and not just blanketly set it to now. But I do agree with the fact that the function name and what it does is misaligned.
| self.timestamp = timestamp; | ||
| // if the timestamp has been advanced by the base_node due to the median time we should not reverse it but we | ||
| // should only change the timestamp if we move it forward. | ||
| if timestamp > self.timestamp { |
There was a problem hiding this comment.
nit: I'd rather go for putting the check outside the function call (it's only used in one place anyhow). It isn't expected for set_timestamp not to set the timestamp. Alternatively, name the function set_forward_timestamp (or something) and explain in a docstring that is only sets the timestamp forward from what it is. This is a nit so feel free to ignore.
There was a problem hiding this comment.
I agree good idea
| @@ -643,7 +650,24 @@ where B: BlockchainBackend | |||
| pub fn prepare_block_merkle_roots(&self, template: NewBlockTemplate) -> Result<Block, ChainStorageError> { | |||
| let NewBlockTemplate { header, mut body, .. } = template; | |||
| body.sort(); | |||
| let header = BlockHeader::from(header); | |||
| let mut header = BlockHeader::from(header); | |||
There was a problem hiding this comment.
nit: We may as well call this function prepare_new_block(NewBlocktemplate) -> Block
There was a problem hiding this comment.
I agree, will rename. That is what the function did anyway. All we do now is we do an extra check before we set the timestamp. Before this, the set_timestamp happened in let header = BlockHeader::from(header);
rename merkle root function
SWvheerden
force-pushed
the
sw_stop_MTP_attack
branch
from
1ebdbe2 to
648797e
Compare
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 < FTLFor a description see: Graft network
and MTP Attack (Jagernan)
How Has This Been Tested?
All current unit tests pass.