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

Improve Remasc storage before 0.5.0 activation #793

Merged
merged 5 commits into from Mar 29, 2019

Conversation

Projects
None yet
6 participants
@tinchou
Copy link
Contributor

commented Mar 26, 2019

This PR is interesting in many ways. I will try to explain the basics here, but I think it deserves a blog post.

Motivation

When we launched our mainnet we overlooked a feature that made the Remasc contract (which manages the distribution of miner fees) store too much information on disk. It stored a list of sibling blocks that changed in every block, and the contract storage grew unbounded.

We were able to claim back some resources through a process we call pruning, which is in many ways similar to Ethereum's garbage collection (only that it targets this specific contract).

For the 0.5.0 version we proposed RSKIP85 (see #611) which fixes this issue by not storing siblings information on disk at all. This change allowed us to move forward but the pain remained for anyone trying to synchronize a node from scratch.

Implementation

Basically we won't validate state roots before 0.5.0. This is in a sense similar to what fast-sync would do: you ignore some part of the history and continue from then on.

Apply RSKIP85 storage changes from the beginning

This is the crux of this change. We want that nodes never store this information from the beginning, which means we won't ever face the current storage problems. This means we can also get rid of the pruning feature, which has to suspend block processing and perform expensive storage copying to work.

State roots won't be validated until 0.5.0 activation

In order to change the storage format, the release that includes this change (possibly 0.6.2) will not validate state roots until the block that activates RSKIP85. This only affects mainnet, from block 0 until block 729000.

State roots will be validated after 0.5.0 activation

These changes were developed so that when the block 729000 is reached, the storage is in exactly the same state as if nothing had changed. From that point on, we turn on state root validation and continue synchronizing as usual. This is possible because when implementing RSKIP85 we ensured the storage was cleaned as soon as the hard fork was activated, and also because the changes in storage do not affect the distribution logic.

Future work

Checkpoints

The proposed implementation only guarantees consistency once you synced past block number 729000. It also doesn't allow to upgrade a node that hadn't synced past block 729000 (though this is unlikely to happen). Checkpoints could solve both problems at once, though not without its own set of problems.

Garbage collection

We leave the work of a full-featured garbage collector for later. Version 0.7.0 will introduce a set of storage changes we call unitrie, which will lend itself to a much easier and efficient garbage collection process. The pruning feature that is deleted here should be considered when implementing it, both to draw inspiration and learn from past mistakes.

@tinchou tinchou force-pushed the simplifyremasc branch 4 times, most recently from 66bea77 to 2ed4da9 Mar 26, 2019

* @return BlockResult with the final state data.
*/
public BlockResult execute(Block block, byte[] stateRoot, boolean discardInvalidTxs) {
return execute(block, stateRoot, discardInvalidTxs, false);
public BlockResult execute(Block block, Block parent, boolean discardInvalidTxs) {

This comment has been minimized.

Copy link
@SergioDemianLerner

SergioDemianLerner Mar 28, 2019

Contributor

It is really necessary to switch from the stateRoot to the parent block here ? In the future nodes will fast sync, and therefore the first block to be executed won't have a block parent to reference. It may have a BlockHeader parent, but not a full block.

This comment has been minimized.

Copy link
@SergioDemianLerner

SergioDemianLerner Mar 28, 2019

Contributor

I think passing the BlockHeader could suffice. If not, then maybe we can pass both the parent block id and the stateroot, and the code uses one or the other (the one that is != null)

This comment has been minimized.

Copy link
@tinchou

tinchou Mar 28, 2019

Author Contributor

I passed the BlockHeader. Good catch!

@SergioDemianLerner

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

What does make more sense, to remove the feature of validating pre RSKIP85 states completely (this PR) or to deactivate this feature by default, but leave a command line switch or configuration option to re-activate it on request ?

@tinchou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

I think we could, but then we wouldn't be able to simplify our codebase as this PR does. In fact, supporting more options makes it more complex. I think that archive versions (such as 0.6.1) should serve to people who want to do this validation when this code is released.

@tinchou tinchou force-pushed the simplifyremasc branch from 2ed4da9 to e294033 Mar 28, 2019

@tinchou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

It seems this breaks co.rsk.rpc.modules.evm.EvmModuleImpl#evm_mine and I haven't found how yet. The federation integration tests show this behavior.

@tinchou tinchou force-pushed the simplifyremasc branch 3 times, most recently from 99e78df to 8f80082 Mar 28, 2019

@tinchou tinchou marked this pull request as ready for review Mar 28, 2019

@tinchou tinchou requested review from lsebrie, diega and SergioDemianLerner Mar 28, 2019

@lsebrie lsebrie force-pushed the simplifyremasc branch from 8f80082 to 88d1494 Mar 29, 2019

@lsebrie
Copy link
Contributor

left a comment

LGTM

@tinchou tinchou force-pushed the simplifyremasc branch from 88d1494 to 8b02782 Mar 29, 2019

@rskops

This comment has been minimized.

Copy link

commented Mar 29, 2019

SonarQube analysis reported 17 issues

  • MAJOR 11 major
  • MINOR 5 minor
  • INFO 1 info

Top 10 issues

  1. MAJOR RskFactory.java#L360: java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path; reads a file whose location might be specified by user input rule
  2. MAJOR RskFactory.java#L361: java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path; reads a file whose location might be specified by user input rule
  3. MAJOR RskFactory.java#L367: java/io/FileOutputStream.(Ljava/lang/String;)V writes to a file whose location might be specified by user input rule
  4. MAJOR RskFactory.java#L535: Either log or rethrow this exception. rule
  5. MAJOR RskFactory.java#L714: java/io/File.(Ljava/lang/String;)V reads a file whose location might be specified by user input rule
  6. MAJOR Trie.java#L130: The Cyclomatic Complexity of this method "fromMessage" is 11 which is greater than 10 authorized. rule
  7. MAJOR Trie.java#L436: Return an empty array instead of null. rule
  8. MAJOR Trie.java#L441: Return an empty array instead of null. rule
  9. MAJOR Trie.java#L450: Return an empty array instead of null. rule
  10. MAJOR Trie.java#L565: The Cyclomatic Complexity of this method "internalPut" is 11 which is greater than 10 authorized. rule

@diega diega merged commit 4c3a4e7 into master Mar 29, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
default Build finished.
Details
sonarqube SonarQube reported 17 issues, no criticals or blockers

@diega diega deleted the simplifyremasc branch Mar 29, 2019

@aeidelman aeidelman added this to the Orchid v0.6.2 milestone Apr 19, 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.