Skip to content

Commit

Permalink
Misc updates to write log. Updates tx result handling in `finalize_bl…
Browse files Browse the repository at this point in the history 
…ock`
  • Loading branch information
@grarco
grarco committed Apr 24, 2024
1 parent 3a846bd commit afa7511
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 52 deletions.
51 changes: 40 additions & 11 deletions crates/apps/src/lib/node/ledger/shell/finalize_block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ where
for _ in 0..commitments_len {
stats.increment_rejected_txs();
}
self.state.drop_tx();
self.state.write_log_mut().drop_batch();
tx_event.extend(Code(ResultCode::InvalidTx));
} else {
for (cmt_hash, batched_result) in
Expand Down Expand Up @@ -476,9 +476,6 @@ where
);
stats.increment_successful_txs();
commit_batch_hash = true;
// FIXME: need to commit only the
// precommit of this specific inner tx
self.state.commit_tx();
self.state
.in_mem_mut()
.block
Expand All @@ -488,6 +485,8 @@ where
response.events.extend(
// ibc events
result
// FIXME: am I populating this
// thing? I think so
.ibc_events
.iter()
.cloned()
Expand Down Expand Up @@ -532,7 +531,6 @@ where
}

stats.increment_rejected_txs();
self.state.drop_tx();
}
}
Err(e) => {
Expand All @@ -559,7 +557,6 @@ where
}
}
stats.increment_rejected_txs();
self.state.drop_tx();
}
}
}
Expand All @@ -568,6 +565,7 @@ where
tx_event.extend(Code(ResultCode::Ok));
}

self.state.write_log_mut().commit_batch();
if commit_batch_hash {
// If at least one of the inner txs of the batch
// requires its hash to be committed than commit the
Expand Down Expand Up @@ -605,6 +603,9 @@ where
.extend(WithGasUsed(tx_gas_meter.get_tx_consumed_gas()))
.extend(Info(msg.to_string()))
.extend(Code(ResultCode::InvalidTx));
// FIXME: should drop the write log here?
// FIXME: should make sure that I clean the write log before
// analyzing the next tx.
}
Err(msg) => {
// This branch represents an error that affects the entire
Expand All @@ -622,8 +623,13 @@ where
// need to always produce the event with the batch
// attribute. But this is probably the only solution

// FIXME: because of this I think I have onesolutions:
// I modify the writeLog and keep precommitting in
// dispatch_tx, but in here I'd need to know which
// transaction should be committed and which one should not.
// Commit everything in the batch and discard the last one

// If user transaction
// FIXME: this could be wrong
// didn't fail because of out of gas nor
// invalid section commitment, commit
// its hash to prevent replays
Expand Down Expand Up @@ -651,14 +657,37 @@ where
}
}

// FIXME: how to manage this? It depends how many txs of the
// non-atomic batch have been succesfully committed
stats.increment_errored_txs();
self.state.drop_tx();
// if is_atomic_batch {
// FIXME: should we commit the valid txs of the batch if it
// is non-atomic?
for _ in 0..commitments_len {
stats.increment_rejected_txs();
}
self.state.write_log_mut().drop_batch();
// } else {
// //FIXME: also, I don't have TxResult here so I cannot
// log anything! -> I need it //FIXME:
// options: // - I don't return
// an error in case of out of gas, but just Ok and I abort
// the execution so that I have the TxResult -> But I'd
// still need to tell that there was a gas error
// // - I create the TxResult before and pass it as
// an argument to dispatch_Tx so that I can update it anyway

// //FIXME: I need the amount of entries in the batch
// log to compute the difference for
// _ in 0..(commitments_len - self.state) {
// stats.increment_errored_txs();
// }
// // Commit the transaction that were accepted before
// the failure self.state.
// write_log_mut().commit_batch(); }

