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

Implement hardcoded sync in the light client #8075

Merged
merged 8 commits into from
Mar 27, 2018

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Mar 8, 2018

Adds a hardcodedSync field to the chain specifications.
When set, contains the RLP of a header, a total difficulty, and a list of CHT root hashes. If there is no light client database, the light client will immediately jump to the given block.

After this change, the light client synchronizes in 10 to 15 seconds even at the first launch on the main network.

For now only the specs of the main network have been updated.

@parity-cla-bot
Copy link

It looks like @tomaka signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@rphmeier rphmeier added the A0-pleasereview 🤓 Pull request needs code review. label Mar 8, 2018
@tomaka tomaka added the M4-core ⛓ Core client code / Rust. label Mar 8, 2018
let cht_num = cht::block_to_cht_number(decoded_header.number() - 1)
.expect("specs provided a hardcoded block with height 0");
if cht_num >= hardcoded_sync.chts.len() as u64 {
panic!("specs didn't provide enough CHT roots for its hardcoded block");
Copy link
Contributor

Choose a reason for hiding this comment

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

rule for panics of any kind is "prove or remove" -- We may rather want to print a warning and fall back to hardcoded-less sync. Or move these checks to the deserialization phase where we can exit with an error more gracefully.

@@ -299,11 +332,36 @@ impl HeaderChain {
///
/// If the block is an epoch transition, provide the transition along with
/// the header.
#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

does that actually help? generally I figure LLVM will figure it out.

let header = if let Some(header) = self.block_header(BlockId::Number(h_num)) {
header
} else {
panic!("Header of block #{} not found in DB", h_num);
Copy link
Contributor

Choose a reason for hiding this comment

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

same: do not panic.


let decoded = header.decode();

let entry: Entry = ::rlp::decode(&self.db.get(self.col, era_key(h_num).as_bytes()).unwrap().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

and we really never use unwrap.

let entry: Entry = ::rlp::decode(&self.db.get(self.col, era_key(h_num).as_bytes()).unwrap().unwrap());
let total_difficulty = entry.candidates.iter()
.find(|c| c.hash == decoded.hash())
.expect("no candidate matching block found in DB")
Copy link
Contributor

Choose a reason for hiding this comment

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

should rather prove based on the fact that an inconsistent but correctly-loaded database shouldn't be possible.

chts.push(cht);
}

unreachable!("we are after an infinite loop")
Copy link
Contributor

@rphmeier rphmeier Mar 8, 2018

Choose a reason for hiding this comment

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

could be a loop with a break-value, either way it could follow the codebase style of proposition; proof; qed

parity/run.rs Outdated
@@ -258,6 +258,11 @@ fn execute_light_impl(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger
let service = light_client::Service::start(config, &spec, fetch, db, cache.clone())
.map_err(|e| format!("Error starting light client: {}", e))?;
let client = service.client();
// Note: uncomment this code if you want to regenerate the JSON of the hardcoded sync feature
// in the chain specifications.
/*if let Some(hs) = client.read_hardcoded_sync() {
Copy link
Contributor

Choose a reason for hiding this comment

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

we could make this a subcommand of some kind

Copy link
Contributor

Choose a reason for hiding this comment

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

generally not a fan of the commented-out code. the CLI is a PITA to work with though :\

@rphmeier rphmeier added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 8, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Mar 8, 2018

needs an opt-out flag also (off by default is fine, just that it should be there).

@rphmeier
Copy link
Contributor

rphmeier commented Mar 8, 2018

looks good other than grumbles

@tomaka
Copy link
Contributor Author

tomaka commented Mar 8, 2018

Should I make read_hardcoded_sync() return None in case of error? Or rather Result<Option<...>, ...>? I admit that I didn't really know what to do with it, it's an internal-ish method (which is also why I totally overlooked its quality :-/).

@rphmeier
Copy link
Contributor

rphmeier commented Mar 8, 2018

Result<Option<_>> would be better IMO

@tomaka
Copy link
Contributor Author

tomaka commented Mar 9, 2018

Sorry for the bad PR quality. I think it should be good now!

@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Mar 9, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Mar 20, 2018

needs merge, can you ping me for review? I'm not looking that closely at the Parity repo these days.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 20, 2018

@rphmeier Conflicts solved!

@5chdn 5chdn added this to the 1.11 milestone Mar 20, 2018
let on_demand = Arc::new(::light::on_demand::OnDemand::new(cache.clone()));

let sync_handle = Arc::new(RwLock::new(Weak::new()));
let fetch = ::light_helpers::EpochFetch {
Copy link
Contributor

Choose a reason for hiding this comment

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

on_demand, sync, network etc. are not necessary to start here -- a NoOpEpochFetch (I think it already exists for tests) will be enough.

@rphmeier
Copy link
Contributor

LGTM except for grumble

@tomaka
Copy link
Contributor Author

tomaka commented Mar 21, 2018

r?

"header": "f90219a061d694007fbaca6e23e73e29c8c6a60099abc740ab7e27ae3cd1b9c6a47efef7a01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347945a0b54d5dc17e0aadc383d2db43b0a0d3e029c4ca0a2f1bdabc1a72737de1c22a76cacc8fc162366904f759a99db7d6d19efee3090a0ac5f5b236e8977928a2ce43c7569ea5a74919643cb0b06d7540407b1ea1298f0a04356ddc5d77c83923a6541260308be167386e0969a608a017770c9e38091cfcab90100a00010002001009080011010141088000004000080081100000a002023000002204204801204084000c000010008000000000880080020c0000440200460000290005010c01c80800080004800100406003380000400402040000028084002a80087000008090a00200100544020019580022000000306100a0080100084020006809000e80000010000254810002000000a240050014200002002c10809202030006422022000203012000241089300080400000009001021020200012410348500028290230408100302000000058c0000020c08c20480081040020260004008481000080000800010010060020000e00020002140100a8988000004400201870b9af4a66df8038350a8018379d54483799eba845ab0426d984554482e45544846414e532e4f52472d3231323134313232a05eeccc80302d8fecca48a47be03654b5a41b5e5f296f271f910ebae631124f518890074810024c6c2b",
"totalDifficulty": "3144406426008470895785",
"CHTs": [
"0x0eb474b7721727204978e92e27d31cddff56471911e424a4c8271c35f9c982cc",
Copy link
Contributor

@5chdn 5chdn Mar 26, 2018

Choose a reason for hiding this comment

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

Is this anyhow possible without adding 2500 CHTs to the chain spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, what would the alternative be? We're hardcoding headers in the client after all, we have to put them somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

the alternative is to allow the client to be deployed w/o history or CHT roots and have the history fetched afterwards

@tomaka tomaka merged commit 0493161 into openethereum:master Mar 27, 2018
@tomaka tomaka deleted the hardcoded-sync branch March 27, 2018 11:57
@5chdn 5chdn mentioned this pull request Jun 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants