[WIP] Implement real transaction system for wallet and RPC#90
Conversation
…et GUI - Add Transaction::signing_hash() method to bitcell-consensus that computes hash excluding signature field - Update RPC eth_sendRawTransaction to validate signatures using signing_hash instead of hash - Implement real transaction creation, signing, and broadcasting in wallet GUI - Add Wallet::get_secret_key_for_address() method for consensus transaction signing - Add parse_address_to_pubkey() helper function in wallet GUI - Replace mock "Hardware wallet coming soon" message with actual transaction flow Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
- Add bitcell-consensus and bincode dependencies to wallet-gui Cargo.toml - Fix Signature::from_bytes usage (doesn't return Result) - Fix Send trait issue by extracting wallet data before async spawn - Get secret key before entering async block to avoid borrowing Rc in tokio::spawn - Wallet GUI now successfully builds Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements a real transaction system for the BitCell wallet, replacing mock transaction formatting with proper cryptographic signing and consensus transaction creation. The implementation adds the critical signing_hash() method to compute transaction hashes excluding the signature field, updates the RPC layer to validate signatures correctly, and enables the wallet GUI to create, sign, and broadcast real transactions.
Key Changes
- Added
Transaction::signing_hash()method for proper cryptographic signing (excludes signature field from hash computation) - Updated RPC signature validation to use
signing_hash()instead of full transaction hash - Implemented transaction creation and signing flow in wallet GUI with proper async handling
- Added
Wallet::get_secret_key_for_address()for advanced signing use cases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/bitcell-consensus/src/block.rs |
Adds signing_hash() method to compute hash of transaction data excluding signature field |
crates/bitcell-node/src/rpc.rs |
Updates signature validation to use signing_hash() and clarifies hash usage in comments |
crates/bitcell-wallet/src/wallet.rs |
Adds get_secret_key_for_address() method to expose secret keys for consensus transaction signing |
crates/bitcell-wallet-gui/src/main.rs |
Implements full transaction creation, signing, and broadcast flow with address parsing helper |
crates/bitcell-wallet-gui/Cargo.toml |
Adds dependencies for consensus transactions (bitcell-consensus, bincode) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
There was a problem hiding this comment.
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.
| 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()) |
| } 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 | ||
| }; |
There was a problem hiding this comment.
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:
- Store the mapping from address to public key in the wallet
- Use a different address format that encodes the full public key
- Require addresses to be provided in raw hex public key format for transactions
| } | ||
|
|
||
| /// Parse address string to PublicKey | ||
| /// For BitCell addresses, the address is the hex-encoded public key with optional prefix |
There was a problem hiding this comment.
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.
| gas_limit, | ||
| gas_price, | ||
| data: vec![], | ||
| signature: bitcell_crypto::Signature::from_bytes([0u8; 64]), // Placeholder |
There was a problem hiding this comment.
[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 placeholderThis makes it immediately obvious in debugging if an unsigned transaction is accidentally used.
| signature: bitcell_crypto::Signature::from_bytes([0u8; 64]), // Placeholder | |
| signature: bitcell_crypto::Signature::from_bytes([0xFFu8; 64]), // Invalid placeholder |
| /// 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) | ||
| } |
There was a problem hiding this comment.
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:
- The signing hash excludes the signature field
- Signing and verification work correctly using the signing hash
- The signing hash differs from the regular transaction hash
- 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);
}| /// 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) |
There was a problem hiding this comment.
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:
- 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 - Return a guard object that automatically zeros the key when dropped
- Provide a more specific
sign_consensus_transactionmethod 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))
}| /// 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)) |
Implementation Plan: Real Transaction System for Wallet and RPC
Phase 1: Wallet GUI Transaction Creation & Signing
Phase 2: RPC Transaction Processing
Phase 3: Balance Updates
Phase 4: Testing & Validation
Changes Made
Core Transaction Signing:
Transaction::signing_hash()method that computes the hash of transaction data excluding the signature fieldsigning_hash()for signature verificationWallet GUI Implementation:
Wallet Core:
Wallet::get_secret_key_for_address()method for advanced use casesHelper Functions:
parse_address_to_pubkey()to convert string addresses to PublicKey formatBuild Status
✅ All packages build successfully:
Next Steps
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.