Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- The mempool's transaction capacity is now configurable ([#1433](https://github.com/0xMiden/miden-node/pull/1433)).
- Renamed card's names in the `miden-network-monitor` binary ([#1441](https://github.com/0xMiden/miden-node/pull/1441)).
- Integrated RPC stack with Validator component for transaction validation ([#1457](https://github.com/0xMiden/miden-node/pull/1457)).
- Added validated transactions check to block validation logc in Validator ([#1460](https://github.com/0xMiden/miden-node/pull/1460)).

### Changes

Expand Down
60 changes: 60 additions & 0 deletions crates/validator/src/block_validation/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use std::sync::Arc;

use miden_lib::block::build_block;
use miden_objects::ProposedBlockError;
use miden_objects::block::{BlockNumber, BlockSigner, ProposedBlock};
use miden_objects::crypto::dsa::ecdsa_k256_keccak::Signature;
use miden_objects::transaction::TransactionId;

use crate::server::ValidatedTransactions;

// BLOCK VALIDATION ERROR
// ================================================================================================

#[derive(thiserror::Error, Debug)]
pub enum BlockValidationError {
#[error("transaction {0} in block {1} has not been validated")]
TransactionNotValidated(TransactionId, BlockNumber),
#[error("failed to build block")]
BlockBuildingFailed(#[from] ProposedBlockError),
}

// BLOCK VALIDATION
// ================================================================================================

/// Validates a block by checking that all transactions in the proposed block have been processed by
/// the validator in the past.
///
/// Removes the validated transactions from the cache upon success.
pub async fn validate_block<S: BlockSigner>(
proposed_block: ProposedBlock,
signer: &S,
validated_transactions: Arc<ValidatedTransactions>,
) -> Result<Signature, BlockValidationError> {
// Build the block.
let (header, body) = build_block(proposed_block)?;

// Check that all transactions in the proposed block have been validated
let validated_txs = validated_transactions.read().await;
for tx_header in body.transactions().as_slice() {
let tx_id = tx_header.id();
if !validated_txs.contains_key(&tx_id) {
return Err(BlockValidationError::TransactionNotValidated(tx_id, header.block_num()));
}
}
// Release the validated transactions read lock.
drop(validated_txs);

// Sign the header.
let signature = signer.sign(&header);

// Remove the validated transactions from the cache.
let mut validated_txs = validated_transactions.write().await;
for tx_header in body.transactions().as_slice() {
validated_txs.remove(&tx_header.id());
}
// Release the validated transactions write lock.
drop(validated_txs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not manually managing the locks as it becomes easy to refactor your way into lock issues. How about replacing the lock type alias with a proper struct ValidatedTransactions instead?

#[derive(Default, Clone)]
struct ValidatedTransactions(RwLock<HashMap<TransactionId, TransactionHeader>>);

impl ValidatedTransactions {
    pub async fn check(&self, txs: impl Iterator<TransactionId>) -> Result<(), BlockValidationError>{
        let inner = self.0.read().await;

        ...
    }

    pub async fn remove(&self, txs: ...) {
        let inner = self.0.read().await;
        for tx in txs {
            inner.remove(tx);
        }
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although thinking on this some more, I don't think we can actually remove these transactions at this stage.

We should only remove transactions once we know that the block has successfully landed in the store? Right now we're assuming that if this function is called that the caller successfully completes its end i.e. we're being optimistic.

Do we not need two methods, one for signing and one for the store to submit a BlockCommitted(TransactionIDs)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the transactions around for a long time. As I mentioned in my other comment, until we have persistence implemented, we could probably use something like an LRU cache (with fairly generous capacity) to keep transactions around. This way, we should be able to avoid the need for manual dropping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Lru impl


Ok(signature)
}
1 change: 1 addition & 0 deletions crates/validator/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod block_validation;
mod server;
mod tx_validation;

Expand Down
47 changes: 37 additions & 10 deletions crates/validator/src/server/mod.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,37 @@
use std::collections::HashMap;
use std::net::SocketAddr;
use std::sync::Arc;
use std::time::Duration;

use anyhow::Context;
use miden_lib::block::build_block;
use miden_node_proto::generated::validator::api_server;
use miden_node_proto::generated::{self as proto};
use miden_node_proto_build::validator_api_descriptor;
use miden_node_utils::ErrorReport;
use miden_node_utils::panic::catch_panic_layer_fn;
use miden_node_utils::tracing::grpc::grpc_trace_fn;
use miden_objects::block::{BlockSigner, ProposedBlock};
use miden_objects::transaction::{ProvenTransaction, TransactionInputs};
use miden_objects::transaction::{
ProvenTransaction,
TransactionHeader,
TransactionId,
TransactionInputs,
};
use miden_objects::utils::{Deserializable, Serializable};
use tokio::net::TcpListener;
use tokio::sync::RwLock;
use tokio_stream::wrappers::TcpListenerStream;
use tonic::Status;
use tower_http::catch_panic::CatchPanicLayer;
use tower_http::trace::TraceLayer;

use crate::COMPONENT;
use crate::block_validation::validate_block;
use crate::tx_validation::validate_transaction;

/// A type alias for a read-write lock that stores validated transactions.
pub type ValidatedTransactions = RwLock<HashMap<TransactionId, TransactionHeader>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we won't have the time to implement proper persistence for this before the next release. So, maybe this could be something like an LRU cache? We could set the size to be pretty big (e.g., thousands of transactions), but at least we'll be able to keep it bounded and we won't need to manually manage transaction eviction.

In the longer run, we probably want to store transactions for a long time for audibility purposes - but that's something to implement when we add persistence mechanism.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we won't have the time to implement proper persistence for this before the next release

If we release the in-memory version won't we risk causing a halt if we restart the node? Which is to say that we can't release this until persistence is in place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo just adding a validator.sqlite with a basic

CREATE TABLE transactions (
    id BLOB PRIMARY KEY,
    header BLOB NOT NULL,
    body BLOB NOT NULL,
    timestamp INTEGER NOT NULL,
);

is probably quicker to do than the time we spend thinking and discussing this :)

The "hardest" part will be the cli configuration and briefly thinking about how/if concurrent writes are a concern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was also the issue of code factoring because the validator db wouldn't go through the store. I had a PR a while ago that implemented a basic approach for it. I assume we are still of the mind that it doesn't make sense for the validator db to be tied to the store, but I haven't thought about it for a while now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be its own separate database


// VALIDATOR
// ================================================================================

Expand Down Expand Up @@ -70,7 +81,7 @@ impl<S: BlockSigner + Send + Sync + 'static> Validator<S> {
.layer(CatchPanicLayer::custom(catch_panic_layer_fn))
.layer(TraceLayer::new_for_grpc().make_span_with(grpc_trace_fn))
.timeout(self.grpc_timeout)
.add_service(api_server::ApiServer::new(ValidatorServer { signer: self.signer }))
.add_service(api_server::ApiServer::new(ValidatorServer::new(self.signer)))
.add_service(reflection_service)
.add_service(reflection_service_alpha)
.serve_with_incoming(TcpListenerStream::new(listener))
Expand All @@ -87,6 +98,14 @@ impl<S: BlockSigner + Send + Sync + 'static> Validator<S> {
/// Implements the gRPC API for the validator.
struct ValidatorServer<S> {
signer: S,
validated_transactions: Arc<ValidatedTransactions>,
}

impl<S> ValidatorServer<S> {
fn new(signer: S) -> Self {
let validated_transactions = Arc::new(ValidatedTransactions::default());
Self { signer, validated_transactions }
}
}

#[tonic::async_trait]
Expand Down Expand Up @@ -123,9 +142,14 @@ impl<S: BlockSigner + Send + Sync + 'static> api_server::Api for ValidatorServer
})?;

// Validate the transaction.
validate_transaction(proven_tx, tx_inputs).await.map_err(|err| {
Status::invalid_argument(err.as_report_context("Invalid transaction"))
})?;
let validated_tx_header =
validate_transaction(proven_tx.clone(), tx_inputs).await.map_err(|err| {
Status::invalid_argument(err.as_report_context("Invalid transaction"))
})?;

// Register the validated transaction.
let tx_id = validated_tx_header.id();
self.validated_transactions.write().await.insert(tx_id, validated_tx_header);

Ok(tonic::Response::new(()))
}
Expand All @@ -145,10 +169,13 @@ impl<S: BlockSigner + Send + Sync + 'static> api_server::Api for ValidatorServer
))
})?;

