diff --git a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs index a50650c7afe..5ed9db30a2a 100644 --- a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -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 @@ -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 @@ -488,6 +485,8 @@ where response.events.extend( // ibc events result + // FIXME: am I populating this + // thing? I think so .ibc_events .iter() .cloned() @@ -532,7 +531,6 @@ where } stats.increment_rejected_txs(); - self.state.drop_tx(); } } Err(e) => { @@ -559,7 +557,6 @@ where } } stats.increment_rejected_txs(); - self.state.drop_tx(); } } } @@ -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 @@ -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 @@ -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 @@ -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)); } } diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 7b9c57ab0b1..109e6c1e5c1 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -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(); + } } }; } @@ -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 { diff --git a/crates/state/src/write_log.rs b/crates/state/src/write_log.rs index 2a86576aa9e..4c03a82aa89 100644 --- a/crates/state/src/write_log.rs +++ b/crates/state/src/write_log.rs @@ -164,8 +164,6 @@ struct BatchedTxWriteLog { address_gen: Option, // The storage modifications for the transaction write_log: HashMap, - // The IBC events for the current transaction - ibc_events: BTreeSet, } // FIXME: problem is. how would I read from this. If I read the write log I @@ -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(); @@ -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); @@ -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; } } @@ -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();