Skip to content

Commit

Permalink
fix: wallet coinbases not validated correctly (#6074)
Browse files Browse the repository at this point in the history
Description
---
Fixes adding inputs scanned from the block to be improperly marked on
adding to the database.

Motivation and Context
---
Inputs scanned, such as coinbases are marked as `unspent` while this is
technically true from the wallet's perspective as they are unspent and
are detected from a mined block. We need to mark them as
`unspentUnconfiremd`. We need to make sure they are properly confirmed
by the validation task. The validation task will pick up these outputs
and flag them as spent/reorged etc.

But in the current case because they are flagged as `unspent` the wallet
can select them when spending, although they might not have passed the
confirmation time, or the might have been reorged out of the main chain.
  • Loading branch information
SWvheerden authored Jan 18, 2024
1 parent 057a68c commit bb66df1
Show file tree
Hide file tree
Showing 13 changed files with 520 additions and 259 deletions.
6 changes: 6 additions & 0 deletions applications/minotari_app_grpc/proto/wallet.proto
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ service Wallet {
rpc CancelTransaction (CancelTransactionRequest) returns (CancelTransactionResponse);
// Will trigger a complete revalidation of all wallet outputs.
rpc RevalidateAllTransactions (RevalidateRequest) returns (RevalidateResponse);
// Will trigger a validation of all wallet outputs.
rpc ValidateAllTransactions (ValidateRequest) returns (ValidateResponse);
// This will send a XTR SHA Atomic swap transaction
rpc SendShaAtomicSwapTransaction(SendShaAtomicSwapRequest) returns (SendShaAtomicSwapResponse);
// This will create a burn transaction
Expand Down Expand Up @@ -289,6 +291,10 @@ message RevalidateRequest{}

message RevalidateResponse{}

message ValidateRequest{}

message ValidateResponse{}

message SetBaseNodeRequest {
string public_key_hex = 1;
string net_address = 2;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ use minotari_app_grpc::tari_rpc::{
TransferRequest,
TransferResponse,
TransferResult,
ValidateRequest,
ValidateResponse,
};
use minotari_wallet::{
connectivity_service::{OnlineStatus, WalletConnectivityInterface},
Expand Down Expand Up @@ -307,6 +309,23 @@ impl wallet_server::Wallet for WalletGrpcServer {
Ok(Response::new(RevalidateResponse {}))
}

async fn validate_all_transactions(
&self,
_request: Request<ValidateRequest>,
) -> Result<Response<ValidateResponse>, Status> {
let mut output_service = self.get_output_manager_service();
output_service
.validate_txos()
.await
.map_err(|e| Status::unknown(e.to_string()))?;
let mut tx_service = self.get_transaction_service();
tx_service
.validate_transactions()
.await
.map_err(|e| Status::unknown(e.to_string()))?;
Ok(Response::new(ValidateResponse {}))
}

async fn send_sha_atomic_swap_transaction(
&self,
request: Request<SendShaAtomicSwapRequest>,
Expand Down
30 changes: 24 additions & 6 deletions base_layer/core/src/transactions/test_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ use std::sync::Arc;

use rand::rngs::OsRng;
use tari_common::configuration::Network;
use tari_common_sqlite::{error::SqliteStorageError, sqlite_connection_pool::PooledDbConnection};
use tari_common_types::types::{Commitment, PrivateKey, PublicKey, Signature};
use tari_crypto::keys::{PublicKey as PK, SecretKey};
use tari_key_manager::key_manager_service::KeyManagerInterface;
use tari_key_manager::key_manager_service::{storage::sqlite_db::KeyManagerSqliteDatabase, KeyManagerInterface};
use tari_script::{inputs, script, ExecutionStack, TariScript};

use super::transaction_components::{TransactionInputVersion, TransactionOutputVersion};
Expand All @@ -43,6 +44,7 @@ use crate::{
TariKeyId,
TransactionKeyManagerBranch,
TransactionKeyManagerInterface,
TransactionKeyManagerWrapper,
TxoStage,
},
tari_amount::MicroMinotari,
Expand All @@ -64,7 +66,13 @@ use crate::{
},
};

pub async fn create_test_input(amount: MicroMinotari, maturity: u64, key_manager: &MemoryDbKeyManager) -> WalletOutput {
pub async fn create_test_input<
TKeyManagerDbConnection: PooledDbConnection<Error = SqliteStorageError> + Clone + 'static,
>(
amount: MicroMinotari,
maturity: u64,
key_manager: &TransactionKeyManagerWrapper<KeyManagerSqliteDatabase<TKeyManagerDbConnection>>,
) -> WalletOutput {
let params = TestParams::new(key_manager).await;
params
.create_input(
Expand Down Expand Up @@ -99,7 +107,9 @@ pub struct TestParams {
}

impl TestParams {
pub async fn new(key_manager: &MemoryDbKeyManager) -> TestParams {
pub async fn new<TKeyManagerDbConnection: PooledDbConnection<Error = SqliteStorageError> + Clone + 'static>(
key_manager: &TransactionKeyManagerWrapper<KeyManagerSqliteDatabase<TKeyManagerDbConnection>>,
) -> TestParams {
let (spend_key_id, spend_key_pk, script_key_id, script_key_pk) =
key_manager.get_next_spend_and_script_key_ids().await.unwrap();
let (sender_offset_key_id, sender_offset_key_pk) = key_manager
Expand Down Expand Up @@ -140,10 +150,12 @@ impl TestParams {
Fee::new(self.transaction_weight)
}

pub async fn create_output(
pub async fn create_output<
TKeyManagerDbConnection: PooledDbConnection<Error = SqliteStorageError> + Clone + 'static,
>(
&self,
params: UtxoTestParams,
key_manager: &MemoryDbKeyManager,
key_manager: &TransactionKeyManagerWrapper<KeyManagerSqliteDatabase<TKeyManagerDbConnection>>,
) -> Result<WalletOutput, String> {
let version = match params.output_version {
Some(v) => v,
Expand Down Expand Up @@ -175,7 +187,13 @@ impl TestParams {

/// Create a random transaction input for the given amount and maturity period. The input's wallet
/// parameters are returned.
pub async fn create_input(&self, params: UtxoTestParams, key_manager: &MemoryDbKeyManager) -> WalletOutput {
pub async fn create_input<
TKeyManagerDbConnection: PooledDbConnection<Error = SqliteStorageError> + Clone + 'static,
>(
&self,
params: UtxoTestParams,
key_manager: &TransactionKeyManagerWrapper<KeyManagerSqliteDatabase<TKeyManagerDbConnection>>,
) -> WalletOutput {
self.create_output(params, key_manager).await.unwrap()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,21 +85,21 @@ impl OutputManagerSqliteDatabase {
if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() {
return Err(OutputManagerStorageError::DuplicateOutput);
}
let new_output = NewOutputSql::new(*o, OutputStatus::Unspent, None)?;
let new_output = NewOutputSql::new(*o, Some(OutputStatus::UnspentMinedUnconfirmed), None)?;
new_output.commit(conn)?
},
DbKeyValuePair::UnspentOutputWithTxId(c, (tx_id, o)) => {
if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() {
return Err(OutputManagerStorageError::DuplicateOutput);
}
let new_output = NewOutputSql::new(*o, OutputStatus::Unspent, Some(tx_id))?;
let new_output = NewOutputSql::new(*o, Some(OutputStatus::UnspentMinedUnconfirmed), Some(tx_id))?;
new_output.commit(conn)?
},
DbKeyValuePair::OutputToBeReceived(c, (tx_id, o)) => {
if OutputSql::find_by_commitment_and_cancelled(&c.to_vec(), false, conn).is_ok() {
return Err(OutputManagerStorageError::DuplicateOutput);
}
let new_output = NewOutputSql::new(*o, OutputStatus::EncumberedToBeReceived, Some(tx_id))?;
let new_output = NewOutputSql::new(*o, Some(OutputStatus::EncumberedToBeReceived), Some(tx_id))?;
new_output.commit(conn)?
},

Expand Down Expand Up @@ -658,7 +658,11 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
})?;

for co in outputs_to_receive {
let new_output = NewOutputSql::new(co.clone(), OutputStatus::ShortTermEncumberedToBeReceived, Some(tx_id))?;
let new_output = NewOutputSql::new(
co.clone(),
Some(OutputStatus::ShortTermEncumberedToBeReceived),
Some(tx_id),
)?;
new_output.commit(&mut conn)?;
}
if start.elapsed().as_millis() > 0 {
Expand Down Expand Up @@ -977,7 +981,7 @@ impl OutputManagerBackend for OutputManagerSqliteDatabase {
if OutputSql::find_by_commitment_and_cancelled(&output.commitment.to_vec(), false, &mut conn).is_ok() {
return Err(OutputManagerStorageError::DuplicateOutput);
}
let new_output = NewOutputSql::new(output, OutputStatus::EncumberedToBeReceived, Some(tx_id))?;
let new_output = NewOutputSql::new(output, Some(OutputStatus::EncumberedToBeReceived), Some(tx_id))?;
new_output.commit(&mut conn)?;

if start.elapsed().as_millis() > 0 {
Expand Down Expand Up @@ -1322,7 +1326,7 @@ mod test {
let uo = DbWalletOutput::from_wallet_output(uo, &key_manager, None, OutputSource::Standard, None, None)
.await
.unwrap();
let o = NewOutputSql::new(uo, OutputStatus::Unspent, None).unwrap();
let o = NewOutputSql::new(uo, Some(OutputStatus::Unspent), None).unwrap();
outputs.push(o.clone());
outputs_unspent.push(o.clone());
o.commit(&mut conn).unwrap();
Expand All @@ -1333,7 +1337,7 @@ mod test {
let uo = DbWalletOutput::from_wallet_output(uo, &key_manager, None, OutputSource::Standard, None, None)
.await
.unwrap();
let o = NewOutputSql::new(uo, OutputStatus::Spent, None).unwrap();
let o = NewOutputSql::new(uo, Some(OutputStatus::Spent), None).unwrap();
outputs.push(o.clone());
outputs_spent.push(o.clone());
o.commit(&mut conn).unwrap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl NewOutputSql {
#[allow(clippy::cast_possible_wrap)]
pub fn new(
output: DbWalletOutput,
status: OutputStatus,
status: Option<OutputStatus>,
received_in_tx_id: Option<TxId>,
) -> Result<Self, OutputManagerStorageError> {
let mut covenant = Vec::new();
Expand All @@ -84,7 +84,7 @@ impl NewOutputSql {
value: output.wallet_output.value.as_u64() as i64,
output_type: i32::from(output.wallet_output.features.output_type.as_byte()),
maturity: output.wallet_output.features.maturity as i64,
status: status as i32,
status: status.unwrap_or(output.status) as i32,
received_in_tx_id: received_in_tx_id.map(|i| i.as_u64() as i64),
hash: output.hash.to_vec(),
script: output.wallet_output.script.to_bytes(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,6 @@ where
self.operation_id
);

// We have to send positions to the base node because if the base node cannot find the hash of the output
// we can't tell if the output ever existed, as opposed to existing and was spent.
// This assumes that the base node has not reorged since the last time we asked.
let response = wallet_client
.query_deleted(QueryDeletedRequest {
chain_must_include_header: last_mined_header_hash.map(|v| v.to_vec()).unwrap_or_default(),
Expand Down
Loading

0 comments on commit bb66df1

Please sign in to comment.