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

Fix inconsistency state before commit #1709

Merged
merged 3 commits into from
Jul 22, 2023
Merged

Conversation

yito88
Copy link
Member

@yito88 yito88 commented Jul 12, 2023

In finalize_block, some data are written into the storage directly and a new epoch is updated in the Merkle tree when the new epoch starts before the commit.
It could cause an inconsistent state when shutdown in finalize_block and then restarting.

@@ -481,6 +471,7 @@ where
);
stats.increment_errored_txs();

self.wl_storage.drop_tx();
Copy link
Member

Choose a reason for hiding this comment

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

Does this only drop the current tx or does it clear the entire wl_storage? From reading the wl_storage code I'm not entirely sure.

Copy link
Member

Choose a reason for hiding this comment

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

only the current tx

.delete(&tx_hash_key)
.expect(
"Error while deleting tx hash key from storage",
);
// Apply only to remove its hash,
// since all other changes have already been dropped
self.wl_storage.commit_tx();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do this?

Copy link
Member

Choose a reason for hiding this comment

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

it is to commit the removal of the tx_hash_key above

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a bit tricky. But we have to commit_tx() to apply changes on the write log when the following commit phase.


// the merkle tree root should not change after finalize_block
let root_post = shell.shell.wl_storage.storage.block.tree.root();
assert_eq!(root_pre.0, root_post.0);
Copy link
Member

Choose a reason for hiding this comment

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

when I remove the fixes and run the tests they do not yet catch the issue - I think we need to add some txs in here

Copy link
Member

Choose a reason for hiding this comment

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

added in f30e158

@tzemanovic tzemanovic self-assigned this Jul 13, 2023
@tzemanovic tzemanovic marked this pull request as draft July 13, 2023 08:06

/// Make a wrapper tx and a processed tx from the wrapped tx that can be
/// added to `FinalizeBlock` request.
fn mk_wrapper_tx(
Copy link
Member

Choose a reason for hiding this comment

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

nit: can these helper functions go to the top of the module? Otherwise people have to read the code from the bottom up (as I currently am)

Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

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

need to auto-squash before merging

@tzemanovic tzemanovic marked this pull request as ready for review July 13, 2023 08:43
@tzemanovic tzemanovic removed their assignment Jul 13, 2023
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Discussed w/ @yito88

@tzemanovic tzemanovic force-pushed the yuji/fix_changes_before_commit branch from 7207fa9 to d87df2a Compare July 14, 2023 11:37
@tzemanovic
Copy link
Member

auto-squashed fixups (no diff)

tzemanovic added a commit that referenced this pull request Jul 14, 2023
* yuji/fix_changes_before_commit:
  test/shell/finalize_block: add some txs to DB commit test
  add changelog
  fix changes in finalize_block
This was referenced Jul 14, 2023
@Fraccaman Fraccaman merged commit 00d1969 into main Jul 22, 2023
11 of 12 checks passed
@Fraccaman Fraccaman deleted the yuji/fix_changes_before_commit branch July 22, 2023 11:41
@yito88 yito88 mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants