Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Invalidate blocks from future #3652

Merged

Conversation

splix
Copy link
Contributor

@splix splix commented Nov 28, 2016

Decline blocks which have a timestamp from future (more than 30 seconds from current time). Otherwise malicious miner can use block with invalid timestamp to attack the network.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 28, 2016
InvalidTimestamp(OutOfBounds { max: Some(get_time().sec as u64 + 30), min: None, found: header.timestamp() }));

header = good.clone();
header.set_timestamp(get_time().sec as u64 + 31);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the test fail spuriously if the blocks check happens to hit the next second interval. Please set to something like + 40 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe 32 is enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32 should be enough unless the test takes longer than a second to execute for some reason. I'd put 40 just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, ok. I considered 2 seconds for such test as a failure already

@@ -516,6 +522,16 @@ mod tests {
InvalidTimestamp(OutOfBounds { max: None, min: Some(parent.timestamp() + 1), found: header.timestamp() }));

header = good.clone();
header.set_timestamp(2450000000);
check_fail(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine),
InvalidTimestamp(OutOfBounds { max: Some(get_time().sec as u64 + 30), min: None, found: header.timestamp() }));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the test fail spuriously if the blocks check happens to hit the next second interval. Test should just check if this is an InvalidTimestamp error.

header = good.clone();
header.set_timestamp(get_time().sec as u64 + 31);
check_fail(basic_test(&create_test_block_with_data(&header, &good_transactions, &good_uncles), engine),
InvalidTimestamp(OutOfBounds { max: Some(get_time().sec as u64 + 30), min: None, found: header.timestamp() }));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 28, 2016
@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 28, 2016
@splix
Copy link
Contributor Author

splix commented Nov 29, 2016

@arkpar fixed

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Nov 29, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 85.978% when pulling 8ec8bcd on ethereumproject:splix/block-ts-validation into a7037f8 on ethcore:master.

@gavofyork gavofyork added B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. B0-patch and removed B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Nov 29, 2016
@gavofyork
Copy link
Contributor

@jesuscript / @General-Beck the gitlab CI isn't firing?

@jesuscript
Copy link
Contributor

I think it's because Gitlab only builds branches in the main repo (not PRs)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants