Feat/hw fixes on x - #12911
Conversation
…rify inputs
A refTx exists so the device can recompute a previous transaction's txid and
only then trust the input amount it shows in the fee — every field feeds that
hash, so a wrong one is a hard reject.
We read previous transactions from /transactions/search, which has no keys for
sequence, scriptPublicKey.version, lockTime or gas; those four were passed to
the device as 0. Confirmed the keys are absent rather than omitted when zero:
a transaction whose sequence is 2^64-1 still comes back without the field.
/blocks/{hash} carries all of them but has no txid index, so resolve
txid -> block via search and read the transaction out of its block. Both hops
are batched — search takes every txid at once and the block reads ride in one
proxy request — so a 90-input transaction costs two round trips, not 91, and
measures ~7s end to end. They share one 30s deadline rather than each holding
their own, keeping the worst case bounded.
Verified against four public kaspad nodes: rebuilding a refTx from the block
endpoint reproduces the node's own view field for field, on v0 and v1 alike.
The mapping moves to refTxUtils so its numeric edges can be tested directly.
An upstream that drops zero-valued numbers means an empty sequence, lockTime,
gas or script version has to read as 0, and each substitution is logged —
otherwise a field that should have carried a value leaves nothing behind but a
device rejection. An empty amount is not defaulted: there, empty is far more
likely to be a value that could not be represented. Anything past 2^53 was
already rounded by the JSON parse and cannot be recovered, so it bails to blind
signing instead.
Also corrects the v0 gate's comment. The REST API does not return bad fields
for v1 transactions — four nodes report the same subnetwork id, and it does not
mirror the txid prefix. The gate stays because recomputing a txid off the
fields KaspaTxAckPrev* carries (blake2b-256 keyed "TransactionID") reproduces a
v0 id exactly but never a v1 one.
|
@codex review |
|
@codex security review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd5f3f110d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| return '0'; | ||
| } | ||
| const parsed = new BigNumber(String(value)); |
There was a problem hiding this comment.
当节点将大整数正确编码为字符串时（例如 amount: "9007199254740992"），BigNumber 并未发生精度丢失，但这里仍按 Number.MAX_SAFE_INTEGER 拒绝该值；KeyringHardware 随后会捕获异常并将整个签名降级为无 refTxs 的盲签。应仅对 number 输入应用 JS 安全整数限制，并对字符串按真正的 uint64 范围校验，否则大额 UTXO 恰好无法获得设备端输入验证。
Useful? React with 👍 / 👎.
| if (decision === 'cancel') { | ||
| lastError = PAIR_CANCELLED_REASON; |
There was a problem hiding this comment.
在 Windows Desktop（Electron main 为单 JS runtime，WinRT 配对状态由独立 helper 子进程持有）中，用户取消时这里先写入可映射为 BlePairingCancelled 的原因，但 helper 随后以 pairing failed with status ... 发出 error 事件，runHelper 的现有错误处理会再次覆盖 lastError；在该已刷新事件先于进程退出到达的正常路径中，调用方最终收到通用配对失败而不是取消错误。应在已显式取消时忽略后续 helper 错误，或让 helper 返回同一个取消原因。
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
| inputs: (tx.inputs ?? []).map((input) => ({ | ||
| prevTxId: input.previousOutpoint.transactionId, | ||
| outputIndex: Number(input.previousOutpoint.index ?? 0), | ||
| sequenceNumber: uint64(input.sequence, 'input.sequence', true), |
There was a problem hiding this comment.
当待花费 UTXO 的创建交易含有非默认 sigOpCount（例如 P2SH/多签输入）时，区块响应已经在 input.sigOpCount 中提供该值，但这里构造 refTx 时完全丢弃了它；该字段参与 Kaspa v0 交易 ID，设备按默认值重算出的 ID 因而与 txId 不符并硬拒绝签名，而调用方也不会在这次设备拒绝后重试盲签。若当前硬件协议不能传输该字段，应像 v1 gate 一样在发现非默认值时提前放弃整组 refTx。
Useful? React with 👍 / 👎.
| /// Backstop so a parent that never answers cannot wedge the helper; the device | ||
| /// gives up on its own pairing window long before this. | ||
| #[cfg(windows)] | ||
| const DECISION_TIMEOUT_MS: u64 = 120_000; |
There was a problem hiding this comment.
在 Windows Desktop 的无人应答路径中，Electron main 会在 60 秒时直接 child.kill()，但持有 WinRT pairing deferral 的 helper 要到 120 秒才把沉默视为取消，因此这个 native backstop 永远无法完成 deferral 并向设备发送 Pairing Failed；例如 PIN 对话框创建失败或一直未响应时，最终仍是强制断链，正是本次修改要避免的半配对状态。应让 helper 的拒绝期限短于父进程期限，或让父进程先写入 cancel 并留出退出宽限期。
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cancelling BLE pairing shows a confusing hardware failure instead of a cancellation
Reported at apps/desktop/app/process/BlePair.ts:156-158 (outside the diff of this PR, so this is a file-level comment)
Severity: non-severe
The user-cancelled reason recorded for the pairing attempt is immediately replaced by the helper's own generic failure text (lastError = event.message at apps/desktop/app/process/BlePair.ts:157) before the attempt finishes, so a deliberate cancel is reported as an unexplained pairing failure.
Impact: A user who presses Cancel on the pairing-code dialog (or cancels from the app) sees a scary low-level Bluetooth failure message rather than a clean "cancelled" result.
How the cancel reason is overwritten before the promise rejects
When the decision callback receives cancel it sets lastError = PAIR_CANCELLED_REASON (apps/desktop/app/process/BlePair.ts:109-111), whose whole purpose is to match the SDK's /connect cancelled/i test so it maps to BlePairingCancelled (see comment at apps/desktop/app/process/BlePair.ts:23-24).
The helper then declines the ceremony, PairAsync returns a non-Paired status, and run_pair returns Err(format!("pairing failed with status {status:?}")) (apps/desktop/native-modules/onekey-ble-pair/src/main.rs:787). main turns that into {"type":"error","message":"pairing failed with status DevicePairingResultStatus(N)"} on stdout, which the stdout parser assigns to lastError, clobbering the cancel reason. The exit handler (apps/desktop/app/process/BlePair.ts:175-190) then rejects with the clobbered message.
A cancelRequested flag that makes the cancel reason win (or that skips overwriting lastError once a cancel has been sent) restores the intended mapping.
Automated code review found blocking issuesReviewed commit Review summaryThis PR changes two hardware-wallet trust paths. On Windows desktop, Electron main and a per-attempt WinRT helper now keep numeric-comparison pairing pending until an explicit confirmation or cancellation; in Kaspa background signing, previous transaction IDs are resolved through the search and block endpoints so complete reference transactions can be streamed to the device. The refTx mapping is now isolated and tested, with local diagnostics and a blind-signing fallback, but two edge paths still bypass the intended safety behavior. What needs attention: Confirm that every pairing termination, including timeout, reaches the WinRT refusal path and that lossless uint64 strings remain eligible for Kaspa refTx verification. Issues to address
Validation gaps
|
|
@cursoragent review |
|
Taking a look!   |
|
|
||
| return { | ||
| txId, | ||
| version: tx.version, |
There was a problem hiding this comment.
🟠 P1 tx.version 为 0 时会被省略
问题
/blocks/{blockId}?includeTransactions=true 的响应来自 kaspad gRPC 的 protobuf → JSON 转换，proto3 默认值字段整体不落键（响应用的正是 previousOutpoint / subnetworkId / verboseData 这套 camelCase 命名，符合 MessageToDict 的行为）。这也正是本 PR 对 sequence、lockTime、gas、scriptPublicKey.version、previousOutpoint.index、payload 都做了缺失兜底的原因。
但 IKaspaBlockTransaction.version 被声明成必填 number，这里也直接透传 version: tx.version，是唯一没有兜底的数值字段。
影响
v0 交易的 version 恰好等于 0，响应里不会带这个键，于是 tx.version === undefined。紧接着 Vault.collectRefTxsByApi 的门禁 refTxs.some((tx) => tx.version !== 0) 恒为真并抛出 unsupported non-v0 prev tx，KeyringHardware 捕获后置 refTxs = undefined。
结果是本 PR 想启用的设备端输入校验，对 100% 的 v0 前序交易都会静默退化成盲签，仅在本地日志留下一条 refTxFetchFailed——而 v0 正是这条链路唯一支持的版本。
建议
把 version 也按「缺失即 0」读取，并同步放宽类型：
// clientRestApi.ts
version?: number | string | null;
// refTxUtils.ts
version: Number(uint64(tx.version, 'version', true)),同时建议补一条「version 缺失时按 v0 处理并通过门禁」的用例，现有测试的 blockTx() 始终显式写了 version: 0，覆盖不到这个真实响应形态。
Generated by Claude Code
| if (!nullMeansZero) { | ||
| throw new OneKeyLocalError(`kaspa refTx: ${field} missing for ${txId}`); | ||
| } | ||
| defaultLogger.transaction.send.refTxFieldDefaulted({ |
There was a problem hiding this comment.
🟡 P2 零值兜底日志会淹没自身信号
问题
零值字段缺失在这条链路上是常态而非异常：几乎每个 input 的 sequence、每笔 tx 的 lockTime 与 gas、每个 output 的 scriptPublicKey.version 实际值都是 0，因而都会走进 nullMeansZero 分支，各写一条 refTxFieldDefaulted 本地日志。
影响
一笔 90 输入的交易要拉最多 90 笔前序交易，每笔按 1~2 个 input、2 个 output 估算约 6 条，单次硬件签名就会写入约 540 条本地日志。send.ts 里这条日志的注释说明它的用途是「某个本该有值的字段被读成 0 时可追溯」，但它在正常路径上高频触发，真正异常的那一条会被完全淹没，同时挤占本地日志容量。
建议
改成按交易聚合一条，把被兜底的字段名与次数一起带出：
// buildKaspaRefTx 内收集，末尾一次性上报
if (defaultedFields.length) {
defaultLogger.transaction.send.refTxFieldDefaulted({
network: networkId,
txId,
field: defaultedFields.join(','),
});
}或者仅对「零值不属于预期」的字段记录日志，sequence / lockTime / gas / scriptVersion 这类常态零值直接静默返回 '0'。
Generated by Claude Code
3 participants
No description provided.