fix(db): return error when MDBX commit is aborted

MDBX's transaction commit returns MDBX_RESULT_TRUE (-1) when the
transaction was aborted due to previous errors. The Rust wrapper
converts this to Ok(true), but reth's DbTx::commit() implementation
was passing this through as a successful result.

Since commit() is typically used as `provider_rw.commit()?` throughout
the codebase, which only checks for Err variants, these silent failures
were going unnoticed - potentially leading to data loss.

This commit:
- Changes DbTx::commit() return type from Result<bool, _> to Result<(), _>
- Maps MDBX_RESULT_TRUE to a proper error in the mdbx tx implementation
- Updates all trait implementations to match the new signature

The bool return value was not useful anyway since it only indicated
whether the commit was a no-op (MDBX_RESULT_TRUE = aborted) vs real
success (MDBX_SUCCESS/false), and callers never checked it.

See: https://libmdbx.dqdkfa.ru/group__c__transactions.html#ga3146a36cf1d0603cc6e3d52e97ec38a9
Amp-Thread-ID: https://ampcode.com/threads/T-019bbe8c-b7b0-722a-8f37-7d4c19682b06
Co-authored-by: Amp <amp@ampcode.com>

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

@@ -295,10 +295,18 @@ 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((true, _)) => (
+                    Err(DatabaseError::Commit(
+                        "transaction was aborted due to a previous error (MDBX_RESULT_TRUE)"
+                            .to_string()
+                            .into(),
+                    )),
+                    None,
+                ),
+                Ok((false, latency)) => (Ok(()), Some(latency)),
                 Err(e) => (Err(e), None),
             }
         })

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/providers/static_file/manager.rs

@@ -1361,38 +1361,21 @@ impl<N: NodePrimitives> StaticFileProvider<N> {
         Provider: DBProvider + BlockReader + StageCheckpointReader,
     {
         debug!(target: "reth::providers::static_file", ?segment, ?highest_static_file_entry, ?highest_static_file_block, "Ensuring invariants");
+
+        // If the static file is truly empty (never had data), this is a valid scenario when
+        // enabling static files for the first time on an existing node. No invariants to check.
+        if highest_static_file_entry.is_none() && highest_static_file_block.is_none() {
+            debug!(target: "reth::providers::static_file", ?segment, "Static file is empty, allowing fresh start");
+            return Ok(None);
+        }
+
         let mut db_cursor = provider.tx_ref().cursor_read::<T>()?;
 
-        if let Some((db_first_entry, _)) = db_cursor.first()? {
-            debug!(target: "reth::providers::static_file", ?segment, db_first_entry, "Found first database entry");
-            if let (Some(highest_entry), Some(highest_block)) =
-                (highest_static_file_entry, highest_static_file_block)
-            {
-                // If there is a gap between the entry found in static file and
-                // database, then we have most likely lost static file data and need to unwind so we
-                // can load it again
-                if !(db_first_entry <= highest_entry || highest_entry + 1 == db_first_entry) {
-                    info!(
-                        target: "reth::providers::static_file",
-                        ?db_first_entry,
-                        ?highest_entry,
-                        unwind_target = highest_block,
-                        ?segment,
-                        "Setting unwind target."
-                    );
-                    return Ok(Some(highest_block))
-                }
-            }
-
-            if let Some((db_last_entry, _)) = db_cursor.last()? &&
-                highest_static_file_entry
-                    .is_none_or(|highest_entry| db_last_entry > highest_entry)
-            {
-                debug!(target: "reth::providers::static_file", ?segment, db_last_entry, ?highest_static_file_entry, "Database has entries beyond static files, no unwind needed");
-                return Ok(None)
-            }
-        } else {
-            debug!(target: "reth::providers::static_file", ?segment, "No database entries found");
+        if let Some((db_last_entry, _)) = db_cursor.last()? &&
+            highest_static_file_entry.is_none_or(|highest_entry| db_last_entry > highest_entry)
+        {
+            debug!(target: "reth::providers::static_file", ?segment, db_last_entry, ?highest_static_file_entry, "Database has entries beyond static files, no unwind needed");
+            return Ok(None)
         }
 
         let highest_static_file_entry = highest_static_file_entry.unwrap_or_default();

crates/storage/provider/src/providers/static_file/writer.rs

@@ -428,8 +428,9 @@ impl<N: NodePrimitives> StaticFileProviderRW<N> {
         let current_block = if let Some(current_block_number) = self.current_block_number() {
             current_block_number
         } else {
-            self.increment_block(0)?;
-            0
+            let start_block = self.writer.user_header().expected_block_start();
+            self.increment_block(start_block)?;
+            start_block
         };
 
         match current_block.cmp(&advance_to) {
@@ -456,6 +457,20 @@ impl<N: NodePrimitives> StaticFileProviderRW<N> {
     pub fn increment_block(&mut self, expected_block_number: BlockNumber) -> ProviderResult<()> {
         let segment = self.writer.user_header().segment();
 
+        // If the static file is empty and the expected block is beyond expected_block_start,
+        // we need to advance to that block first. This can happen when enabling static files
+        // on an existing node where the first block to write is not at the segment boundary.
+        if self.writer.user_header().block_end().is_none() {
+            let start_block = self.writer.user_header().expected_block_start();
+            if expected_block_number > start_block {
+                // Initialize the block range and advance to expected_block_number - 1
+                self.writer.user_header_mut().increment_block();
+                for block in start_block + 1..expected_block_number {
+                    self.writer.user_header_mut().increment_block();
+                }
+            }
+        }
+
         self.check_next_block_number(expected_block_number)?;
 
         let start = Instant::now();

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.