Page MenuHomePhabricator

[Cashtab] Do not try to parse outputscript of coinbase txs
ClosedPublic

Authored by bytesofman on Nov 18 2023, 14:49.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCde6232f1fc45: [Cashtab] Do not try to parse outputscript of coinbase txs
Summary

Patch bug in Cashtab tx history parsing causing an error to be thrown in wallets with coinbase txs

Note: there is a lot more we can do for these txs, i.e. rendering them as such. This diff is minimal as the issue currently causes Cashtab wallets with any coinbase tx in the last 10 txs to be unusable.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
alita-debug
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25672
Build 50924: Build Diffcashtab-tests
Build 50923: arc lint + arc unit

Event Timeline

bytesofman updated this revision to Diff 43166.

remove debug logging

this fixes the bug but is not the right implementation

If you have a coinbase tx, dont iterate through inputs at all

cashtab/src/utils/chronik.js
552 ↗(On Diff #43167)

we don't get to this loop anymore with coinbase txs. It already has error handling so is robust against thisInput.outputScript being undefined.

568 ↗(On Diff #43167)

we have no error handling here for the case of thisInput.outputScript being undefined and causing thisInput.outputScript.includes(thisWalletHash160) to fail.

Add gated type checking to prevent cashtab crashing in the event of somehow getting here (should never happen but still good practice).

Fabien requested changes to this revision.Nov 18 2023, 15:53
Fabien added a subscriber: Fabien.

There is no test for the change.

Regarding the implementation, the problem is that the function does too many things. You should split it into logical pieces so you can bail early for coinbase txs without adding another nesting level that makes the code harder to follow.

This revision now requires changes to proceed.Nov 18 2023, 15:53

add test for parsing coinbase tx

the problem is that the function does too many things. You should split it into logical pieces so you can bail early for coinbase txs without adding another nesting level that makes the code harder to follow.

similar to the tx functions, there is a lot of technical debt in parseChronikTx and it should be rewritten separately from this patch (which is needed to fix a critical issue for wallets with staking rewards).

the problem is that the function does too many things. You should split it into logical pieces so you can bail early for coinbase txs without adding another nesting level that makes the code harder to follow.

similar to the tx functions, there is a lot of technical debt in parseChronikTx and it should be rewritten separately from this patch (which is needed to fix a critical issue for wallets with staking rewards).

Yes the comment is not for requesting this diff to fix it all

This revision is now accepted and ready to land.Nov 19 2023, 19:35