Skip to content
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
16 changes: 16 additions & 0 deletions crates/bitcell-consensus/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,22 @@ impl Transaction {
let serialized = bincode::serialize(self).expect("transaction serialization should never fail");
Hash256::hash(&serialized)
}

/// Compute signing hash (hash of transaction data WITHOUT signature)
///
/// This is the hash that should be signed/verified, as it excludes the signature field.
/// The regular hash() includes the signature and cannot be used for signing.
pub fn signing_hash(&self) -> Hash256 {
let mut data = Vec::new();
data.extend_from_slice(&self.nonce.to_le_bytes());
data.extend_from_slice(self.from.as_bytes());
data.extend_from_slice(self.to.as_bytes());
data.extend_from_slice(&self.amount.to_le_bytes());
data.extend_from_slice(&self.gas_limit.to_le_bytes());
data.extend_from_slice(&self.gas_price.to_le_bytes());
data.extend_from_slice(&self.data);
Hash256::hash(&data)
}
Comment on lines +116 to +130
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The new signing_hash() method lacks test coverage. Given the critical nature of transaction signing (incorrect implementation could lead to funds loss or security vulnerabilities), this method should have comprehensive tests that verify:

  1. The signing hash excludes the signature field
  2. Signing and verification work correctly using the signing hash
  3. The signing hash differs from the regular transaction hash
  4. Changing any transaction field (except signature) changes the signing hash

Consider adding a test like:

#[test]
fn test_transaction_signing_hash() {
    let sk = SecretKey::generate();
    let mut tx = Transaction {
        nonce: 1,
        from: sk.public_key(),
        to: sk.public_key(),
        amount: 100,
        gas_limit: 21000,
        gas_price: 1,
        data: vec![],
        signature: Signature::from_bytes([0u8; 64]),
    };
    
    // Sign with signing_hash
    let signing_hash = tx.signing_hash();
    tx.signature = sk.sign(signing_hash.as_bytes());
    
    // Verify signature is valid
    assert!(tx.signature.verify(&tx.from, signing_hash.as_bytes()).is_ok());
    
    // Verify signing_hash != full hash
    assert_ne!(tx.signing_hash(), tx.hash());
    
    // Verify signature changes don't affect signing_hash
    let hash1 = tx.signing_hash();
    tx.signature = sk.sign(b"different");
    let hash2 = tx.signing_hash();
    assert_eq!(hash1, hash2);
}

Copilot uses AI. Check for mistakes.
}

/// Battle proof (placeholder for ZK proof)
Expand Down
9 changes: 5 additions & 4 deletions crates/bitcell-node/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,9 +514,9 @@ async fn eth_send_raw_transaction(state: &RpcState, params: Option<Value>) -> Re
data: None,
})?;

// Validate transaction signature
let tx_hash = tx.hash();
if tx.signature.verify(&tx.from, tx_hash.as_bytes()).is_err() {
// Validate transaction signature (must sign the data EXCLUDING the signature field)
let signing_hash = tx.signing_hash();
if tx.signature.verify(&tx.from, signing_hash.as_bytes()).is_err() {
return Err(JsonRpcError {
code: -32602,
message: "Invalid transaction signature".to_string(),
Expand Down Expand Up @@ -612,7 +612,8 @@ async fn eth_send_raw_transaction(state: &RpcState, params: Option<Value>) -> Re
});
}

// Return transaction hash
// Return transaction hash (use full hash for identification, not signing hash)
let tx_hash = tx.hash();
Ok(json!(format!("0x{}", hex::encode(tx_hash.as_bytes()))))
}

Expand Down
2 changes: 2 additions & 0 deletions crates/bitcell-wallet-gui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ path = "src/main.rs"
[dependencies]
bitcell-wallet = { path = "../bitcell-wallet" }
bitcell-crypto = { path = "../bitcell-crypto" }
bitcell-consensus = { path = "../bitcell-consensus" }

# Slint UI framework - native rendering, no WebView
slint = "1.9"

# Serialization
serde.workspace = true
serde_json = "1.0"
bincode = "1.3"

# Error handling
thiserror.workspace = true
Expand Down
172 changes: 139 additions & 33 deletions crates/bitcell-wallet-gui/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,36 @@ fn chain_display_name(chain: Chain) -> &'static str {
}
}

/// Parse address string to PublicKey
/// For BitCell addresses, the address is the hex-encoded public key with optional prefix
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The comment states that for BitCell addresses "the address is the hex-encoded public key with optional prefix", but this is not accurate. According to the Address implementation, BitCell addresses are base58-encoded hashes of the public key (similar to Bitcoin), not the raw public key itself. The comment should be corrected to reflect the actual encoding scheme.

