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.

Sign up for GitHub

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

Jump to bottom

fix: further block deserialization fixes #46

Merged
merged 5 commits into from
Oct 31, 2024
Merged

fix: further block deserialization fixes #46

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c3624de
fix: qcommitment seqwit always false
ogabrielides Oct 31, 2024
ef77bee
fix: qcommitment quorumIndex field
ogabrielides Oct 31, 2024
d4e5bea
fix: qcommitment signers validmembers bitsets
ogabrielides Oct 31, 2024
ca7ad39
correct size
ogabrielides Oct 31, 2024
0db8960
do not unwrap
ogabrielides Oct 31, 2024
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
3 changes: 3 additions & 0 deletions dash/src/blockdata/transaction/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,9 @@ impl Decodable for Transaction {
if special_transaction_type == TransactionType::AssetUnlock {
segwit = false;
}
if special_transaction_type == TransactionType::QuorumCommitment {
segwit = false;
}
Comment on lines +656 to +658
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can now generate the final response. The code shows that the consensus_encode implementation in Transaction properly handles witness serialization and has special cases for transaction types. The have_witness flag is determined by input witnesses and is explicitly set to false for certain transaction types, including AssetUnlock. However, I don't see the QuorumCommitment case being handled in the serialization code, which is inconsistent with the deserialization behavior.

Serialization handling for QuorumCommitment transactions needs to be updated

The deserialization code sets segwit = false for QuorumCommitment transactions, but the corresponding serialization code in consensus_encode doesn't handle this case. This could lead to serialization/deserialization inconsistency. Add the following case in consensus_encode:

if self.tx_type() == TransactionType::QuorumCommitment {
    have_witness = false;
}
🔗 Analysis chain

Verify serialization consistency for QuorumCommitment transactions.

While the deserialization change looks correct, we should ensure that the serialization logic in consensus_encode handles QuorumCommitment transactions consistently. Please verify that:

  1. The have_witness flag in consensus_encode matches this deserialization behavior
  2. The transaction roundtrip (serialize -> deserialize) works correctly for QuorumCommitment transactions

Let's verify the serialization handling:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for witness handling in consensus_encode
ast-grep --pattern 'consensus_encode<W: io::Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
  $$$
  let mut have_witness = $$$
  $$$
}'

Length of output: 153

Script:

#!/bin/bash
# Let's try a broader search for consensus_encode implementation
rg -U "consensus_encode.*?\{.*?have_witness" -g "*.rs" --multiline

Length of output: 68

Script:

#!/bin/bash
# Also search for Transaction serialization tests
rg -l "test.*serialization.*Transaction" -g "*.rs"

Length of output: 52

Script:

#!/bin/bash
# Look for QuorumCommitment handling in transaction code
rg "QuorumCommitment" -A 5 -B 5 -g "*.rs"

Length of output: 22043

Script:

#!/bin/bash
# Let's search for Transaction consensus_encode implementation
rg -U "impl.*?Transaction.*?consensus_encode.*?\{.*?have_witness" -g "*.rs" --multiline -A 20

Length of output: 95

Script:

#!/bin/bash
# Also search for Transaction serialization tests specifically for QuorumCommitment
rg "test.*serialization.*QuorumCommitment" -g "*.rs" -A 10

Length of output: 60

Script:

#!/bin/bash
# Try a more general search for Transaction consensus_encode
rg -U "impl.*?Transaction.*?consensus_encode" -g "*.rs" --multiline

Length of output: 69

Script:

#!/bin/bash
# Look for Transaction serialization tests with broader context
rg "test.*Transaction.*serialization" -g "*.rs" -B 5 -A 15

Length of output: 60

Script:

#!/bin/bash
# Search for witness handling in Transaction implementation
rg "have_witness" -g "*.rs" -B 5 -A 5

Length of output: 1822

