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
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Fix inconsistency state before commit
([\#1709](https://github.com/anoma/namada/issues/1709))
37 changes: 21 additions & 16 deletions apps/src/lib/node/ledger/shell/finalize_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,6 @@ where
let tx_hash_key =
replay_protection::get_tx_hash_key(&tx_hash);
self.wl_storage
.storage
.delete(&tx_hash_key)
.expect("Error while deleting tx hash from storage");
}
Expand All @@ -221,16 +220,14 @@ where
processed_tx.header_hash().0,
));
self.wl_storage
.storage
.write(&wrapper_tx_hash_key, vec![])
.write_bytes(&wrapper_tx_hash_key, vec![])
.expect("Error while writing tx hash to storage");

let inner_tx_hash_key = replay_protection::get_tx_hash_key(
&tx.clone().update_header(TxType::Raw).header_hash(),
);
self.wl_storage
.storage
.write(&inner_tx_hash_key, vec![])
.write_bytes(&inner_tx_hash_key, vec![])
.expect("Error while writing tx hash to storage");

#[cfg(not(feature = "mainnet"))]
Expand All @@ -256,11 +253,7 @@ where
match balance.checked_sub(wrapper_fees) {
Some(amount) => {
self.wl_storage
.storage
.write(
&balance_key,
amount.try_to_vec().unwrap(),
)
.write(&balance_key, amount)
.unwrap();
}
None => {
Expand All @@ -271,12 +264,9 @@ where
if reject {
// Burn remaining funds
self.wl_storage
.storage
.write(
&balance_key,
Amount::native_whole(0)
.try_to_vec()
.unwrap(),
Amount::native_whole(0),
)
.unwrap();
tx_event["info"] =
Expand Down Expand Up @@ -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

// If transaction type is Decrypted and failed because of
// out of gas, remove its hash from storage to allow
// rewrapping it
Expand All @@ -491,15 +482,16 @@ where
let tx_hash_key =
replay_protection::get_tx_hash_key(&hash);
self.wl_storage
.storage
.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.

}
}

self.wl_storage.drop_tx();
tx_event["gas_used"] = self
.gas_meter
.get_current_transaction_gas()
Expand Down Expand Up @@ -1834,7 +1826,14 @@ mod test_finalize_block {
votes: votes.clone(),
..Default::default()
};
// merkle tree root before finalize_block
let root_pre = shell.shell.wl_storage.storage.block.tree.root();

let _events = shell.finalize_block(req).unwrap();

// 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

let new_state = store_block_state(&shell);
// The new state must be unchanged
itertools::assert_equal(
Expand Down Expand Up @@ -2226,6 +2225,8 @@ mod test_finalize_block {
},
};
shell.enqueue_tx(wrapper_tx);
// merkle tree root before finalize_block
let root_pre = shell.shell.wl_storage.storage.block.tree.root();

let _event = &shell
.finalize_block(FinalizeBlock {
Expand All @@ -2234,6 +2235,10 @@ mod test_finalize_block {
})
.expect("Test failed")[0];

// 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);
tzemanovic marked this conversation as resolved.
Show resolved Hide resolved

// FIXME: uncomment when proper gas metering is in place
// // Check inner tx hash has been removed from storage
// assert_eq!(event.event_type.to_string(), String::from("applied"));
Expand Down
7 changes: 6 additions & 1 deletion core/src/ledger/storage/wl_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@ where
/// Commit the current block's write log to the storage and commit the block
/// to DB. Starts a new block write log.
pub fn commit_block(&mut self) -> storage_api::Result<()> {
if self.storage.last_epoch != self.storage.block.epoch {
self.storage
.update_epoch_in_merkle_tree()
.into_storage_result()?;
}

let mut batch = D::batch();
self.write_log
.commit_block(&mut self.storage, &mut batch)
Expand Down Expand Up @@ -205,7 +211,6 @@ where
.new_epoch(height, evidence_max_age_num_blocks);
tracing::info!("Began a new epoch {}", self.storage.block.epoch);
}
self.storage.update_epoch_in_merkle_tree()?;
Ok(new_epoch)
}
}
Expand Down