// Build and sign header.
let (header, _body) = build_block(proposed_block)
.map_err(|err| tonic::Status::internal(format!("Failed to build block: {err}")))?;
let signature = self.signer.sign(&header);
// Validate the block.
let signature =
validate_block(proposed_block, &self.signer, self.validated_transactions.clone())
.await
.map_err(|err| {
tonic::Status::invalid_argument(format!("Failed to validate block: {err}",))
})?;

// Send the signature.
let response = proto::blockchain::BlockSignature { signature: signature.to_bytes() };
Expand Down
6 changes: 4 additions & 2 deletions crates/validator/src/tx_validation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ pub enum TransactionValidationError {

/// Validates a transaction by verifying its proof, executing it and comparing its header with the
/// provided proven transaction.
///
/// Returns the header of the executed transaction if successful.
pub async fn validate_transaction(
proven_tx: ProvenTransaction,
tx_inputs: TransactionInputs,
) -> Result<(), TransactionValidationError> {
) -> Result<TransactionHeader, TransactionValidationError> {
// First, verify the transaction proof
let tx_verifier = TransactionVerifier::new(MIN_PROOF_SECURITY_LEVEL);
tx_verifier.verify(&proven_tx)?;
Expand All @@ -50,7 +52,7 @@ pub async fn validate_transaction(
let executed_tx_header: TransactionHeader = (&executed_tx).into();
let proven_tx_header: TransactionHeader = (&proven_tx).into();
if executed_tx_header == proven_tx_header {
Ok(())
Ok(executed_tx_header)
} else {
Err(TransactionValidationError::Mismatch {
proven_tx_header: proven_tx_header.into(),
Expand Down