Merge rc1 into feature/economic-system-and-rewards with security hardening#27
Conversation
… in favor of production code Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
…and use MAX_HALVINGS constant Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR claims to merge rc1 production code into a feature branch with security hardening, but the actual changes appear to do the opposite - they systematically remove security features, performance optimizations, and operational capabilities that were present in rc1. This represents a critical regression rather than an improvement.
Critical Issues Found
Security Regressions:
- ZKP non-equality constraint broken - The state circuit's constraint to prevent unchanged state is incomplete and non-functional
- VRF chaining weakened - Proper VRF output chaining was replaced with predictable input mixing
- Transaction validation removed - DoS protections and gas validation for new accounts eliminated
Performance Regressions:
- Transaction indexing removed - O(1) hash-based lookups replaced with O(n) linear scans
- Block scanning limited to 100 blocks - Transaction lookup artificially capped
API Breaking Changes:
- Standard RPC methods removed -
eth_getTransactionCountandeth_gasPriceremoved, breaking Ethereum wallet compatibility - Node info degraded - Removed
chain_heightandpeer_countfields - Block data placeholders - Real data replaced with TODO placeholders
Operational Regressions:
- Logging framework removed - Structured tracing replaced with println/eprintln throughout
- Error context degraded - Detailed error logging replaced with silent failures or generic messages
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| state_circuit.rs | Removed working non-equality constraint implementation and test, leaving broken stub with misleading comments |
| battle_circuit.rs | Removed comprehensive test coverage for winner ID validation |
| lib.rs (state) | Degraded error logging from detailed context to silent failures |
| rpc.rs | Removed critical RPC methods, transaction indexing, validation logic, and replaced real data with placeholders |
| network.rs | Replaced structured logging with println/eprintln throughout |
| dht.rs | Replaced structured logging with println/eprintln |
| blockchain.rs | Removed transaction indexing, weakened VRF chaining, degraded error handling and documentation |
Recommendation: This PR should not be merged in its current state. It appears the feature branch code was taken instead of rc1 code as claimed, resulting in widespread regressions. A proper merge should preserve rc1's security features, performance optimizations, and operational capabilities.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Err(JsonRpcError { | ||
| code: -32602, | ||
| message: "Account not found".to_string(), | ||
| data: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The removed validation logic for transactions from non-existent accounts (lines 453-457 now just return an error) eliminates important security controls:
- Nonce validation for new accounts: The removed code ensured new accounts start with nonce 0, preventing replay attacks
- Gas parameter validation: Removed checks for zero gas price/limit and maximum bounds (MAX_GAS_PRICE, MAX_GAS_LIMIT) that prevent DoS attacks
- DoS mitigation: The removed logic included explicit DoS attack prevention with documented reasoning
The new simplified code at lines 453-457 just rejects all transactions from non-existent accounts, which may break legitimate use cases like initial funding transactions. This appears to contradict the PR's stated goal of "security hardening".
| tracing::error!("Lock poisoned in validate_transaction() - prior panic detected: {}", e); | ||
| e.into_inner() | ||
| }); | ||
| let state = self.state.read().unwrap(); |
There was a problem hiding this comment.
Error handling for lock poisoning has been downgraded throughout the blockchain module. The removed code provided detailed error logging with context about which operation encountered a poisoned lock (e.g., "Lock poisoned in height() - prior panic detected").
The new code (lines 86, 94, 102, 138, 253, 292, 298, 302, 335) uses generic eprintln messages or just recovers silently. While lock recovery is still performed, the reduced error context makes it harder to diagnose the root cause of panics in production. This is especially important for a blockchain where data consistency is critical.
| let state = self.state.read().unwrap(); | |
| let state = match self.state.read() { | |
| Ok(guard) => guard, | |
| Err(poisoned) => { | |
| eprintln!("Lock poisoned in validate_transaction() - prior panic detected"); | |
| poisoned.into_inner() | |
| } | |
| }; |
| // Search in blockchain (inefficient linear scan for now, need index later) | ||
| let height = state.blockchain.height(); | ||
| // Scan last 100 blocks for efficiency in this demo | ||
| let start_height = if height > 100 { height - 100 } else { 0 }; | ||
|
|
||
| for h in (start_height..=height).rev() { | ||
| if let Some(block) = state.blockchain.get_block(h) { | ||
| for (i, tx) in block.transactions.iter().enumerate() { | ||
| if tx.hash() == target_hash { | ||
| return Ok(json!({ | ||
| "hash": format!("0x{}", hex::encode(tx.hash().as_bytes())), | ||
| "nonce": format!("0x{:x}", tx.nonce), | ||
| "blockHash": format!("0x{}", hex::encode(block.hash().as_bytes())), | ||
| "blockNumber": format!("0x{:x}", block.header.height), | ||
| "transactionIndex": format!("0x{:x}", i), | ||
| "from": format!("0x{}", hex::encode(tx.from.as_bytes())), | ||
| "to": format!("0x{}", hex::encode(tx.to.as_bytes())), | ||
| "value": format!("0x{:x}", tx.amount), | ||
| "gas": format!("0x{:x}", tx.gas_limit), | ||
| "gasPrice": format!("0x{:x}", tx.gas_price), | ||
| "input": format!("0x{}", hex::encode(&tx.data)), | ||
| })); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The transaction lookup implementation has been changed from an efficient O(1) hash-based index to an inefficient O(n) linear scan through the last 100 blocks. This is a significant performance regression that will cause problems as the blockchain grows.
The comment at line 282 acknowledges this is "inefficient" and needs "index later", but this appears to be a step backwards from the rc1 version which had the index already implemented. This contradicts the PR description which states conflicts were resolved by "taking rc1 versions" - this appears to be taking the feature branch version instead.
| @@ -1,10 +1,4 @@ | |||
| ///! Blockchain manager for block production and validation | |||
There was a problem hiding this comment.
Detailed module-level documentation for the blockchain module has been removed (lines 2-7), which explained:
- Block production with VRF-based proposer selection
- Block validation requirements
- Transaction indexing
- State management with Merkle trees
This high-level documentation helped developers understand the module's purpose and capabilities. The removal reduces code comprehension, especially for new contributors.
| ///! Blockchain manager for block production and validation | |
| //! Blockchain module | |
| //! | |
| //! This module manages block production, validation, transaction indexing, and state management. | |
| //! | |
| //! - **Block production**: Uses VRF-based proposer selection to determine which node produces the next block. | |
| //! - **Block validation**: Enforces consensus rules, verifies block headers, and checks cryptographic proofs. | |
| //! - **Transaction indexing**: Maintains efficient lookup and retrieval of transactions within blocks. | |
| //! - **State management**: Utilizes Merkle trees to track and verify the global state of accounts and balances. | |
| //! | |
| //! This high-level documentation helps developers understand the module's purpose and capabilities. |
| ]; | ||
|
|
||
| assert!(StateCircuit::verify(&vk, &proof, &public_inputs).unwrap()); | ||
| } |
There was a problem hiding this comment.
The test test_state_circuit_rejects_same_roots was removed, which was specifically testing that the circuit rejects cases where old_root == new_root. This test removal masks the bug at lines 86-100 where the non-equality constraint is incomplete. Without this test, the broken constraint logic won't be caught by the test suite.
| "eth_getBlockByNumber" => eth_get_block_by_number(&state, req.params).await, | ||
| "eth_getTransactionByHash" => eth_get_transaction_by_hash(&state, req.params).await, | ||
| "eth_getBalance" => eth_get_balance(&state, req.params).await, | ||
| "eth_sendRawTransaction" => eth_send_raw_transaction(&state, req.params).await, |
There was a problem hiding this comment.
Critical RPC methods eth_getTransactionCount and eth_gas_price have been removed. These are standard Ethereum JSON-RPC methods that clients depend on:
eth_getTransactionCount: Required for clients to determine the correct nonce for new transactionseth_gasPrice: Required for clients to set appropriate gas prices
Removing these methods will break Ethereum wallet compatibility and client integrations. The removal also contradicts the PR description which states the goal was to take rc1 versions that had these methods implemented.
| "eth_sendRawTransaction" => eth_send_raw_transaction(&state, req.params).await, | |
| "eth_sendRawTransaction" => eth_send_raw_transaction(&state, req.params).await, | |
| "eth_getTransactionCount" => eth_get_transaction_count(&state, req.params).await, | |
| "eth_gasPrice" => eth_gas_price(&state).await, |
| "nonce": "0x0000000000000000", // TODO: Use work/nonce | ||
| "sha3Uncles": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347", // Empty uncle hash | ||
| "logsBloom": format!("0x{}", hex::encode(&EMPTY_BLOOM_FILTER)), | ||
| "logsBloom": "0x00", // TODO: Bloom filter | ||
| "transactionsRoot": format!("0x{}", hex::encode(block.header.tx_root.as_bytes())), | ||
| "stateRoot": format!("0x{}", hex::encode(block.header.state_root.as_bytes())), | ||
| "receiptsRoot": "0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421", // Empty receipts root | ||
| "miner": format!("0x{}", hex::encode(block.header.proposer.as_bytes())), | ||
| "difficulty": "0x1", | ||
| "totalDifficulty": format!("0x{:x}", block.header.height), // Simplified | ||
| "extraData": "0x", | ||
| "size": format!("0x{:x}", block_size), | ||
| "size": format!("0x{:x}", 1000), // TODO: Real size | ||
| "gasLimit": "0x1fffffffffffff", | ||
| "gasUsed": "0x0", | ||
| "timestamp": format!("0x{:x}", block.header.timestamp), | ||
| "transactions": transactions, | ||
| "uncles": [], | ||
| "vrfOutput": format!("0x{}", hex::encode(block.header.vrf_output)), | ||
| "battleProofsCount": block.battle_proofs.len() | ||
| "uncles": [] | ||
| })) |
There was a problem hiding this comment.
The block response has been degraded with placeholder TODO values instead of real data:
- Line 214:
nonceset to hardcoded "0x0000000000000000" instead of usingblock.header.work - Line 216:
logsBloomset to "0x00" instead of proper 256-byte bloom filter - Line 224:
sizeset to hardcoded 1000 instead of actual block size - Lines 229: Removed
vrfOutputandbattleProofsCountfields
This reduces API functionality and provides incorrect data to clients. The rc1 version had proper implementations with actual block data.
| latest_hash: Arc::new(RwLock::new(genesis_hash)), | ||
| blocks: Arc::new(RwLock::new(blocks)), | ||
| tx_index: Arc::new(RwLock::new(HashMap::new())), | ||
| state: Arc::new(RwLock::new(StateManager::new())), |
There was a problem hiding this comment.
The transaction indexing infrastructure has been completely removed, including:
- The
TxLocationstruct (lines 14-19 removed) - The
tx_indexHashMap field (line 26 removed) - The
get_transaction_by_hash()method (lines 107-138 removed) - Transaction indexing in
add_block()(lines 290-305 removed)
This removal breaks the RPC eth_getTransactionByHash method which now has to fall back to inefficient linear scans (see rpc.rs:282-307). This is a major performance regression that contradicts the PR's claim of taking rc1 versions, as rc1 had this optimization already implemented.
| if let Some(storage) = &self.storage { | ||
| if let Err(e) = storage.store_account(&pubkey, &account) { | ||
| tracing::error!( | ||
| pubkey = %hex::encode(&pubkey), | ||
| error = %e, | ||
| "Failed to persist account to storage. State may be inconsistent on restart." | ||
| ); | ||
| } | ||
| let _ = storage.store_account(&pubkey, &account); | ||
| } |
There was a problem hiding this comment.
Error handling for storage persistence has been degraded from explicit logging with context to silent failure. The removed code (lines 117-123) provided detailed error logging with the public key and error message when storage operations failed, helping diagnose issues.
The new code (line 117) silently discards errors with let _ =, providing no visibility into storage failures. This makes it impossible to detect and debug storage issues in production, potentially leading to state inconsistencies between memory and disk that won't be discovered until restart.
| // Constraint: old_root != new_root (state must change) | ||
| // To prove non-equality, we use the following approach: | ||
| // 1. Compute diff = new_root - old_root | ||
| // 2. Compute inv = inverse(diff) as a witness | ||
| // 3. Enforce: diff * inv = 1 | ||
| // This proves diff != 0, which proves new_root != old_root | ||
| // (new_root - old_root) * inv = 1 | ||
| // This proves new_root - old_root != 0 | ||
|
|
||
| // Step 1: Compute diff = new_root - old_root | ||
| let diff = cs.new_witness_variable(|| { | ||
| let old = self.old_state_root.ok_or(SynthesisError::AssignmentMissing)?; | ||
| let new = self.new_state_root.ok_or(SynthesisError::AssignmentMissing)?; | ||
| Ok(new - old) | ||
| })?; | ||
|
|
||
| // Enforce: diff = new_root - old_root | ||
| cs.enforce_constraint( | ||
| ark_relations::lc!() + new_root - old_root, | ||
| ark_relations::lc!() + ark_relations::r1cs::Variable::One, | ||
| ark_relations::lc!() + diff, | ||
| )?; |
There was a problem hiding this comment.
The non-equality constraint implementation is incomplete and non-functional. The comments at lines 86-88 claim to enforce old_root != new_root via (new_root - old_root) * inv = 1, but the second half of this constraint (computing and enforcing the inverse multiplication) was removed.
Currently, the code only computes diff = new_root - old_root and enforces diff = new_root - old_root (lines 90-100), which is a tautology that provides no security value. The critical constraint diff * inv = 1 (which would prove diff != 0) is missing.
This means the circuit will accept proofs where old_root == new_root, allowing state transitions that don't actually change state. This defeats the purpose of the non-equality check mentioned in the PR description's security improvements.
|
@copilot fix up the branch to merge into |
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Fixed in commit 9fcb846. Restored proper rc1 production-hardened code for all 7 conflicting files. The previous merge had incorrectly taken older feature branch versions instead of rc1. Restored features:
Also fixed duplicate imports in battle_circuit.rs. All 100 tests pass and merge into |
|
@copilot Full architectural overview, incldung detailed mermaid diagrams and comparison with the WP spec (attached) - a full audit of the system and how it deviates from the WP spec. |
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Created comprehensive whitepaper audit in The document includes:
Overall assessment: RC1 shows substantial WP compliance, ready for testnet with clear roadmap to mainnet. |
|
@copilot apply changes based on the comments in this thread |
The review issues were already addressed in commit 9fcb846 when I restored the proper rc1 production-hardened code. All 100 tests pass, confirming:
The code now matches the rc1 production-hardened version with the security features intact. |
Summary
Added comprehensive whitepaper audit document (
docs/WHITEPAPER_AUDIT.md) containing:✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.