Skip to content

Commit

Permalink
fix(api): Fix transaction methods for pruned transactions (#2168)
Browse files Browse the repository at this point in the history
## What ❔

Reworks `TxCache` for full nodes to look more like a mempool:

- Retains transactions from it when they are sent to the main node
- Gets rid of querying transactions from the main node in `TxProxy`.

## Why ❔

Right now, two transaction-related methods (`eth_getTransactionByHash`
and `zks_getTransactionDetails`) return data for pruned transactions.
Some other methods (e.g., `eth_getTransactionReceipt`) do not return
data for pruned transactions (i.e., return `null`). This looks
inconsistent; also may be wasteful w.r.t. calls to the main node.

## Checklist

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zk fmt` and `zk lint`.
- [x] Spellcheck has been run via `zk spellcheck`.
  • Loading branch information
slowli committed Jun 12, 2024
1 parent 1cb0887 commit 00c4cca
Show file tree
Hide file tree
Showing 7 changed files with 681 additions and 119 deletions.
2 changes: 1 addition & 1 deletion core/lib/dal/src/models/storage_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ impl From<StorageTransactionReceipt> for TransactionReceipt {
}

#[derive(Debug, Clone, sqlx::FromRow)]
pub struct StorageTransactionDetails {
pub(crate) struct StorageTransactionDetails {
pub is_priority: bool,
pub initiator_address: Vec<u8>,
pub gas_limit: Option<BigDecimal>,
Expand Down
76 changes: 75 additions & 1 deletion core/lib/db_connection/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use sqlx::{
use crate::{
connection_pool::ConnectionPool,
error::{DalConnectionError, DalResult},
instrument::InstrumentExt,
metrics::CONNECTION_METRICS,
utils::InternalMarker,
};
Expand Down Expand Up @@ -183,6 +184,7 @@ impl<'a, DB: DbMarker> Connection<'a, DB> {
}
}

/// Starts a transaction or a new checkpoint within the current transaction.
pub async fn start_transaction(&mut self) -> DalResult<Connection<'_, DB>> {
let (conn, tags) = self.conn_and_tags();
let inner = ConnectionInner::Transaction {
Expand All @@ -198,6 +200,24 @@ impl<'a, DB: DbMarker> Connection<'a, DB> {
})
}

/// Starts building a new transaction with custom settings. Unlike [`Self::start_transaction()`], this method
/// will error if called from a transaction; it is a logical error to change transaction settings in the middle of it.
pub fn transaction_builder(&mut self) -> DalResult<TransactionBuilder<'_, 'a, DB>> {
if let ConnectionInner::Transaction { tags, .. } = &self.inner {
let err = io::Error::new(
io::ErrorKind::Other,
"`Connection::transaction_builder()` can only be invoked outside of a transaction",
);
return Err(
DalConnectionError::start_transaction(sqlx::Error::Io(err), tags.cloned()).into(),
);
}
Ok(TransactionBuilder {
connection: self,
is_readonly: false,
})
}

/// Checks if the `Connection` is currently within database transaction.
pub fn in_transaction(&self) -> bool {
matches!(self.inner, ConnectionInner::Transaction { .. })
Expand Down Expand Up @@ -260,9 +280,36 @@ impl<'a, DB: DbMarker> Connection<'a, DB> {
}
}

/// Builder of transactions allowing to configure transaction characteristics (for now, just its readonly status).
#[derive(Debug)]
pub struct TransactionBuilder<'a, 'c, DB: DbMarker> {
connection: &'a mut Connection<'c, DB>,
is_readonly: bool,
}

impl<'a, DB: DbMarker> TransactionBuilder<'a, '_, DB> {
/// Sets the readonly status of the created transaction.
pub fn set_readonly(mut self) -> Self {
self.is_readonly = true;
self
}

/// Builds the transaction with the provided characteristics.
pub async fn build(self) -> DalResult<Connection<'a, DB>> {
let mut transaction = self.connection.start_transaction().await?;
if self.is_readonly {
sqlx::query("SET TRANSACTION READ ONLY")
.instrument("set_transaction_characteristics")
.execute(&mut transaction)
.await?;
}
Ok(transaction)
}
}

#[cfg(test)]
mod tests {
use crate::{connection_pool::ConnectionPool, utils::InternalMarker};
use super::*;

#[tokio::test]
async fn processor_tags_propagate_to_transactions() {
Expand Down Expand Up @@ -296,4 +343,31 @@ mod tests {
assert!(traced.is_empty());
}
}

#[tokio::test]
async fn creating_readonly_transaction() {
let pool = ConnectionPool::<InternalMarker>::constrained_test_pool(1).await;
let mut connection = pool.connection().await.unwrap();
let mut readonly_transaction = connection
.transaction_builder()
.unwrap()
.set_readonly()
.build()
.await
.unwrap();
assert!(readonly_transaction.in_transaction());

sqlx::query("SELECT COUNT(*) AS \"count?\" FROM miniblocks")
.instrument("test")
.fetch_optional(&mut readonly_transaction)
.await
.unwrap()
.expect("no row returned");
// Check that it's impossible to execute write statements in the transaction.
sqlx::query("DELETE FROM miniblocks")
.instrument("test")
.execute(&mut readonly_transaction)
.await
.unwrap_err();
}
}
Loading

0 comments on commit 00c4cca

Please sign in to comment.