fix(db): change commit return type from Result<bool> to Result<()> (#21077)

Co-authored-by: Sergei Shulepov <pep@tempo.xyz>

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<bool, DatabaseError> {
-        Ok(true)
+    fn commit(self) -> Result<(), DatabaseError> {
+        Ok(())
     }
 
     /// Aborts the transaction.

crates/storage/db-api/src/transaction.rs

@@ -35,7 +35,7 @@ pub trait DbTx: Debug + Send {
     ) -> Result<Option<T::Value>, DatabaseError>;
     /// Commit for read only transaction will consume and free transaction and allows
     /// freeing of memory pages
-    fn commit(self) -> Result<bool, DatabaseError>;
+    fn commit(self) -> Result<(), DatabaseError>;
     /// Aborts transaction
     fn abort(self);
     /// Iterate over read only values in table.

crates/storage/db/src/implementation/mdbx/tx.rs

@@ -302,10 +302,10 @@ impl<K: TransactionKind> DbTx for Tx<K> {
         })
     }
 
-    fn commit(self) -> Result<bool, DatabaseError> {
+    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),
             }
         })

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) })
         }

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

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<Error> 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<bool> {
     match err_code {

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<CommitLatency> {
+        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.

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();

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();
         }
     }
 

crates/storage/provider/src/providers/database/provider.rs

@@ -125,7 +125,7 @@ impl<DB: Database, N: NodeTypes> AsRef<DatabaseProvider<<DB as Database>::TXMut,
 
 impl<DB: Database, N: NodeTypes + 'static> DatabaseProviderRW<DB, N> {
     /// Commit database transaction and static file if it exists.
-    pub fn commit(self) -> ProviderResult<bool> {
+    pub fn commit(self) -> ProviderResult<()> {
         self.0.commit()
     }
 
@@ -3422,7 +3422,7 @@ impl<TX: DbTx + 'static, N: NodeTypes + 'static> DBProvider for DatabaseProvider
     }
 
     /// Commit database transaction, static files, and pending `RocksDB` batches.
-    fn commit(self) -> ProviderResult<bool> {
+    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<TX: DbTx + 'static, N: NodeTypes + 'static> DBProvider for DatabaseProvider
             self.tx.commit()?;
         }
 
-        Ok(true)
+        Ok(())
     }
 }
 

crates/storage/provider/src/test_utils/mock.rs

@@ -267,7 +267,7 @@ impl<T: NodePrimitives, ChainSpec: EthChainSpec + 'static> DBProvider
         self.tx
     }
 
-    fn commit(self) -> ProviderResult<bool> {
+    fn commit(self) -> ProviderResult<()> {
         Ok(self.tx.commit()?)
     }
 

crates/storage/rpc-provider/src/lib.rs

@@ -1357,7 +1357,7 @@ where
         self
     }
 
-    fn commit(self) -> ProviderResult<bool> {
+    fn commit(self) -> ProviderResult<()> {
         unimplemented!("commit not supported for RPC provider")
     }
 

crates/storage/storage-api/src/database_provider.rs

@@ -37,7 +37,7 @@ pub trait DBProvider: Sized {
     }
 
     /// Commit database transaction
-    fn commit(self) -> ProviderResult<bool>;
+    fn commit(self) -> ProviderResult<()>;
 
     /// Returns a reference to prune modes.
     fn prune_modes_ref(&self) -> &PruneModes;

crates/storage/storage-api/src/noop.rs

@@ -639,7 +639,7 @@ impl<ChainSpec: Send + Sync, N: NodePrimitives> DBProvider for NoopProvider<Chai
         &self.prune_modes
     }
 
-    fn commit(self) -> ProviderResult<bool> {
+    fn commit(self) -> ProviderResult<()> {
         use reth_db_api::transaction::DbTx;
 
         Ok(self.tx.commit()?)

docs/crates/db.md

@@ -159,7 +159,7 @@ pub trait DbTx: Debug + Send + Sync {
     ) -> Result<Option<T::Value>, DatabaseError>;
     /// Commit for read only transaction will consume and free transaction and allows
     /// freeing of memory pages
-    fn commit(self) -> Result<bool, DatabaseError>;
+    fn commit(self) -> Result<(), DatabaseError>;
     /// Aborts transaction
     fn abort(self);
     /// Iterate over read only values in table.