tx_event
.extend(WithGasUsed(tx_gas_meter.get_tx_consumed_gas()))
.extend(Info(msg.to_string()))
// FIXME: correct to mark it as invalid even if some txs
// might have been committed?
.extend(Code(ResultCode::InvalidTx));
}
}
Expand Down
66 changes: 47 additions & 19 deletions crates/namada/src/ledger/protocol/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,29 +250,52 @@ where
tx_wasm_cache,
},
) {
// Err(e @ Error::GasError(_)) => {
// // Gas error aborts the execution of the entire batch
// // FIXME: maybe implement a method on Error called
// // recoverable() and check that here?
// state.write_log_mut().drop_tx();
// return Err(e);
// }
// res @ Err(_) => {
// // Need to drop to prevent next txs in the batch from
// // reading invalid states
// state.write_log_mut().drop_tx();
// tx_result.batch_results.insert(cmt.get_hash(), res);
// }
// // FIXME: we keep going even for atomic batches which
// could // instead be aborted, should
// we do that? res @ Ok(_) => {
// // FIXME: wait what about the events? I should be
// able // to precommit those too!!!
// //FIXME: if the transaction was rejected I MUST NOT
// commit it to the batch!!! But I still must update the
// tx_result match
// state.write_log_mut().commit_tx_to_batch(cmt);
// tx_result.batch_results.insert(cmt.get_hash(), res);
// }
// FIXME: improve
// FIXME: we keep going even for atomic batches which could
// instead be aborted, should we do that?
Err(e @ Error::GasError(_)) => {
// Gas error aborts the exeuction of the entire batch
// FIXME: maybe implement a method on Error called
// recoverable() and check that here?
state.write_log_mut().drop_tx_keep_precommit();
// Gas error aborts the execution of the entire batch
state.write_log_mut().drop_tx();
// FIXME: should push something to the batch results in
// this case?
return Err(e);
}
res @ Err(_) => {
// Need to drop to prevent next txs in the batch from
// reading invalid states FIXME:
// this means that the precommit field in the write log
// cannot be a vector cause the indexes don't
// necessarely match
state.write_log_mut().drop_tx_keep_precommit();
tx_result.batch_results.insert(cmt.get_hash(), res);
}
// FIXME: we keep going even for atomic batches which could
// instead be aborted, should we do that?
res @ Ok(_) => {
// FIXME: wait what about the events? I should be able
// to precommit those too!!!
state.write_log_mut().precommit_tx();
res => {
// FIXME: improve
let is_accepted = match &res {
Ok(result) if result.is_accepted() => true,
_ => false,
};
tx_result.batch_results.insert(cmt.get_hash(), res);
if is_accepted {
state.write_log_mut().commit_tx_to_batch(cmt);
} else {
state.write_log_mut().drop_tx();
}
}
};
}
Expand Down Expand Up @@ -692,6 +715,11 @@ where

let initialized_accounts = state.write_log().get_initialized_accounts();
let changed_keys = state.write_log().get_keys();
// FIXME: if I return early the ibc events could still be populated and it
// remaing the write log until I commit or drop, meaning that I post in the
// events the same ibc events multiple times FIXME: I could fix this by
// committing for the non-atomic batch, but what about the atomic ones? I'd
// still have the duplicated entries!
let ibc_events = state.write_log_mut().take_ibc_events();

Ok(BatchedTxResult {
Expand Down
30 changes: 8 additions & 22 deletions crates/state/src/write_log.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ struct BatchedTxWriteLog {
address_gen: Option<EstablishedAddressGen>,
// The storage modifications for the transaction
write_log: HashMap<storage::Key, StorageModification>,
// The IBC events for the current transaction
ibc_events: BTreeSet<IbcEvent>,
}

// FIXME: problem is. how would I read from this. If I read the write log I
Expand Down Expand Up @@ -661,6 +659,9 @@ impl WriteLog {
/// Commit the current transaction's write log and precommit log to the
/// batch when it's accepted by all the triggered validity predicates.
/// Starts a new transaction write log.
// FIXME: do I need this? Better, do I need to pass the cmt? No, more in
// generale the batch_write_log does not need to be indexed and can be just
// a Vec
pub fn commit_tx_to_batch(&mut self, cmt: &Commitments) {
// First precommit everything
self.precommit_tx();
Expand All @@ -674,8 +675,6 @@ impl WriteLog {
let batched_log = BatchedTxWriteLog {
address_gen: tx_write_log.address_gen,
write_log: tx_write_log.precommit_write_log,
// FIXME: need the events in here?
ibc_events: tx_write_log.ibc_events,
};

self.batch_write_log.insert(cmt.get_hash(), batched_log);
Expand All @@ -696,23 +695,11 @@ impl WriteLog {
self.tx_write_log.write_log.clear();
}

/// Commit the log of the specified commitment from the batch log to the
/// block log.
pub fn commit_batched_tx(&mut self, cmt: &Commitments) {
match self.batch_write_log.shift_remove(&cmt.get_hash()) {
Some(batched_log) => {
self.block_write_log.extend(batched_log.write_log);
self.block_address_gen = batched_log.address_gen;
}
// FIXME: return an error in this case
None => (),
}
}

/// Drop the log of the specified commitment from the batch bucket.
pub fn drop_batched_tx(&mut self, cmt: &Commitments) {
if self.batch_write_log.shift_remove(&cmt.get_hash()).is_none() {
// FIXME: return an error
/// Commit the entire batch to the block log.
pub fn commit_batch(&mut self) {
for (_, log) in std::mem::take(&mut self.batch_write_log) {
self.block_write_log.extend(log.write_log);
self.block_address_gen = log.address_gen;
}
}

Expand All @@ -721,7 +708,6 @@ impl WriteLog {
self.batch_write_log = Default::default();
}

// FIXME: pass something to enforce that is a wrapper? Maybe not
pub fn commit_tx(&mut self) {
// First precommit everything
self.precommit_tx();
Expand Down

0 comments on commit afa7511

Please sign in to comment.