if segwit {
let segwit_flag = u8::consensus_decode_from_finite_reader(r)?;
match segwit_flag {
Expand Down
69 changes: 65 additions & 4 deletions dash/src/blockdata/transaction/special_transaction/quorum_commitment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
//! It is defined in DIP6 [dip-0006.md](https://github.com/dashpay/dips/blob/master/dip-0006.md).
//!

use std::io::{Read, Write};
use crate::bls_sig_utils::{BLSPublicKey, BLSSignature};
use crate::consensus::{Decodable, Encodable, encode};
use crate::hash_types::{QuorumHash, QuorumVVecHash};
Expand All @@ -33,6 +34,7 @@ pub struct QuorumFinalizationCommitment {
pub version: u16,
pub llmq_type: u8,
pub quorum_hash: QuorumHash,
pub quorum_index: Option<i16>,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential None value for quorum_index more safely

In the QuorumFinalizationCommitment struct, quorum_index is of type Option<i16>. In the encoding process, you're using .unwrap() when encoding quorum_index, which can cause a panic if quorum_index is None. It's safer to handle the None case explicitly or use .expect() with a clear error message.

Consider modifying the consensus_encode method to handle the None case:

if self.version == 2 || self.version == 4 {
-    len += self.quorum_index.unwrap().consensus_encode(w)?;
+    if let Some(index) = self.quorum_index {
+        len += index.consensus_encode(w)?;
+    } else {
+        return Err(io::Error::new(io::ErrorKind::InvalidData, "quorum_index is None for version 2 or 4"));
+    }
}

Committable suggestion was skipped due to low confidence.

pub signers: Vec<u8>,
pub valid_members: Vec<u8>,
pub quorum_public_key: BLSPublicKey,
Expand All @@ -47,6 +49,9 @@ impl QuorumFinalizationCommitment {
let mut size = 2 + 1 + 32 + 48 + 32 + 96 + 96;
size += VarInt(self.signers.len() as u64).len() + self.signers.len();
size += VarInt(self.valid_members.len() as u64).len() + self.valid_members.len();
if self.version == 2 || self.version == 4 {
size += 16;
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the size calculation for quorum_index

In the size method, when version is 2 or 4, you're adding 16 to the size. However, i16 is 2 bytes, not 16 bytes. This could lead to incorrect size calculations.

Modify the size addition to reflect the actual size of i16:

if self.version == 2 || self.version == 4 {
-    size += 16;
+    size += 2; // i16 is 2 bytes
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.version == 2 || self.version == 4 {
size += 16;
}
if self.version == 2 || self.version == 4 {
size += 2; // i16 is 2 bytes
}

size
}
}
Expand All @@ -57,6 +62,9 @@ impl Encodable for QuorumFinalizationCommitment {
len += self.version.consensus_encode(w)?;
len += self.llmq_type.consensus_encode(w)?;
len += self.quorum_hash.consensus_encode(w)?;
if self.version == 2 || self.version == 4 {
len += self.quorum_index.unwrap().consensus_encode(w)?;
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent potential panic when unwrapping quorum_index

Similar to the earlier comment, unwrapping quorum_index without checking if it's Some can cause a panic. Ensure that quorum_index is handled safely during encoding.

Apply the same modification as suggested for lines 37-37 to handle quorum_index safely.

len += self.signers.consensus_encode(w)?;
len += self.valid_members.consensus_encode(w)?;
len += self.quorum_public_key.consensus_encode(w)?;
Expand All @@ -72,8 +80,11 @@ impl Decodable for QuorumFinalizationCommitment {
let version = u16::consensus_decode(r)?;
let llmq_type = u8::consensus_decode(r)?;
let quorum_hash = QuorumHash::consensus_decode(r)?;
let signers = Vec::<u8>::consensus_decode(r)?;
let valid_members = Vec::<u8>::consensus_decode(r)?;
let quorum_index = if version == 2 || version == 4 { Some(i16::consensus_decode(r)?) } else { None };
let signers_count = read_compact_size(r)?;
let signers = read_fixed_bitset(r, signers_count as usize)?;
let valid_members_count = read_compact_size(r)?;
let valid_members = read_fixed_bitset(r, valid_members_count as usize)?;
let quorum_public_key = BLSPublicKey::consensus_decode(r)?;
let quorum_vvec_hash = QuorumVVecHash::consensus_decode(r)?;
let quorum_sig = BLSSignature::consensus_decode(r)?;
Expand All @@ -82,8 +93,9 @@ impl Decodable for QuorumFinalizationCommitment {
version,
llmq_type,
quorum_hash,
signers,
valid_members,
quorum_index,
signers: signers.iter().map(|&b| b as u8).collect(),
valid_members: valid_members.iter().map(|&b| b as u8).collect(),
Comment on lines +98 to +100
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify conversion from Vec<bool> to Vec<u8>

In the consensus_decode method, you're converting a Vec<bool> to a Vec<u8> by mapping each boolean to a u8. This could be streamlined if the original data is intended to be a byte array.

If signers and valid_members represent bitfields, consider using a dedicated bitset structure or keeping them as Vec<u8> throughout to simplify the code.

quorum_public_key,
quorum_vvec_hash,
quorum_sig,
Expand Down Expand Up @@ -130,6 +142,54 @@ impl Decodable for QuorumCommitmentPayload {
}
}

fn read_compact_size<R: Read + ?Sized>(r: &mut R) -> io::Result<u64> {
let mut marker = [0u8; 1];
r.read_exact(&mut marker)?;
match marker[0] {
0xFD => {
// Read the next 2 bytes as a little-endian u16
let mut buf = [0u8; 2];
r.read_exact(&mut buf)?;
Ok(u16::from_le_bytes(buf) as u64)
}
0xFE => {
// Read the next 4 bytes as a little-endian u32
let mut buf = [0u8; 4];
r.read_exact(&mut buf)?;
Ok(u32::from_le_bytes(buf) as u64)
}
0xFF => {
// Read the next 8 bytes as a little-endian u64
let mut buf = [0u8; 8];
r.read_exact(&mut buf)?;
Ok(u64::from_le_bytes(buf))
}
value => {
// For values less than 253, the value is stored directly in the marker byte
Ok(value as u64)
}
}
}
Comment on lines +147 to +174
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider reusing existing VarInt decoding instead of read_compact_size.

The read_compact_size function replicates functionality already provided by VarInt::consensus_decode. Reusing existing implementations can reduce code duplication and potential errors.

Replace read_compact_size with VarInt::consensus_decode:

-let signers_count = read_compact_size(r)?;
+let signers_count = VarInt::consensus_decode(r)?.0;

Make similar changes for valid_members_count.

If read_compact_size is no longer used after these changes, consider removing it to keep the codebase lean and maintainable.

Committable suggestion was skipped due to low confidence.


fn read_fixed_bitset<R: Read + ?Sized>(r: &mut R, size: usize) -> std::io::Result<Vec<bool>> {
// Calculate the number of bytes needed
let num_bytes = (size + 7) / 8;
let mut bytes = vec![0u8; num_bytes];

// Read bytes from the reader
r.read_exact(&mut bytes)?;

// Unpack bits into a vector of bools
let mut bits = Vec::with_capacity(size);
for p in 0..size {
let byte = bytes[p / 8];
let bit = (byte >> (p % 8)) & 1;
bits.push(bit != 0);
}

Ok(bits)
}

#[cfg(test)]
mod tests {
use hashes::Hash;
Expand All @@ -151,6 +211,7 @@ mod tests {
version: 0,
llmq_type: 0,
quorum_hash: QuorumHash::all_zeros(),
quorum_index: None,
signers: vec![1, 2, 3, 4, 5],
valid_members: vec![6, 7, 8, 9, 0],
quorum_public_key: BLSPublicKey::from([0; 48]),
Expand Down
Loading