Additionally, the current implementation will not work correctly for parsing actual BitCell addresses as created by the wallet.

Copilot uses AI. Check for mistakes.
fn parse_address_to_pubkey(address: &str) -> Result<bitcell_crypto::PublicKey, String> {
// Remove common prefixes
let address = address.trim();
let address = if address.starts_with("0x") {
&address[2..]
} else if address.starts_with("BC1") || address.starts_with("bc1") {
// BitCell address format - for now, just strip prefix
// In a real implementation, this would decode the address properly
&address[3..]
} else {
address
};
Comment on lines +67 to +73
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The address parsing logic assumes BitCell addresses are hex-encoded after stripping the "BC1" prefix, but according to Address::to_string_formatted() (line 122 in address.rs), BitCell addresses are actually base58-encoded, not hex-encoded. This mismatch will cause transaction creation to fail when using BitCell addresses.

The parsing should use:

} else if address.starts_with("BC1") || address.starts_with("bc1") {
    // BitCell addresses are base58-encoded, need to decode
    let bytes = bs58::decode(&address[3..])
        .into_vec()
        .map_err(|e| format!("Invalid base58 in BitCell address: {}", e))?;
    // Then need to extract public key from the address bytes...

However, note that BitCell addresses appear to encode the address bytes (which are derived from the public key hash), not the public key itself. You may need to rethink the approach here - either:

  1. Store the mapping from address to public key in the wallet
  2. Use a different address format that encodes the full public key
  3. Require addresses to be provided in raw hex public key format for transactions

Copilot uses AI. Check for mistakes.

// Decode hex to bytes
let bytes = hex::decode(address)
.map_err(|e| format!("Invalid hex in address: {}", e))?;

if bytes.len() != 33 {
return Err(format!("Address must be 33 bytes (compressed public key), got {}", bytes.len()));
}

let mut key_bytes = [0u8; 33];
key_bytes.copy_from_slice(&bytes);

bitcell_crypto::PublicKey::from_bytes(key_bytes)
.map_err(|e| format!("Invalid public key: {}", e))
}

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
// Initialize logging
Expand Down Expand Up @@ -416,10 +446,10 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
// Convert to smallest units (1 CELL = 100_000_000 units)
let amount_units = (amount * 100_000_000.0) as u64;

// Get wallet and RPC client
let app_state = state.borrow();

