From 3ed3d96d4a6ee598f6188ce45a00284dbbc5be30 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Wed, 29 May 2024 04:08:36 +0300 Subject: [PATCH 1/7] Add input signature script checking to RPC --- rpc/core/src/error.rs | 11 +++++++---- rpc/service/src/service.rs | 7 +++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/rpc/core/src/error.rs b/rpc/core/src/error.rs index 59f6b910e7..5f7d1d5986 100644 --- a/rpc/core/src/error.rs +++ b/rpc/core/src/error.rs @@ -1,4 +1,4 @@ -use kaspa_consensus_core::{subnets::SubnetworkConversionError, tx::TransactionId}; +use kaspa_consensus_core::{subnets::SubnetworkConversionError, tx::TransactionId}; use kaspa_utils::networking::IpAddress; use std::{net::AddrParseError, num::TryFromIntError}; use thiserror::Error; @@ -47,6 +47,9 @@ pub enum RpcError { #[error("Rejected transaction {0}: {1}")] RejectedTransaction(RpcTransactionId, String), + #[error("Transaction {0} has an input with an empty signature_script at index {1}.")] + EmptySignatureScript(RpcTransactionId, usize), + #[error("Block {0} is invalid. No verbose data can be built.")] InvalidBlock(RpcHash), @@ -116,9 +119,9 @@ pub enum RpcError { #[error("transaction query must either not filter transactions or include orphans")] InconsistentMempoolTxQuery, - #[error(transparent)] - SubnetParsingError(#[from] SubnetworkConversionError), - + #[error(transparent)] + SubnetParsingError(#[from] SubnetworkConversionError), + #[error(transparent)] WasmError(#[from] workflow_wasm::error::Error), diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index 00cc0d0828..c1e08245a8 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -493,6 +493,13 @@ NOTE: This error usually indicates an RPC conversion error between the node and let transaction: Transaction = (&request.transaction).try_into()?; let transaction_id = transaction.id(); + let inputs = &transaction.inputs; + + if let Some((index, _)) = inputs.iter().enumerate().find(|(_, input)| input.signature_script.is_empty()) { + let err = RpcError::EmptySignatureScript(transaction_id, index); + return Err(err) + } + let session = self.consensus_manager.consensus().unguarded_session(); let orphan = match allow_orphan { true => Orphan::Allowed, From d0cc992182fb5dea1d224bbf0d43030ebbe99580 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Wed, 29 May 2024 16:41:15 +0300 Subject: [PATCH 2/7] Return multiple indices w the EmptySignatureScript error --- rpc/core/src/error.rs | 4 ++-- rpc/service/src/service.rs | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/rpc/core/src/error.rs b/rpc/core/src/error.rs index 5f7d1d5986..ada3879558 100644 --- a/rpc/core/src/error.rs +++ b/rpc/core/src/error.rs @@ -47,8 +47,8 @@ pub enum RpcError { #[error("Rejected transaction {0}: {1}")] RejectedTransaction(RpcTransactionId, String), - #[error("Transaction {0} has an input with an empty signature_script at index {1}.")] - EmptySignatureScript(RpcTransactionId, usize), + #[error("Transaction {0} has an input with an empty signature_script at indices {1}.")] + EmptySignatureScript(RpcTransactionId, String), #[error("Block {0} is invalid. No verbose data can be built.")] InvalidBlock(RpcHash), diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index c1e08245a8..6d48c205dd 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -495,9 +495,11 @@ NOTE: This error usually indicates an RPC conversion error between the node and let transaction_id = transaction.id(); let inputs = &transaction.inputs; - if let Some((index, _)) = inputs.iter().enumerate().find(|(_, input)| input.signature_script.is_empty()) { - let err = RpcError::EmptySignatureScript(transaction_id, index); - return Err(err) + let empty_indices: Vec = + inputs.iter().enumerate().filter_map(|(index, input)| (input.signature_script.is_empty()).then(|| index)).collect(); + if !empty_indices.is_empty() { + let indices_str = empty_indices.iter().map(|index| index.to_string()).collect::>().join(", "); + return Err(RpcError::EmptySignatureScript(transaction_id, indices_str)); } let session = self.consensus_manager.consensus().unguarded_session(); From 9e5685668fe129e7dc05bdce3076399fed3c9e9f Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Wed, 29 May 2024 17:12:57 +0300 Subject: [PATCH 3/7] Apply suggestions --- rpc/core/src/error.rs | 2 +- rpc/service/src/service.rs | 34 ++++++++++++++++++---------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/rpc/core/src/error.rs b/rpc/core/src/error.rs index ada3879558..5749fa03fc 100644 --- a/rpc/core/src/error.rs +++ b/rpc/core/src/error.rs @@ -47,7 +47,7 @@ pub enum RpcError { #[error("Rejected transaction {0}: {1}")] RejectedTransaction(RpcTransactionId, String), - #[error("Transaction {0} has an input with an empty signature_script at indices {1}.")] + #[error("Transaction {0} has input with empty signature scripts at indices {1}.")] EmptySignatureScript(RpcTransactionId, String), #[error("Block {0} is invalid. No verbose data can be built.")] diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index 6d48c205dd..7ad4b1657d 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -495,24 +495,26 @@ NOTE: This error usually indicates an RPC conversion error between the node and let transaction_id = transaction.id(); let inputs = &transaction.inputs; - let empty_indices: Vec = - inputs.iter().enumerate().filter_map(|(index, input)| (input.signature_script.is_empty()).then(|| index)).collect(); + let empty_indices = inputs + .iter() + .enumerate() + .filter_map(|(index, input)| (input.signature_script.is_empty()).then_some(index.to_string())) + .collect::>(); if !empty_indices.is_empty() { - let indices_str = empty_indices.iter().map(|index| index.to_string()).collect::>().join(", "); - return Err(RpcError::EmptySignatureScript(transaction_id, indices_str)); + Err(RpcError::EmptySignatureScript(transaction_id, empty_indices.join(", "))) + } else { + let session = self.consensus_manager.consensus().unguarded_session(); + let orphan = match allow_orphan { + true => Orphan::Allowed, + false => Orphan::Forbidden, + }; + self.flow_context.submit_rpc_transaction(&session, transaction, orphan).await.map_err(|err| { + let err = RpcError::RejectedTransaction(transaction_id, err.to_string()); + debug!("{err}"); + err + })?; + Ok(SubmitTransactionResponse::new(transaction_id)) } - - let session = self.consensus_manager.consensus().unguarded_session(); - let orphan = match allow_orphan { - true => Orphan::Allowed, - false => Orphan::Forbidden, - }; - self.flow_context.submit_rpc_transaction(&session, transaction, orphan).await.map_err(|err| { - let err = RpcError::RejectedTransaction(transaction_id, err.to_string()); - debug!("{err}"); - err - })?; - Ok(SubmitTransactionResponse::new(transaction_id)) } async fn get_current_network_call(&self, _: GetCurrentNetworkRequest) -> RpcResult { From 4f4167f01d06fe16eb1b812a3dc4e02f4ff17417 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Thu, 30 May 2024 17:18:50 +0300 Subject: [PATCH 4/7] Refctorize some part of code to apply suggestions --- Cargo.lock | 1 + rpc/service/Cargo.toml | 1 + rpc/service/src/service.rs | 26 ++++++++++++++------------ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 22cd64f4f9..4911ca8901 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3253,6 +3253,7 @@ name = "kaspa-rpc-service" version = "0.14.1" dependencies = [ "async-trait", + "itertools 0.11.0", "kaspa-addresses", "kaspa-consensus-core", "kaspa-consensus-notify", diff --git a/rpc/service/Cargo.toml b/rpc/service/Cargo.toml index d606d51533..592178b66a 100644 --- a/rpc/service/Cargo.toml +++ b/rpc/service/Cargo.toml @@ -33,4 +33,5 @@ async-trait.workspace = true log.workspace = true tokio.workspace = true triggered.workspace = true +itertools.workspace = true workflow-rpc.workspace = true diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index 7ad4b1657d..a9d428c468 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -4,6 +4,7 @@ use super::collector::{CollectorFromConsensus, CollectorFromIndex}; use crate::converter::{consensus::ConsensusConverter, index::IndexConverter, protocol::ProtocolConverter}; use crate::service::NetworkType::{Mainnet, Testnet}; use async_trait::async_trait; +use itertools::Itertools; use kaspa_consensus_core::api::counters::ProcessingCounters; use kaspa_consensus_core::errors::block::RuleError; use kaspa_consensus_core::{ @@ -64,6 +65,7 @@ use kaspa_txscript::{extract_script_pub_key_address, pay_to_address_script}; use kaspa_utils::{channel::Channel, triggers::SingleTrigger}; use kaspa_utils_tower::counters::TowerConnectionCounters; use kaspa_utxoindex::api::UtxoIndexProxy; +use std::iter; use std::{ collections::HashMap, iter::once, @@ -495,24 +497,24 @@ NOTE: This error usually indicates an RPC conversion error between the node and let transaction_id = transaction.id(); let inputs = &transaction.inputs; - let empty_indices = inputs - .iter() - .enumerate() - .filter_map(|(index, input)| (input.signature_script.is_empty()).then_some(index.to_string())) - .collect::>(); - if !empty_indices.is_empty() { - Err(RpcError::EmptySignatureScript(transaction_id, empty_indices.join(", "))) + let mut empty_indices = + inputs.iter().enumerate().filter_map(|(index, input)| (input.signature_script.is_empty()).then_some(index.to_string())); + let first_empty = empty_indices.next(); + if let Some(first_empty) = first_empty { + Err(RpcError::EmptySignatureScript(transaction_id, iter::once(first_empty).chain(empty_indices).join(", "))) } else { let session = self.consensus_manager.consensus().unguarded_session(); let orphan = match allow_orphan { true => Orphan::Allowed, false => Orphan::Forbidden, }; - self.flow_context.submit_rpc_transaction(&session, transaction, orphan).await.map_err(|err| { - let err = RpcError::RejectedTransaction(transaction_id, err.to_string()); - debug!("{err}"); - err - })?; + self.flow_context + .submit_rpc_transaction(&session, transaction, orphan) + .await + .as_ref() + .map_err(ToString::to_string) + .map_err(|err| RpcError::RejectedTransaction(transaction_id, err)) + .inspect_err(|err| debug!("{err}"))?; Ok(SubmitTransactionResponse::new(transaction_id)) } } From 5a99345703ce8521067e8f36fccbad9bac4180d9 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Thu, 25 Jul 2024 22:49:24 +0300 Subject: [PATCH 5/7] Revert in-RPC changes and move error to in-consensus --- Cargo.lock | 1 - consensus/Cargo.toml | 1 + consensus/core/src/errors/tx.rs | 3 ++ .../transaction_validator_populated.rs | 14 +++++-- rpc/core/src/error.rs | 3 -- rpc/service/Cargo.toml | 1 - rpc/service/src/service.rs | 37 ++++++------------- 7 files changed, 27 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b31ec7aa7d..0d4703b8ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3284,7 +3284,6 @@ name = "kaspa-rpc-service" version = "0.14.1" dependencies = [ "async-trait", - "itertools 0.11.0", "kaspa-addresses", "kaspa-consensus-core", "kaspa-consensus-notify", diff --git a/consensus/Cargo.toml b/consensus/Cargo.toml index f151e404b4..3f4a1b456d 100644 --- a/consensus/Cargo.toml +++ b/consensus/Cargo.toml @@ -30,6 +30,7 @@ kaspa-muhash.workspace = true kaspa-notify.workspace = true kaspa-pow.workspace = true kaspa-txscript.workspace = true +kaspa-txscript-errors.workspace = true kaspa-utils.workspace = true log.workspace = true once_cell.workspace = true diff --git a/consensus/core/src/errors/tx.rs b/consensus/core/src/errors/tx.rs index ec1899faad..1aeb5220fe 100644 --- a/consensus/core/src/errors/tx.rs +++ b/consensus/core/src/errors/tx.rs @@ -80,6 +80,9 @@ pub enum TxRuleError { #[error("failed to verify the signature script: {0}")] SignatureInvalid(TxScriptError), + #[error("failed to verify empty signature script. Inner error: {0}")] + SignatureEmpty(TxScriptError), + #[error("input {0} sig op count is {1}, but the calculated value is {2}")] WrongSigOpCount(usize, u64, u64), diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 1835ba61e4..904652c926 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -1,7 +1,8 @@ use crate::constants::{MAX_SOMPI, SEQUENCE_LOCK_TIME_DISABLED, SEQUENCE_LOCK_TIME_MASK}; -use kaspa_consensus_core::{hashing::sighash::SigHashReusedValues, tx::VerifiableTransaction}; +use kaspa_consensus_core::{hashing::sighash::SigHashReusedValues, tx::{TransactionInput, VerifiableTransaction}}; use kaspa_core::warn; use kaspa_txscript::{get_sig_op_count, TxScriptEngine}; +use kaspa_txscript_errors::TxScriptError; use super::{ errors::{TxResult, TxRuleError}, @@ -167,14 +168,21 @@ impl TransactionValidator { let mut reused_values = SigHashReusedValues::new(); for (i, (input, entry)) in tx.populated_inputs().enumerate() { let mut engine = TxScriptEngine::from_transaction_input(tx, input, i, entry, &mut reused_values, &self.sig_cache) - .map_err(TxRuleError::SignatureInvalid)?; - engine.execute().map_err(TxRuleError::SignatureInvalid)?; + .map_err(|err| map_script_err(err, input))?; + engine.execute().map_err(|err| map_script_err(err, input))?; } Ok(()) } } +fn map_script_err(script_err: TxScriptError, input: &TransactionInput) -> TxRuleError { + match input.signature_script.is_empty() { + true => TxRuleError::SignatureEmpty(script_err), + false => TxRuleError::SignatureInvalid(script_err), + } +} + #[cfg(test)] mod tests { use super::super::errors::TxRuleError; diff --git a/rpc/core/src/error.rs b/rpc/core/src/error.rs index 5749fa03fc..6e1083ab75 100644 --- a/rpc/core/src/error.rs +++ b/rpc/core/src/error.rs @@ -47,9 +47,6 @@ pub enum RpcError { #[error("Rejected transaction {0}: {1}")] RejectedTransaction(RpcTransactionId, String), - #[error("Transaction {0} has input with empty signature scripts at indices {1}.")] - EmptySignatureScript(RpcTransactionId, String), - #[error("Block {0} is invalid. No verbose data can be built.")] InvalidBlock(RpcHash), diff --git a/rpc/service/Cargo.toml b/rpc/service/Cargo.toml index 592178b66a..d606d51533 100644 --- a/rpc/service/Cargo.toml +++ b/rpc/service/Cargo.toml @@ -33,5 +33,4 @@ async-trait.workspace = true log.workspace = true tokio.workspace = true triggered.workspace = true -itertools.workspace = true workflow-rpc.workspace = true diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index 356538548a..a4774c5718 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -4,7 +4,6 @@ use super::collector::{CollectorFromConsensus, CollectorFromIndex}; use crate::converter::{consensus::ConsensusConverter, index::IndexConverter, protocol::ProtocolConverter}; use crate::service::NetworkType::{Mainnet, Testnet}; use async_trait::async_trait; -use itertools::Itertools; use kaspa_consensus_core::api::counters::ProcessingCounters; use kaspa_consensus_core::errors::block::RuleError; use kaspa_consensus_core::{ @@ -65,7 +64,6 @@ use kaspa_txscript::{extract_script_pub_key_address, pay_to_address_script}; use kaspa_utils::{channel::Channel, triggers::SingleTrigger}; use kaspa_utils_tower::counters::TowerConnectionCounters; use kaspa_utxoindex::api::UtxoIndexProxy; -use std::iter; use std::{ collections::HashMap, iter::once, @@ -495,28 +493,17 @@ NOTE: This error usually indicates an RPC conversion error between the node and let transaction: Transaction = (&request.transaction).try_into()?; let transaction_id = transaction.id(); - let inputs = &transaction.inputs; - - let mut empty_indices = - inputs.iter().enumerate().filter_map(|(index, input)| (input.signature_script.is_empty()).then_some(index.to_string())); - let first_empty = empty_indices.next(); - if let Some(first_empty) = first_empty { - Err(RpcError::EmptySignatureScript(transaction_id, iter::once(first_empty).chain(empty_indices).join(", "))) - } else { - let session = self.consensus_manager.consensus().unguarded_session(); - let orphan = match allow_orphan { - true => Orphan::Allowed, - false => Orphan::Forbidden, - }; - self.flow_context - .submit_rpc_transaction(&session, transaction, orphan) - .await - .as_ref() - .map_err(ToString::to_string) - .map_err(|err| RpcError::RejectedTransaction(transaction_id, err)) - .inspect_err(|err| debug!("{err}"))?; - Ok(SubmitTransactionResponse::new(transaction_id)) - } + let session = self.consensus_manager.consensus().unguarded_session(); + let orphan = match allow_orphan { + true => Orphan::Allowed, + false => Orphan::Forbidden, + }; + self.flow_context.submit_rpc_transaction(&session, transaction, orphan).await.map_err(|err| { + let err = RpcError::RejectedTransaction(transaction_id, err.to_string()); + debug!("{err}"); + err + })?; + Ok(SubmitTransactionResponse::new(transaction_id)) } async fn submit_transaction_replacement_call( @@ -1005,4 +992,4 @@ impl AsyncService for RpcCoreService { Ok(()) }) } -} +} \ No newline at end of file From 85dfab0c232b1dd3f66c7b6a808f21dc96b11be7 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Thu, 25 Jul 2024 22:51:12 +0300 Subject: [PATCH 6/7] Apply Maxims suggestions (match -> if) --- .../transaction_validator_populated.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 904652c926..972e93e439 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -177,9 +177,10 @@ impl TransactionValidator { } fn map_script_err(script_err: TxScriptError, input: &TransactionInput) -> TxRuleError { - match input.signature_script.is_empty() { - true => TxRuleError::SignatureEmpty(script_err), - false => TxRuleError::SignatureInvalid(script_err), + if input.signature_script.is_empty() { + TxRuleError::SignatureEmpty(script_err) + } else { + TxRuleError::SignatureInvalid(script_err) } } From 5882ea91ee83902e123462c0d928e5ac67db5ede Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Thu, 25 Jul 2024 23:35:33 +0300 Subject: [PATCH 7/7] Simple forgotten lints --- .../transaction_validator/transaction_validator_populated.rs | 5 ++++- rpc/service/src/service.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 972e93e439..f7a43aad2d 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -1,5 +1,8 @@ use crate::constants::{MAX_SOMPI, SEQUENCE_LOCK_TIME_DISABLED, SEQUENCE_LOCK_TIME_MASK}; -use kaspa_consensus_core::{hashing::sighash::SigHashReusedValues, tx::{TransactionInput, VerifiableTransaction}}; +use kaspa_consensus_core::{ + hashing::sighash::SigHashReusedValues, + tx::{TransactionInput, VerifiableTransaction}, +}; use kaspa_core::warn; use kaspa_txscript::{get_sig_op_count, TxScriptEngine}; use kaspa_txscript_errors::TxScriptError; diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index a4774c5718..c69fda4f14 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -992,4 +992,4 @@ impl AsyncService for RpcCoreService { Ok(()) }) } -} \ No newline at end of file +}