-
Notifications
You must be signed in to change notification settings - Fork 955
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
Conversation
@@ -481,6 +471,7 @@ where | |||
); | |||
stats.increment_errored_txs(); | |||
|
|||
self.wl_storage.drop_tx(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in f30e158
|
||
/// Make a wrapper tx and a processed tx from the wrapped tx that can be | ||
/// added to `FinalizeBlock` request. | ||
fn mk_wrapper_tx( |
There was a problem hiding this comment.
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)
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed w/ @yito88
7207fa9
to
d87df2a
Compare
auto-squashed fixups (no diff) |
* yuji/fix_changes_before_commit: test/shell/finalize_block: add some txs to DB commit test add changelog fix changes in finalize_block
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.