let (from_address, rpc_client) = {
// Get wallet info and secret key before async operation
let (from_addr_formatted, secret_key, rpc_client) = {
let app_state = state.borrow();
let wallet = match &app_state.wallet {
Some(w) => w,
None => {
Expand All @@ -428,16 +458,30 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
}
};

if !wallet.is_unlocked() {
wallet_state.set_status_message("Wallet is locked. Please unlock it first.".into());
return;
}

// Get the first address as sender
let addresses = wallet.all_addresses();
let from_addr = match addresses.iter().find(|a| a.chain() == chain) {
Some(a) => a.to_string_formatted(),
let from_addr_obj = match addresses.iter().find(|a| a.chain() == chain) {
Some(a) => a,
None => {
wallet_state.set_status_message(format!("No {} address available", chain_display_name(chain)).into());
return;
}
};

// Get secret key for signing
let sk = match wallet.get_secret_key_for_address(from_addr_obj) {
Ok(sk) => sk,
Err(e) => {
wallet_state.set_status_message(format!("Failed to get secret key: {}", e).into());
return;
}
};

let rpc = match &app_state.rpc_client {
Some(c) => c.clone(),
None => {
Expand All @@ -446,23 +490,20 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
}
};

(from_addr, rpc)
(from_addr_obj.to_string_formatted(), sk, rpc)
};

// Drop app_state borrow before the async operation
drop(app_state);

// Set loading state
wallet_state.set_is_loading(true);
wallet_state.set_status_message("Preparing transaction...".into());

let window_weak = window.as_weak();
let to_address = to_address.to_string();

// Async nonce fetch and transaction preparation
// Async nonce fetch and transaction creation
tokio::spawn(async move {
// Get nonce from node
let nonce = match rpc_client.get_transaction_count(&from_address).await {
let nonce = match rpc_client.get_transaction_count(&from_addr_formatted).await {
Ok(n) => n,
Err(e) => {
let _ = slint::invoke_from_event_loop(move || {
Expand All @@ -482,30 +523,95 @@ fn setup_callbacks(window: &MainWindow, state: Rc<RefCell<AppState>>) {
Err(_) => DEFAULT_GAS_PRICE, // Use default if unavailable
};

// Calculate fee (simple estimate)
let fee = gas_price.saturating_mul(21000);
// Gas limit for simple transfer
let gas_limit = 21000u64;

// For now, display transaction details and inform user signing requires wallet unlock
// In production, this would integrate with hardware wallet or secure key management
let tx_info = format!(
"Transaction prepared:\n\
From: {}\n\
To: {}\n\
Amount: {} units\n\
Fee: {} units\n\
Nonce: {}\n\n\
Hardware wallet signing coming soon. \
Use the CLI or Admin console with HSM for secure signing.",
from_address, to_address, amount_units, fee, nonce
);
// Parse addresses to PublicKey format
let from_pk = match parse_address_to_pubkey(&from_addr_formatted) {
Ok(pk) => pk,
Err(e) => {
let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(format!("Invalid from address: {}", e).into());
}
});
return;
}
};

let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(tx_info.into());
let to_pk = match parse_address_to_pubkey(&to_address) {
Ok(pk) => pk,
Err(e) => {
let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(format!("Invalid to address: {}", e).into());
}
});
return;
}
});
};

// Create consensus transaction (without signature initially)
let mut tx = bitcell_consensus::Transaction {
nonce,
from: from_pk,
to: to_pk,
amount: amount_units,
gas_limit,
gas_price,
data: vec![],
signature: bitcell_crypto::Signature::from_bytes([0u8; 64]), // Placeholder
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment on line 567 says "Placeholder" but a zero signature might be indistinguishable from a valid signature in some edge cases. While this is immediately overwritten on line 574, consider using a more explicit placeholder that would clearly fail verification if accidentally used, such as:

signature: bitcell_crypto::Signature::from_bytes([0xFFu8; 64]), // Invalid placeholder

This makes it immediately obvious in debugging if an unsigned transaction is accidentally used.

Suggested change
signature: bitcell_crypto::Signature::from_bytes([0u8; 64]), // Placeholder
signature: bitcell_crypto::Signature::from_bytes([0xFFu8; 64]), // Invalid placeholder

Copilot uses AI. Check for mistakes.
};

// Compute signing hash (hash of transaction WITHOUT signature field)
let signing_hash = tx.signing_hash();

// Sign the transaction
tx.signature = secret_key.sign(signing_hash.as_bytes());

// Serialize transaction
let tx_bytes = match bincode::serialize(&tx) {
Ok(bytes) => bytes,
Err(e) => {
let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(format!("Failed to serialize transaction: {}", e).into());
}
});
return;
}
};

// Send transaction via RPC
match rpc_client.send_raw_transaction_bytes(&tx_bytes).await {
Ok(tx_hash) => {
let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(format!(
"Transaction sent successfully!\nHash: {}",
tx_hash
).into());
}
});
}
Err(e) => {
let _ = slint::invoke_from_event_loop(move || {
if let Some(window) = window_weak.upgrade() {
let ws = window.global::<WalletState>();
ws.set_is_loading(false);
ws.set_status_message(format!("Failed to send transaction: {}", e).into());
}
});
}
}
});
});
}
Expand Down
25 changes: 25 additions & 0 deletions crates/bitcell-wallet/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,31 @@ impl Wallet {
self.sign_transaction(tx, from)
}

