Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove activated feature that checks tx signature len #21747

Merged
merged 1 commit into from
Dec 14, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 0 additions & 4 deletions rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2037,10 +2037,6 @@ fn verify_transaction(
return Err(RpcCustomError::TransactionPrecompileVerificationFailure(e).into());
}

if !transaction.verify_signatures_len() {
return Err(RpcCustomError::TransactionSignatureVerificationFailure.into());
}

Ok(())
}

Expand Down
19 changes: 6 additions & 13 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5281,10 +5281,6 @@ impl Bank {
})
}?;

if self.verify_tx_signatures_len_enabled() && !sanitized_tx.verify_signatures_len() {
return Err(TransactionError::SanitizeFailure);
}

if verification_mode == TransactionVerificationMode::HashAndVerifyPrecompiles
|| verification_mode == TransactionVerificationMode::FullVerification
{
Expand Down Expand Up @@ -5770,11 +5766,6 @@ impl Bank {
.is_active(&feature_set::no_overflow_rent_distribution::id())
}

pub fn verify_tx_signatures_len_enabled(&self) -> bool {
self.feature_set
.is_active(&feature_set::verify_tx_signatures_len::id())
}

pub fn versioned_tx_message_enabled(&self) -> bool {
self.feature_set
.is_active(&feature_set::versioned_tx_message_enabled::id())
Expand Down Expand Up @@ -15012,12 +15003,14 @@ pub(crate) mod tests {
Some(TransactionError::SanitizeFailure),
);
}
// Too many signatures: Success without feature switch
// Too many signatures: Sanitization failure
{
let tx = make_transaction(TestCase::AddSignature);
assert!(bank
.verify_transaction(tx.into(), TransactionVerificationMode::FullVerification)
.is_ok());
assert_eq!(
bank.verify_transaction(tx.into(), TransactionVerificationMode::FullVerification)
.err(),
Some(TransactionError::SanitizeFailure),
);
}
}

Expand Down
5 changes: 0 additions & 5 deletions sdk/src/transaction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,11 +299,6 @@ impl Transaction {
Signature::default()
}

/// Verify the length of signatures matches the value in the message header
pub fn verify_signatures_len(&self) -> bool {
self.signatures.len() == self.message.header.num_required_signatures as usize
}

/// Verify the transaction and hash its message
pub fn verify_and_hash_message(&self) -> Result<Hash> {
let message_bytes = self.message_data();
Expand Down
5 changes: 0 additions & 5 deletions sdk/src/transaction/sanitized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,6 @@ impl SanitizedTransaction {
}
}

/// Verify the length of signatures matches the value in the message header
pub fn verify_signatures_len(&self) -> bool {
self.signatures.len() == self.message.header().num_required_signatures as usize
}

/// Verify the transaction signatures
pub fn verify(&self) -> Result<()> {
let message_bytes = self.message_data();
Expand Down
12 changes: 7 additions & 5 deletions sdk/src/transaction/versioned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use {
transaction::{Result, Transaction, TransactionError},
},
serde::Serialize,
std::cmp::Ordering,
};

// NOTE: Serialization-related changes must be paired with the direct read at sigverify.
Expand All @@ -29,11 +30,12 @@ impl Sanitize for VersionedTransaction {
fn sanitize(&self) -> std::result::Result<(), SanitizeError> {
self.message.sanitize()?;

// Once the "verify_tx_signatures_len" feature is enabled, this may be
// updated to an equality check.
if usize::from(self.message.header().num_required_signatures) > self.signatures.len() {
return Err(SanitizeError::IndexOutOfBounds);
}
let num_required_signatures = usize::from(self.message.header().num_required_signatures);
match num_required_signatures.cmp(&self.signatures.len()) {
Ordering::Greater => Err(SanitizeError::IndexOutOfBounds),
Ordering::Less => Err(SanitizeError::InvalidValue),
Ordering::Equal => Ok(()),
}?;

// Signatures are verified before message keys are mapped so all signers
// must correspond to unmapped keys.
Expand Down