Bloom upgrade in master

[NikVolf]

had to move the same in-place upgrade logic that we had in beta

[coveralls]

Coverage decreased (-0.05%) to 86.456% when pulling 1d3934b on bloom-upgrade-master into 4581469 on master.

[coveralls]

Coverage remained the same at 86.502% when pulling 1d3934b on bloom-upgrade-master into 4581469 on master.

[coveralls]

Coverage increased (+0.01%) to 86.512% when pulling 1d3934b on bloom-upgrade-master into 4581469 on master.

[rphmeier]

should add a comment warning about the dangers of adding future migrations which affect the bloom without adjusting the inplace migrations. See https://github.com/ethcore/parity/pull/2411#issuecomment-251810990 -- adding this mechanism to master is just technical debt we'll have to clean up later.

[rphmeier]

same println! issue as the beta PR.

[rphmeier]

this comment is actually wrong -- OverlayRecent will work with Archive, but not vice-versa, as Archive wouldn't be able to read the latest state from the journal overlay.

[rphmeier]

It's also probably best to just take the pruning argument as a parameter -- we have it in the migration phase, so there's no point firing shots in the dark. I don't see why refcounted and light pruning wouldn't be supported with this migration.

[NikVolf]

@rphmeier
nobody had issues running migration, this code is already used for several beta versions
where do you think will be fail there?

[rphmeier]

@NikVolf well, really it's because nobody uses light or refcounted but it's probably better to say "migration unsupported for pruning algorithm {}" than to fail unexpectedly with TrieError::NonexistantStateRoot. Although that's not really correct either, because there's no reason that the migration would be unsupported for that pruning algorithm because it will work for an arbitrary pruning algorithm.

[NikVolf]

why it will fail with this error?
there is only one state root used there, which is for the best block, no?

[NikVolf]

@rphmeier i don't see where exactly this warning will have any use

[rphmeier]

@NikVolf If you imagine a migration 10 -> 11 which assumes the bloom is adjusted to a larger size, it'll fail for every 9 -> 11 migration if the inplace migration isn't adjusted accordingly. The warning will help the developer write the code correctly and also signal that the inplace migrations as a temporary mechanism have outstayed their welcome.

I'm not saying that it has to be fixed right away, but when we introduce solutions which won't function correctly for some edge cases, they're usually marked with a TODO.

[NikVolf]

@rphmeier
no, if we alter bloom size sometimes, such in-place upgrades are not temporary mechanism
i can't see how 9->11 migration will fail here
bloom will just be generated with the most recent options after 9->11 migration finishes

[rphmeier]

Ok, maybe you can explain the flaw in the reasoning here, but the way I see it is:

1. 9 -> 10 migration applied, creating a bloom with the initial size.
2. 10 -> 11 migration applied, which expects a bloom with the new size and fails, because inplace migrations are applied at the end.

To avoid this, you'd have to write your 10 -> 11 migration in such a way that it can handle blooms which have and have not been in-place upgraded without a version bump.

[NikVolf]

@rphmeier
9 -> 10 migration will create bloom with the most actual size right away, so this is not the case

[NikVolf]

Looks like trie iterations take too long on many machines for 10M+ accounts

[gavofyork]

closing for now; probably worth keeping the code in general since we'll want to rebuild after we've done the pruning and after a sufficiently high number of suicides have happened.