/// Get the secret key for an address (for advanced use cases like consensus transaction signing)
///
/// This method should be used with caution as it exposes the raw secret key.
/// Prefer using sign_transaction when possible.
pub fn get_secret_key_for_address(&self, address: &Address) -> Result<SecretKey> {
if !self.is_unlocked() {
return Err(Error::WalletLocked);
}

let path = DerivationPath::for_chain(address.chain(), address.index());

// We need to derive the key without caching (since self is immutable)
let seed = self.master_seed.as_ref().ok_or(Error::WalletLocked)?;

let path_str = path.to_string();
let mut derivation_data = Vec::new();
derivation_data.extend_from_slice(seed.as_bytes());
derivation_data.extend_from_slice(path_str.as_bytes());

let derived_hash = Hash256::hash(&derivation_data);
let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?;

Ok(secret_key)
Comment on lines +415 to +423
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The key derivation logic in get_secret_key_for_address duplicates the derivation code from the private derive_key method (lines 237-245). This code duplication makes the implementation harder to maintain and introduces a risk of inconsistency if the derivation algorithm is updated in one place but not the other.

Consider refactoring to extract the core derivation logic into a private helper method that both derive_key and get_secret_key_for_address can use. For example:

fn derive_secret_key_from_seed(seed: &SeedBytes, path: &DerivationPath) -> Result<SecretKey> {
    let path_str = path.to_string();
    let mut derivation_data = Vec::new();
    derivation_data.extend_from_slice(seed.as_bytes());
    derivation_data.extend_from_slice(path_str.as_bytes());
    
    let derived_hash = Hash256::hash(&derivation_data);
    SecretKey::from_bytes(derived_hash.as_bytes())
}

Then both methods can call this helper, ensuring consistency.

Suggested change
let path_str = path.to_string();
let mut derivation_data = Vec::new();
derivation_data.extend_from_slice(seed.as_bytes());
derivation_data.extend_from_slice(path_str.as_bytes());
let derived_hash = Hash256::hash(&derivation_data);
let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?;
Ok(secret_key)
Self::derive_secret_key_from_seed(seed, &path)
}
/// Helper to derive a secret key from seed and derivation path
fn derive_secret_key_from_seed(seed: &SeedBytes, path: &DerivationPath) -> Result<SecretKey> {
let path_str = path.to_string();
let mut derivation_data = Vec::new();
derivation_data.extend_from_slice(seed.as_bytes());
derivation_data.extend_from_slice(path_str.as_bytes());
let derived_hash = Hash256::hash(&derivation_data);
SecretKey::from_bytes(derived_hash.as_bytes())

Copilot uses AI. Check for mistakes.
Comment on lines +401 to +423
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The get_secret_key_for_address method exposes raw secret keys, which is a significant security risk. While the documentation warns about this, the API design could be improved to reduce the attack surface.

Consider alternative approaches:

  1. Accept a callback function that receives the secret key temporarily: fn with_secret_key_for_address<F, R>(&self, address: &Address, f: F) -> Result<R> where F: FnOnce(&SecretKey) -> R
  2. Return a guard object that automatically zeros the key when dropped
  3. Provide a more specific sign_consensus_transaction method that handles the signing internally without exposing the raw key

The callback approach is particularly good as it ensures the secret key never leaves the wallet's control and is automatically dropped after use:

pub fn with_secret_key_for_address<F, R>(&self, address: &Address, f: F) -> Result<R> 
where 
    F: FnOnce(&SecretKey) -> R 
{
    if !self.is_unlocked() {
        return Err(Error::WalletLocked);
    }
    
    let path = DerivationPath::for_chain(address.chain(), address.index());
    let seed = self.master_seed.as_ref().ok_or(Error::WalletLocked)?;
    
    let path_str = path.to_string();
    let mut derivation_data = Vec::new();
    derivation_data.extend_from_slice(seed.as_bytes());
    derivation_data.extend_from_slice(path_str.as_bytes());
    
    let derived_hash = Hash256::hash(&derivation_data);
    let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?;
    
    Ok(f(&secret_key))
}
Suggested change
/// Get the secret key for an address (for advanced use cases like consensus transaction signing)
///
/// This method should be used with caution as it exposes the raw secret key.
/// Prefer using sign_transaction when possible.
pub fn get_secret_key_for_address(&self, address: &Address) -> Result<SecretKey> {
if !self.is_unlocked() {
return Err(Error::WalletLocked);
}
let path = DerivationPath::for_chain(address.chain(), address.index());
// We need to derive the key without caching (since self is immutable)
let seed = self.master_seed.as_ref().ok_or(Error::WalletLocked)?;
let path_str = path.to_string();
let mut derivation_data = Vec::new();
derivation_data.extend_from_slice(seed.as_bytes());
derivation_data.extend_from_slice(path_str.as_bytes());
let derived_hash = Hash256::hash(&derivation_data);
let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?;
Ok(secret_key)
/// Perform an operation with the secret key for an address (for advanced use cases like consensus transaction signing)
///
/// This method is safer than exposing the raw secret key. The key is only available inside the callback and is dropped immediately after.
/// Prefer using sign_transaction when possible.
pub fn with_secret_key_for_address<F, R>(&self, address: &Address, f: F) -> Result<R>
where
F: FnOnce(&SecretKey) -> R,
{
if !self.is_unlocked() {
return Err(Error::WalletLocked);
}
let path = DerivationPath::for_chain(address.chain(), address.index());
let seed = self.master_seed.as_ref().ok_or(Error::WalletLocked)?;
let path_str = path.to_string();
let mut derivation_data = Vec::new();
derivation_data.extend_from_slice(seed.as_bytes());
derivation_data.extend_from_slice(path_str.as_bytes());
let derived_hash = Hash256::hash(&derivation_data);
let secret_key = SecretKey::from_bytes(derived_hash.as_bytes())?;
Ok(f(&secret_key))

Copilot uses AI. Check for mistakes.
}

/// Get transaction history
pub fn history(&self) -> &TransactionHistory {
&self.history
Expand Down