fixed broken logs #7934

debris · 2018-02-18T14:28:18Z

what caused the bug?

The only place where blockchain's blocks_blooms is updated is in a function blooms_at, line 173. This function updates bloom fetched from db.

https://github.com/paritytech/parity/blob/ff0c44c060e111c6234a47cd1aa8f6411533a202/ethcore/src/blockchain/blockchain.rs#L170-L177

That means, that if there are two consecutive blocks inserted into blockchain, but there was no database transaction flush in the meantime, blooms for new blocks will be created incorrectly. This is caused, by the fact, that our blooms are stored in multiple layers and each layer contain information about blooms from 16 consecutive blocks
Initially I suspected unordered insert to cause invalid log problems, that's why test function is using insert_unordered_block. However, unordered insert is unrelated and log problems occur always when there is no database transaction flush between two consecutive insertions (no matter if ordered or not).

update

I decided to completely remove bloom groups from blockchain database. Block blooms are almost full and merging them is completely redundant.

tomusdrw

Need some help with understanding the entire context.

tomusdrw · 2018-02-19T13:33:07Z

ethcore/src/blockchain/blockchain.rs

 		// These cached values must be updated last with all four locks taken to avoid
 		// cache decoherence
 		{
 			let mut best_block = self.pending_best_block.write();
+			let mut write_blocks_blooms = self.blocks_blooms.write();


Lock order is changed and does not match the order of fields initialization.

like in many other places in this file (although I check and locks never interfere) :) But you are right, I will bring back the old lock order, cause this change is not justified.

tomusdrw · 2018-02-19T13:46:06Z

ethcore/src/blockchain/blockchain.rs

