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

[Bridge] ETH block headers validation #901

Merged
merged 9 commits into from May 1, 2019

Conversation

Projects
None yet
3 participants
@ilblackdragon
Copy link
Member

commented Apr 24, 2019

  • Adding ETHash to Runtime, Fixes #832
  • API in WASM for header validation
    TODO: Add contract example to validate headers

@bowenwang1996 bowenwang1996 requested a review from evgenykuzyakov Apr 30, 2019

@bowenwang1996 bowenwang1996 marked this pull request as ready for review Apr 30, 2019

Show resolved Hide resolved nearmint/src/lib.rs Outdated
@evgenykuzyakov
Copy link
Collaborator

left a comment

It seems super insecure with file cache implementation.
We should have only one Eth provider per node. But now we have one for runtime and one for state_viewer

Show resolved Hide resolved runtime/wasm/runtest/generate-wasm/to-wasm/src/lib.rs Outdated
Show resolved Hide resolved runtime/runtime/src/ethereum.rs Outdated
}

pub fn from_file(cache_dir: &PathBuf, epoch: u64) -> io::Result<Self> {
let mut file = File::open(Self::storage_file(cache_dir, epoch))?;

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Apr 30, 2019

Collaborator

It doesn't provide much protection on same file reading/writing

This comment has been minimized.

Copy link
@bowenwang1996

bowenwang1996 May 1, 2019

Member

What's the better option?

@evgenykuzyakov evgenykuzyakov requested a review from nearmax Apr 30, 2019

bowenwang1996 added some commits May 1, 2019

Nit
@bowenwang1996

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I resolved the problem by using Arc<Mutex<EthashProvider>>.

@bowenwang1996 bowenwang1996 requested a review from evgenykuzyakov May 1, 2019

@evgenykuzyakov
Copy link
Collaborator

left a comment

I don't really see a way to introduce this change without more design and security considerations.

},
};
match cache {
None => {

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov May 1, 2019

Collaborator

This implementation can easily be abused by using random block_number. Overall we shouldn't add it without security considerations.
Now if your node needs to catch up you can't rely on only having recent block_numbers, but it may be requesting really old blocks.
If computing cache takes 25 seconds, then your node might just go completely down after a single block.

This comment has been minimized.

Copy link
@bowenwang1996

bowenwang1996 May 1, 2019

Member

I am confused. The block number here is ethereum block number, not our block number. For a random block number, we will reject it if it is not captured by the cache (i.e, too far in the future). Also the cache only needs to be computed every 30000 blocks.

This comment has been minimized.

Copy link
@bowenwang1996

bowenwang1996 May 1, 2019

Member

There are some features that have not been implemented yet (for example tracking ethereum headers), which might be the point of confusion.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov May 1, 2019

Collaborator

Yes. I don't see we rejecting random headers yet. So how about we don't expose it until we do

@bowenwang1996

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I'll merge this PR for now and open another one for the ethereum contract.

PR getting large

@bowenwang1996 bowenwang1996 merged commit 52f255c into master May 1, 2019

1 check passed

ci/gitlab/ethash Pipeline passed on GitLab
Details

@bowenwang1996 bowenwang1996 deleted the ethash branch May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.