fix(lite-backend): never hang or panic a sync on malformed server data
The lightwalletd server controls the compact ciphertext length, the block, prev-block and tx hash lengths, and (through the chain) the full txs' epks. Each of these could wedge or poison a sync: - try_sapling_compact_note_decryption asserts a 52-byte ciphertext. A shorter (or full 580-byte) one panicked a pool job in scan_output_internal while the original sender stayed alive, so the collector waited forever with sync_lock, txs.write() and a wallet read guard held. - scan_taddress_txids kept its own sender alive too: one panicking t-address job (scan_full_tx) left crx.iter().take(n) blocked forever. - CompactBlock::hash()/prev_hash() assert 32 bytes when no header is sent, and every vtx hash is copied into a [u8; 32], one of them under txs.write(): a 31-byte tx hash poisoned the wallet for good, and a bad hash in the block's txid list panicked after the witnesses had moved. - scan_full_tx and scan_full_mempool_tx unwrapped as_prime_order() on the epk. Consensus rejects only a small-order epk, so a mixed-order one can be mined: an own tx then failed every sync, and the mempool receiver thread died silently. Changes: - append_leaf(node: Node, ..) commits a leaf to the existing, block and new witnesses and the tree; the epk-error path, the new short-ciphertext path and the normal path all use it (a behaviour-neutral extraction). - A compact ciphertext under 52 bytes is skipped with its cmu still committed; a longer one is truncated to 52 bytes, which is exact (the compact plaintext is those bytes and the cmu re-check validates it). - Each decryption job catches a panic and sends exactly one message; drop(tx) after the dispatch, and the collector logs a vanished or panicked job and treats the output as not ours. - scan_taddress_txids runs through the private seam scan_taddress_txids_with(job): catch_unwind per job, drop(ctx), and a count check, so a dead job is an error, never a hang or a silent partial success. - scan_block_with_pool checks the block, prev-block and vtx hash lengths right after the parse and returns Err(-1), as for an unparsable block, before any change (Err(height) would refetch the same block forever under sync_lock). - scan_full_tx and scan_full_mempool_tx skip an output whose epk is not in the prime-order subgroup (one check per output, used by every decryption and the outgoing recovery). - The mempool receiver thread catches a panic of scan_full_mempool_tx. Flagged behaviour changes: - a compact ciphertext under 52 bytes is skipped (its cmu is still committed); one over 52 bytes is truncated; - pool-job panics are reported (logged; the output is treated as not ours, which a rescan repairs) instead of hanging the sync; - bad hash lengths return Err(-1) before any change; - outputs with a non-prime-order epk are skipped; - the mempool thread survives a bad tx. Wallet format v9 is unchanged; no ABI or symbol change. Honest data takes the same path as before (an old-vs-new run over generated honest chains is byte-identical). Tests (each on its own thread under a 15 s watchdog): 0/51/52/53/580-byte ciphertexts in one block on the inline and deferred witness paths (the 51-byte notes are skipped, the 53- and 580-byte notes are found, the tree and every witness match the reference); a 31-byte tx hash, an empty block hash, a 31-byte prev hash and a 33-byte foreign tx hash each return Err(-1) with the txs lock usable and last_scanned_height, trees and witnesses unchanged; a mixed-order epk in scan_full_tx and scan_full_mempool_tx does not panic and decrypts nothing while the real epk still decrypts; a t-address job panicking for 1 of 3 addresses is an Err. The oracle generator gains short (0..=51-byte) and full (580-byte) ciphertext outputs without an extra random draw, so each seed keeps its sequence of blocks, values and keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+82
-23
@@ -1171,7 +1171,13 @@ pub fn start_mempool_monitor(lc: Arc<LightClient>) -> Result<(), String> {
|
||||
&rtx.data[..])
|
||||
{
|
||||
let light_wallet_clone = lc.wallet.clone();
|
||||
light_wallet_clone.read().unwrap().scan_full_mempool_tx(&tx, rtx.height as i32, 0, true);
|
||||
// One bad mempool tx must not end this thread: the mempool monitor would stop silently.
|
||||
let scanned = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
|
||||
light_wallet_clone.read().unwrap().scan_full_mempool_tx(&tx, rtx.height as i32, 0, true);
|
||||
}));
|
||||
if scanned.is_err() {
|
||||
error!("Scanning mempool tx {} panicked; skipped it", tx.txid());
|
||||
}
|
||||
}
|
||||
}
|
||||
});
|
||||
@@ -1631,26 +1637,14 @@ pub fn start_mempool_monitor(lc: Arc<LightClient>) -> Result<(), String> {
|
||||
end_height: u64,
|
||||
no_cert: bool
|
||||
) -> Result<Vec<()>, String> {
|
||||
let addresses = self.wallet.read()
|
||||
.map_err(|e| format!("Failed to read wallet: {:?}", e))?
|
||||
.get_all_taddresses().iter()
|
||||
.cloned()
|
||||
.collect::<Vec<String>>();
|
||||
|
||||
let (ctx, crx) = channel();
|
||||
let num_addresses = addresses.len();
|
||||
|
||||
for address in addresses {
|
||||
let address_clone = address.clone();
|
||||
// The per-address job: fetch the address's txids and full-scan each tx (scan_full_tx).
|
||||
self.scan_taddress_txids_with(pool, {
|
||||
let wallet = self.wallet.clone();
|
||||
let pool = pool.clone();
|
||||
let server_uri = self.get_server_uri();
|
||||
let ctx = ctx.clone();
|
||||
let block_times = block_times.clone();
|
||||
|
||||
pool.execute(move || {
|
||||
|
||||
let r = fetch_transparent_txids(
|
||||
move |address: String| -> Result<(), String> {
|
||||
let wallet = wallet.clone();
|
||||
let block_times = block_times.clone();
|
||||
fetch_transparent_txids(
|
||||
&server_uri,
|
||||
address,
|
||||
start_height,
|
||||
@@ -1687,16 +1681,49 @@ pub fn start_mempool_monitor(lc: Arc<LightClient>) -> Result<(), String> {
|
||||
},
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// The t-address collector; `job` is the per-address work (a private seam, so a test can make a job panic). Every
|
||||
// job sends exactly one result, a panicking one included, and only the jobs hold senders: the collector can
|
||||
// neither wait forever on a job that died (with sync_lock held) nor report a partial scan as success.
|
||||
fn scan_taddress_txids_with<F>(&self, pool: &ThreadPool, job: F) -> Result<Vec<()>, String>
|
||||
where F: Fn(String) -> Result<(), String> + Send + Sync + 'static
|
||||
{
|
||||
let addresses = self.wallet.read()
|
||||
.map_err(|e| format!("Failed to read wallet: {:?}", e))?
|
||||
.get_all_taddresses().iter()
|
||||
.cloned()
|
||||
.collect::<Vec<String>>();
|
||||
|
||||
let job = Arc::new(job);
|
||||
let (ctx, crx) = channel();
|
||||
let num_addresses = addresses.len();
|
||||
|
||||
for address in addresses {
|
||||
let address_clone = address.clone();
|
||||
let ctx = ctx.clone();
|
||||
let job = job.clone();
|
||||
|
||||
pool.execute(move || {
|
||||
let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (*job)(address)))
|
||||
.unwrap_or_else(|_| Err("t-address scan job panicked".to_string()));
|
||||
match ctx.send(r) {
|
||||
Ok(_) => info!("Successfully sent data for address: {}", address_clone),
|
||||
Err(e) => error!("Failed to send data for address: {}: {:?}", address_clone, e),
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
crx.iter().take(num_addresses).collect()
|
||||
drop(ctx);
|
||||
|
||||
// Ends once every job has sent its result and dropped its sender.
|
||||
let results: Vec<Result<(), String>> = crx.iter().collect();
|
||||
if results.len() != num_addresses {
|
||||
return Err(format!("t-address scan: {} of {} jobs reported", results.len(), num_addresses));
|
||||
}
|
||||
results.into_iter().collect()
|
||||
}
|
||||
|
||||
|
||||
@@ -2088,4 +2115,36 @@ pub fn test_bad_import() {
|
||||
assert_eq!(lc.do_seed_phrase().unwrap()["seed"], TEST_SEED.to_string());
|
||||
}
|
||||
|
||||
// C245 Fix 2 (s2-D6a seam). A t-address job that panicked left the collector waiting forever, with sync_lock
|
||||
// held, for a result that never came: the original sender stayed alive. Each job now reports exactly once (a
|
||||
// panic is an Err) and the collector counts the reports. Runs under the 15 s watchdog.
|
||||
#[test]
|
||||
pub fn test_taddress_scan_job_panic_is_error() {
|
||||
let (n, all_ok, one_err, one_panic) = crate::lightwallet::tests::within_watchdog("t-address scan", || {
|
||||
let tmp = TempDir::new("lctest").unwrap();
|
||||
let lc = LightClient::new(&main_config(&tmp), 0).unwrap();
|
||||
lc.do_new_address("R").unwrap();
|
||||
lc.do_new_address("R").unwrap();
|
||||
let addrs = lc.wallet.read().unwrap().get_all_taddresses();
|
||||
let pool = threadpool::ThreadPool::new(2);
|
||||
let all_ok = lc.scan_taddress_txids_with(&pool, |_a: String| -> Result<(), String> { Ok(()) });
|
||||
let a1 = addrs[1].clone();
|
||||
let one_err = lc.scan_taddress_txids_with(&pool, move |a: String| -> Result<(), String> {
|
||||
if a == a1 { Err("injected error".to_string()) } else { Ok(()) }
|
||||
});
|
||||
let a1 = addrs[1].clone();
|
||||
let one_panic = lc.scan_taddress_txids_with(&pool, move |a: String| -> Result<(), String> {
|
||||
if a == a1 {
|
||||
panic!("injected t-address job panic");
|
||||
}
|
||||
Ok(())
|
||||
});
|
||||
(addrs.len(), all_ok, one_err, one_panic)
|
||||
});
|
||||
assert_eq!(n, 3);
|
||||
assert_eq!(all_ok, Ok(vec![(); 3]));
|
||||
assert_eq!(one_err, Err("injected error".to_string()));
|
||||
assert_eq!(one_panic, Err("t-address scan job panicked".to_string()));
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user