+				BlockLocation::BranchBecomingCanonChain(_) => {
+					// clear all existing blooms, cause they may be created for block
+					// number higher than current best block
+					*write_blocks_blooms = update.blocks_blooms;


So we override write_blocks_blooms, but we don't really clear anything from the batch. So we may end up writing some additional data to the database, right? I guess it's not harmful?

On a second thought:

Why do we actually clear everything from write_blocks_blooms? It's actually a cache of whatever is in the database, no?

So do I understand correctly that previously:

The cache was reflecting what was in the db (since we always did extend_with_cache)

The problem was that some keys were overwritten instead of accrued (that's what the CanonChain branch is doing)

Currently we may end up clearing the cache, but at least the database should contain correct values?

Could you also elaborate on what is actually stored in update.blocks_blooms (what is used as a key and value)?

So we override write_blocks_blooms, but we don't really clear anything from the batch. So we may end up writing some additional data to the database, right? I guess it's not harmful?

There is nothing in the batch to be cleared.

Why do we actually clear everything from write_blocks_blooms? It's actually a cache of whatever is in the database, no?

cause new BlockLocation is BranchBecomingCanonChain. Database entries need to be updated and so is cache.

The cache was reflecting what was in the db (since we always did extend_with_cache)

No, it was always empty after executing this function, cause UpdatePolicy was Remove.

https://github.com/paritytech/parity/blob/5b4abec2dbc3f5653b2caf2cc481411a29c7d00b/ethcore/src/db.rs#L127-L131

The problem was that some keys were overwritten instead of accrued (that's what the CanonChain branch is doing)

No. The problem was that there were never in cache and not yet in database, cause noone has written transaction batch to it.

Currently we may end up clearing the cache, but at least the database should contain correct values?

We do not care about database, cause everything is in the cache (as it should be).

andresilva

Here's my understanding of this:

When there's a reorg we need to rewrite the existing blooms since they might already have data from the forked branch
When we're already on the canon chain, we need to first check if there's already a bloom for this key and if there is we update the existing bloom. Otherwise, we create it.

Previously we removed the keys from the cache, now we don't touch the cache at all, is this correct?

andresilva · 2018-02-19T15:43:49Z

This only fixes the issue going forward, so I think people that already have a borked database will still have this issue. We should remember this PR if we see issues related to missing log data in the future.

codecov-io · 2018-02-19T22:06:41Z

Codecov Report

Merging #7934 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7934      +/-   ##
==========================================
- Coverage    76.6%   76.55%   -0.06%     
==========================================
  Files         658      658              
  Lines       93388    92992     -396     
==========================================
- Hits        71542    71186     -356     
+ Misses      21846    21806      -40

Impacted Files	Coverage Δ
whisper/src/net/mod.rs	`49.25% <0%> (-9.63%)`	⬇️
util/rlp_derive/src/de.rs	`52.39% <0%> (-6.49%)`	⬇️
secret_store/src/key_server_cluster/cluster.rs	`70.99% <0%> (-5.98%)`	⬇️
whisper/src/net/tests.rs	`78.02% <0%> (-5.5%)`	⬇️
util/network/src/host.rs	`60.57% <0%> (-3.71%)`	⬇️
..._server_cluster/client_sessions/signing_session.rs	`87.68% <0%> (-2.85%)`	⬇️
...tore/src/key_server_cluster/jobs/key_access_job.rs	`93.02% <0%> (-2.33%)`	⬇️
ethcore/light/src/net/error.rs	`15.21% <0%> (-2.18%)`	⬇️
ethcore/evm/src/interpreter/memory.rs	`55.71% <0%> (-2.15%)`	⬇️
util/network/src/session.rs	`72.8% <0%> (-2.12%)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01d9bff...b59073b. Read the comment docs.

debris · 2018-02-21T10:40:10Z

This only fixes the issue going forward, so I think people that already have a borked database will still have this issue. We should remember this PR if we see issues related to missing log data in the future.

Borked database will no longer be a problem

tomusdrw

lgtm!

tomusdrw · 2018-02-21T10:54:01Z

ethcore/src/blockchain/blockchain.rs

 			.into_iter()
-			.map(|b| b as BlockNumber)
+			.filter_map(|number| self.block_hash(number).map(|hash| (number, hash)))


Could possibly be done concurrently, no?

tomusdrw · 2018-02-21T10:56:34Z

ethcore/src/blockchain/blockchain.rs

@@ -351,11 +333,13 @@ impl BlockProvider for BlockChain {

 	/// Returns numbers of blocks containing given bloom.
 	fn blocks_with_bloom(&self, bloom: &Bloom, from_block: BlockNumber, to_block: BlockNumber) -> Vec<BlockNumber> {


Imho would be best to return hashes here, we convert to numbers, then in client we pass numbers to blockchain::logs method that converts to hashes again. Seems pretty wasteful

tomusdrw · 2018-02-21T10:57:51Z

On a second thought. Should we have a (optional?) migration that removes block groups from DB? Or maybe even a separate binary that you can run to clean up the db?

andresilva

LGTM. We're also using BloomGroup in tracedb so we should probably make sure that everything's correct there. I agree with @tomusdrw that it would be nice to have a migration clean up the database (not sure how hard that is).

debris · 2018-02-21T12:09:28Z

@tomusdrw

On a second thought. Should we have a (optional?) migration that removes block groups from DB? Or maybe even a separate binary that you can run to clean up the db?

Yes, let's clean it up!

@andresilva

LGTM. We're also using BloomGroup in tracedb so we should probably make sure that everything's correct there. I agree with @tomusdrw that it would be nice to have a migration clean up the database (not sure how hard that is).

These are different blooms, but they have the same problem. I'll take care of them after fixing these blooms :)

* fixed broken logs * bring back old lock order * removed bloom groups from blockchain * revert unrelated changes * simplify blockchain_block_blooms

* Hardware-wallet/usb-subscribe-refactor (#7860) * Hardware-wallet fix * More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 <niklasadolfsson1@gmail.com> * Validate product_id of detected ledger devices * closed_device => unlocked_device * address comments * add target in debug * Address feedback * Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused * fixed broken logs (#7934) * fixed broken logs * bring back old lock order * removed bloom groups from blockchain * revert unrelated changes * simplify blockchain_block_blooms * Bump WS (#7952) * Calculate proper keccak256/sha3 using parity. (#7953) * Increase max download limit to 128MB (#7965) * fetch: increase max download limit to 64MB * parity: increase download size limit for updater service * Detect too large packets in snapshot sync. (#7977) * fix traces, removed bloomchain crate, closes #7228, closes #7167 (#7979) * Remvoe generator.rs * Make block generator easier to use (#7888) * Make block generator easier to use * applied review suggestions * rename BlockMetadata -> BlockOptions * removed redundant uses of blockchain generator and genereator.next().unwrap() calls

This reverts commit f8a2e53.

* Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167" This reverts commit 1bf6203. * Revert "fixed broken logs (#7934)" This reverts commit f8a2e53. * fixed broken logs * bring back old lock order * remove migration v13 * revert CURRENT_VERSION to 12 in migration.rs

* updater: apply exponential backoff after download failure (#8059) * updater: apply exponential backoff after download failure * updater: reset backoff on new release * Limit incoming connections. (#8060) * Limit ingress connections * Optimized handshakes logging * Max code size on Kovan (#8067) * Enable code size limit on kovan * Fix formatting. * add some dos protection (#8084) * more dos protection (#8104) * Const time comparison (#8113) * Use `subtle::slices_equal` for constant time comparison. Also update the existing version of subtle in `ethcrypto` from 0.1 to 0.5 * Test specifically for InvalidPassword error. * revert removing blooms (#8066) * Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167" This reverts commit 1bf6203. * Revert "fixed broken logs (#7934)" This reverts commit f8a2e53. * fixed broken logs * bring back old lock order * remove migration v13 * revert CURRENT_VERSION to 12 in migration.rs * Fix compilation. * Check one step deeper if we're on release track branches * add missing pr * Fix blooms? * Fix tests compiilation. * Fix size.

* Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167" This reverts commit 1bf6203. * Revert "fixed broken logs (#7934)" This reverts commit f8a2e53. * fixed broken logs * bring back old lock order * remove migration v13 * revert CURRENT_VERSION to 12 in migration.rs

* Support parity protocol. (#8035) * updater: apply exponential backoff after download failure (#8059) * updater: apply exponential backoff after download failure * updater: reset backoff on new release * Max code size on Kovan (#8067) * Enable code size limit on kovan * Fix formatting. * Limit incoming connections. (#8060) * Limit ingress connections * Optimized handshakes logging * WASM libraries bump (#7970) * update wasmi, parity-wasm, wasm-utils to latest version * Update to new wasmi & error handling * also utilize new stack limiter * fix typo * replace dependency url * Cargo.lock update * add some dos protection (#8084) * revert removing blooms (#8066) * Revert "fix traces, removed bloomchain crate, closes #7228, closes #7167" This reverts commit 1bf6203. * Revert "fixed broken logs (#7934)" This reverts commit f8a2e53. * fixed broken logs * bring back old lock order * remove migration v13 * revert CURRENT_VERSION to 12 in migration.rs * more dos protection (#8104) * Const time comparison (#8113) * Use `subtle::slices_equal` for constant time comparison. Also update the existing version of subtle in `ethcrypto` from 0.1 to 0.5 * Test specifically for InvalidPassword error. * fix trace filter returning returning unrelated reward calls, closes #8070 (#8098) * network: init discovery using healthy nodes (#8061) * network: init discovery using healthy nodes * network: fix style grumble * network: fix typo * Postpone Kovan hard fork (#8137) * ethcore: postpone Kovan hard fork * util: update version fork metadata * Disable UI by default. (#8105) * dapps: update parity-ui dependencies (#8160)

fixed broken logs

6aaf90e

debris added A0-pleasereview 🤓 Pull request needs code review. B0-patch M4-core ⛓ Core client code / Rust. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. labels Feb 18, 2018

debris added this to the 1.10 milestone Feb 18, 2018

tomusdrw reviewed Feb 19, 2018

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 19, 2018

bring back old lock order

8b43f61

debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 19, 2018

andresilva reviewed Feb 19, 2018

View reviewed changes

debris 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 Feb 19, 2018

debris added 3 commits February 21, 2018 11:09

Merge branch 'master' into fixed_broken_logs

1b6bb1a

removed bloom groups from blockchain

1fa4de9

revert unrelated changes

53f399d

debris 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 Feb 21, 2018

tomusdrw approved these changes Feb 21, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 21, 2018

andresilva reviewed Feb 21, 2018

View reviewed changes

simplify blockchain_block_blooms

b59073b

5chdn merged commit f8a2e53 into master Feb 22, 2018

5chdn deleted the fixed_broken_logs branch February 22, 2018 10:23

debris mentioned this pull request Feb 22, 2018

removed old migrations #7974

Merged

jo-tud mentioned this pull request Feb 22, 2018

Incorrect stateDiff in trace_replaytransaction #6520

Closed

debris mentioned this pull request Feb 22, 2018

Significantly reduce Archive Node size #7967

Closed

tomusdrw pushed a commit that referenced this pull request Feb 27, 2018

fixed broken logs (#7934)

b933bd0

* fixed broken logs * bring back old lock order * removed bloom groups from blockchain * revert unrelated changes * simplify blockchain_block_blooms

tomusdrw mentioned this pull request Feb 27, 2018

[beta] Backports #8011

Merged

10 tasks

debris added a commit that referenced this pull request Mar 6, 2018

Revert "fixed broken logs (#7934)"

0862028

This reverts commit f8a2e53.

debris mentioned this pull request Mar 6, 2018

revert removing blooms #8066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed broken logs #7934

fixed broken logs #7934

debris commented Feb 18, 2018 •

edited

Loading

tomusdrw left a comment

tomusdrw Feb 19, 2018

debris Feb 19, 2018

tomusdrw Feb 19, 2018

tomusdrw Feb 19, 2018

debris Feb 19, 2018 •

edited

Loading

andresilva left a comment •

edited

Loading

andresilva commented Feb 19, 2018

codecov-io commented Feb 19, 2018 •

edited

Loading

debris commented Feb 21, 2018

tomusdrw left a comment

tomusdrw Feb 21, 2018

debris Feb 21, 2018

tomusdrw Feb 21, 2018

tomusdrw commented Feb 21, 2018

andresilva left a comment

debris commented Feb 21, 2018

		@@ -351,11 +333,13 @@ impl BlockProvider for BlockChain {

		/// Returns numbers of blocks containing given bloom.
		fn blocks_with_bloom(&self, bloom: &Bloom, from_block: BlockNumber, to_block: BlockNumber) -> Vec<BlockNumber> {

fixed broken logs #7934

fixed broken logs #7934

Conversation

debris commented Feb 18, 2018 • edited Loading

update

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Feb 19, 2018

Choose a reason for hiding this comment

debris Feb 19, 2018

Choose a reason for hiding this comment

tomusdrw Feb 19, 2018

Choose a reason for hiding this comment

tomusdrw Feb 19, 2018

Choose a reason for hiding this comment

debris Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

andresilva left a comment • edited Loading

Choose a reason for hiding this comment

andresilva commented Feb 19, 2018

codecov-io commented Feb 19, 2018 • edited Loading

Codecov Report

debris commented Feb 21, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Feb 21, 2018

Choose a reason for hiding this comment

debris Feb 21, 2018

Choose a reason for hiding this comment

tomusdrw Feb 21, 2018

Choose a reason for hiding this comment

tomusdrw commented Feb 21, 2018

andresilva left a comment

Choose a reason for hiding this comment

debris commented Feb 21, 2018

debris commented Feb 18, 2018 •

edited

Loading

debris Feb 19, 2018 •

edited

Loading

andresilva left a comment •

edited

Loading

codecov-io commented Feb 19, 2018 •

edited

Loading