Page MenuHomePhabricator
Differential D14818

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

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman created this revision.Nov 18 2023, 14:49
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 18 2023, 14:49
bytesofman requested review of this revision.Nov 18 2023, 14:49
bytesofman updated this revision to Diff 43166.

remove debug logging

Harbormaster completed remote builds in B25672: Diff 43165.Nov 18 2023, 14:52
bytesofman planned changes to this revision.Nov 18 2023, 14:53

this fixes the bug but is not the right implementation

Harbormaster completed remote builds in B25673: Diff 43166.Nov 18 2023, 14:53
bytesofman updated this revision to Diff 43167.Nov 18 2023, 14:57

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

bytesofman added inline comments.Nov 18 2023, 14:59
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).

Harbormaster completed remote builds in B25674: Diff 43167.Nov 18 2023, 14:59
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
bytesofman updated this revision to Diff 43168.Nov 18 2023, 17:51

add test for parsing coinbase tx

bytesofman added a comment.Nov 18 2023, 17:52

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).

Harbormaster completed remote builds in B25675: Diff 43168.Nov 18 2023, 17:54
Fabien added a comment.EditedNov 19 2023, 19:34
In D14818#332833, @bytesofman wrote:

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

Fabien accepted this revision.Nov 19 2023, 19:35
This revision is now accepted and ready to land.Nov 19 2023, 19:35
Closed by commit rABCde6232f1fc45: [Cashtab] Do not try to parse outputscript of coinbase txs (authored by bytesofman). · Explain WhyNov 19 2023, 21:05
This revision was automatically updated to reflect the committed changes.
bytesofman added a commit: rABCde6232f1fc45: [Cashtab] Do not try to parse outputscript of coinbase txs.

Revision Contents
Changeset List

PathSize
cashtab/
src/
utils/
__mocks__/
58 lines
__tests__/
19 lines
102 lines

Diff 43172

View Options

cashtab/src/utils/__mocks__/chronikTxHistory.js

Loading...
View Options

cashtab/src/utils/__tests__/chronik.test.js

Loading...
View Options

cashtab/src/utils/chronik.js

Loading...