diff --git a/crates/storage/db-api/src/mock.rs b/crates/storage/db-api/src/mock.rs index 60f69ae8f0..9928a66c0d 100644 --- a/crates/storage/db-api/src/mock.rs +++ b/crates/storage/db-api/src/mock.rs @@ -92,10 +92,10 @@ impl DbTx for TxMock { /// Commits the transaction. /// - /// **Mock behavior**: Always returns `Ok(true)`, indicating successful commit. + /// **Mock behavior**: Always returns `Ok(())`, indicating successful commit. /// No actual data is persisted since this is a mock implementation. - fn commit(self) -> Result { - Ok(true) + fn commit(self) -> Result<(), DatabaseError> { + Ok(()) } /// Aborts the transaction. diff --git a/crates/storage/db-api/src/transaction.rs b/crates/storage/db-api/src/transaction.rs index 281912267f..545c0ce39f 100644 --- a/crates/storage/db-api/src/transaction.rs +++ b/crates/storage/db-api/src/transaction.rs @@ -35,7 +35,7 @@ pub trait DbTx: Debug + Send { ) -> Result, DatabaseError>; /// Commit for read only transaction will consume and free transaction and allows /// freeing of memory pages - fn commit(self) -> Result; + fn commit(self) -> Result<(), DatabaseError>; /// Aborts transaction fn abort(self); /// Iterate over read only values in table. diff --git a/crates/storage/db/src/implementation/mdbx/tx.rs b/crates/storage/db/src/implementation/mdbx/tx.rs index 25de65c853..c0e958ef81 100644 --- a/crates/storage/db/src/implementation/mdbx/tx.rs +++ b/crates/storage/db/src/implementation/mdbx/tx.rs @@ -302,10 +302,10 @@ impl DbTx for Tx { }) } - fn commit(self) -> Result { + fn commit(self) -> Result<(), DatabaseError> { self.execute_with_close_transaction_metric(TransactionOutcome::Commit, |this| { match this.inner.commit().map_err(|e| DatabaseError::Commit(e.into())) { - Ok((v, latency)) => (Ok(v), Some(latency)), + Ok(latency) => (Ok(()), Some(latency)), Err(e) => (Err(e), None), } }) diff --git a/crates/storage/libmdbx-rs/src/codec.rs b/crates/storage/libmdbx-rs/src/codec.rs index c0b2f0f1cf..91142362c6 100644 --- a/crates/storage/libmdbx-rs/src/codec.rs +++ b/crates/storage/libmdbx-rs/src/codec.rs @@ -42,7 +42,9 @@ impl TableObject for Cow<'_, [u8]> { #[cfg(not(feature = "return-borrowed"))] { let is_dirty = (!K::IS_READ_ONLY) && - crate::error::mdbx_result(ffi::mdbx_is_dirty(_txn, data_val.iov_base))?; + crate::error::mdbx_result(unsafe { + ffi::mdbx_is_dirty(_txn, data_val.iov_base) + })?; Ok(if is_dirty { Cow::Owned(s.to_vec()) } else { Cow::Borrowed(s) }) } diff --git a/crates/storage/libmdbx-rs/src/environment.rs b/crates/storage/libmdbx-rs/src/environment.rs index fa11bf5629..524f434029 100644 --- a/crates/storage/libmdbx-rs/src/environment.rs +++ b/crates/storage/libmdbx-rs/src/environment.rs @@ -989,7 +989,10 @@ mod tests { result @ Err(_) => result.unwrap(), } } - tx.commit().unwrap(); + // The transaction may be in an error state after hitting MapFull, + // so commit could fail. We don't care about the result here since + // the purpose of this test is to verify the HSR callback was called. + let _ = tx.commit(); } // Expect the HSR to be called diff --git a/crates/storage/libmdbx-rs/src/error.rs b/crates/storage/libmdbx-rs/src/error.rs index 2f852c3dc7..007d828af2 100644 --- a/crates/storage/libmdbx-rs/src/error.rs +++ b/crates/storage/libmdbx-rs/src/error.rs @@ -123,6 +123,12 @@ pub enum Error { /// Read transaction has been timed out. #[error("read transaction has been timed out")] ReadTransactionTimeout, + /// The transaction commit was aborted due to previous errors. + /// + /// This can happen in exceptionally rare cases and it signals the problem coming from inside + /// of mdbx. + #[error("botched transaction")] + BotchedTransaction, /// Permission defined #[error("permission denied to setup database")] Permission, @@ -204,6 +210,7 @@ impl Error { Self::WriteTransactionUnsupportedInReadOnlyMode | Self::NestedTransactionsUnsupportedWithWriteMap => ffi::MDBX_EACCESS, Self::ReadTransactionTimeout => -96000, // Custom non-MDBX error code + Self::BotchedTransaction => -96001, Self::Permission => ffi::MDBX_EPERM, Self::Other(err_code) => *err_code, } @@ -216,6 +223,14 @@ impl From for i32 { } } +/// Parses an MDBX error code into a result type. +/// +/// Note that this function returns `Ok(false)` on `MDBX_SUCCESS` and +/// `Ok(true)` on `MDBX_RESULT_TRUE`. The return value requires extra +/// care since its interpretation depends on the callee being called. +/// +/// The most unintuitive case is `mdbx_txn_commit` which returns `Ok(true)` +/// when the commit has been aborted. #[inline] pub(crate) const fn mdbx_result(err_code: c_int) -> Result { match err_code { diff --git a/crates/storage/libmdbx-rs/src/transaction.rs b/crates/storage/libmdbx-rs/src/transaction.rs index 913f5156b5..f0f4f120ae 100644 --- a/crates/storage/libmdbx-rs/src/transaction.rs +++ b/crates/storage/libmdbx-rs/src/transaction.rs @@ -170,8 +170,8 @@ where /// Commits the transaction. /// /// Any pending operations will be saved. - pub fn commit(self) -> Result<(bool, CommitLatency)> { - let result = self.txn_execute(|txn| { + pub fn commit(self) -> Result { + match self.txn_execute(|txn| { if K::IS_READ_ONLY { #[cfg(feature = "read-tx-timeouts")] self.env().txn_manager().remove_active_read_transaction(txn); @@ -186,10 +186,21 @@ where .send_message(TxnManagerMessage::Commit { tx: TxnPtr(txn), sender }); rx.recv().unwrap() } - })?; - - self.inner.set_committed(); - result + })? { + // + Ok((false, lat)) => { + self.inner.set_committed(); + Ok(lat) + } + Ok((true, _)) => { + // MDBX_RESULT_TRUE means the transaction was aborted due to prior errors. + // The transaction is still finished/freed by MDBX, so we must mark it as + // committed to prevent the Drop impl from trying to abort it again. + self.inner.set_committed(); + Err(Error::BotchedTransaction) + } + Err(e) => Err(e), + } } /// Opens a handle to an MDBX database. diff --git a/crates/storage/libmdbx-rs/tests/cursor.rs b/crates/storage/libmdbx-rs/tests/cursor.rs index afb9cee958..0d483baf29 100644 --- a/crates/storage/libmdbx-rs/tests/cursor.rs +++ b/crates/storage/libmdbx-rs/tests/cursor.rs @@ -111,7 +111,7 @@ fn test_iter() { for (key, data) in &items { txn.put(db.dbi(), key, data, WriteFlags::empty()).unwrap(); } - assert!(!txn.commit().unwrap().0); + txn.commit().unwrap(); } let txn = env.begin_ro_txn().unwrap(); diff --git a/crates/storage/libmdbx-rs/tests/transaction.rs b/crates/storage/libmdbx-rs/tests/transaction.rs index 7e4b18e4fd..81da1beada 100644 --- a/crates/storage/libmdbx-rs/tests/transaction.rs +++ b/crates/storage/libmdbx-rs/tests/transaction.rs @@ -148,13 +148,13 @@ fn test_clear_db() { { let txn = env.begin_rw_txn().unwrap(); txn.put(txn.open_db(None).unwrap().dbi(), b"key", b"val", WriteFlags::empty()).unwrap(); - assert!(!txn.commit().unwrap().0); + txn.commit().unwrap(); } { let txn = env.begin_rw_txn().unwrap(); txn.clear_db(txn.open_db(None).unwrap().dbi()).unwrap(); - assert!(!txn.commit().unwrap().0); + txn.commit().unwrap(); } let txn = env.begin_ro_txn().unwrap(); @@ -178,7 +178,7 @@ fn test_drop_db() { .unwrap(); // Workaround for MDBX dbi drop issue txn.create_db(Some("canary"), DatabaseFlags::empty()).unwrap(); - assert!(!txn.commit().unwrap().0); + txn.commit().unwrap(); } { let txn = env.begin_rw_txn().unwrap(); @@ -187,7 +187,7 @@ fn test_drop_db() { txn.drop_db(dbi).unwrap(); } assert!(matches!(txn.open_db(Some("test")).unwrap_err(), Error::NotFound)); - assert!(!txn.commit().unwrap().0); + txn.commit().unwrap(); } } diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 4bde73a37f..78424785cc 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -125,7 +125,7 @@ impl AsRef::TXMut, impl DatabaseProviderRW { /// Commit database transaction and static file if it exists. - pub fn commit(self) -> ProviderResult { + pub fn commit(self) -> ProviderResult<()> { self.0.commit() } @@ -3422,7 +3422,7 @@ impl DBProvider for DatabaseProvider } /// Commit database transaction, static files, and pending `RocksDB` batches. - fn commit(self) -> ProviderResult { + fn commit(self) -> ProviderResult<()> { // For unwinding it makes more sense to commit the database first, since if // it is interrupted before the static files commit, we can just // truncate the static files according to the @@ -3453,7 +3453,7 @@ impl DBProvider for DatabaseProvider self.tx.commit()?; } - Ok(true) + Ok(()) } } diff --git a/crates/storage/provider/src/test_utils/mock.rs b/crates/storage/provider/src/test_utils/mock.rs index 065b9ee71d..f5c40978a7 100644 --- a/crates/storage/provider/src/test_utils/mock.rs +++ b/crates/storage/provider/src/test_utils/mock.rs @@ -267,7 +267,7 @@ impl DBProvider self.tx } - fn commit(self) -> ProviderResult { + fn commit(self) -> ProviderResult<()> { Ok(self.tx.commit()?) } diff --git a/crates/storage/rpc-provider/src/lib.rs b/crates/storage/rpc-provider/src/lib.rs index 76f4802188..fa340ea2ae 100644 --- a/crates/storage/rpc-provider/src/lib.rs +++ b/crates/storage/rpc-provider/src/lib.rs @@ -1357,7 +1357,7 @@ where self } - fn commit(self) -> ProviderResult { + fn commit(self) -> ProviderResult<()> { unimplemented!("commit not supported for RPC provider") } diff --git a/crates/storage/storage-api/src/database_provider.rs b/crates/storage/storage-api/src/database_provider.rs index b206ca0922..417f8e282b 100644 --- a/crates/storage/storage-api/src/database_provider.rs +++ b/crates/storage/storage-api/src/database_provider.rs @@ -37,7 +37,7 @@ pub trait DBProvider: Sized { } /// Commit database transaction - fn commit(self) -> ProviderResult; + fn commit(self) -> ProviderResult<()>; /// Returns a reference to prune modes. fn prune_modes_ref(&self) -> &PruneModes; diff --git a/crates/storage/storage-api/src/noop.rs b/crates/storage/storage-api/src/noop.rs index b3d2c7a909..8e912c23a4 100644 --- a/crates/storage/storage-api/src/noop.rs +++ b/crates/storage/storage-api/src/noop.rs @@ -639,7 +639,7 @@ impl DBProvider for NoopProvider ProviderResult { + fn commit(self) -> ProviderResult<()> { use reth_db_api::transaction::DbTx; Ok(self.tx.commit()?) diff --git a/docs/crates/db.md b/docs/crates/db.md index 38dc31736f..f6460b6c12 100644 --- a/docs/crates/db.md +++ b/docs/crates/db.md @@ -159,7 +159,7 @@ pub trait DbTx: Debug + Send + Sync { ) -> Result, DatabaseError>; /// Commit for read only transaction will consume and free transaction and allows /// freeing of memory pages - fn commit(self) -> Result; + fn commit(self) -> Result<(), DatabaseError>; /// Aborts transaction fn abort(self); /// Iterate over read only values in table.