partial revert of tx decode sanity check backport
ClosedPublic
Actions

Authored by markblundeberg on Dec 8 2019, 05:00.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGface53f7c55e: partial revert of tx decode sanity check backport
rABCface53f7c55e: partial revert of tx decode sanity check backport

Summary

(partial revert of D4247)

This Diff had the unfortunate side effect of rendering real
on-chain transactions to be un-decodeable in decoderawtransaction. As
seen in the original PR, the rationale for this loss of functionality was
that the segwit transaction format is ambiguous with 0-input transactions
used during transaction construction.

In the case of BCH, we don't need to damage our RPC for the sake of segwit.

Test Plan

make check
test_runner.py

try decoding mainnet tx ebc9fa1196a59e192352d76c0f6e73167046b9d37b8302b6bb6968dfd279b767 and ensure it does not fail (has truncated scripts):

bitcoin-cli decoderawtransaction 01000000019ac03d5ae6a875d970128ef9086cef276a1919684a6988023cc7254691d97e6d010000006b4830450221009d41dc793ba24e65f571473d40b299b6459087cea1509f0d381740b1ac863cb6022039c425906fcaf51b2b84d8092569fb3213de43abaff2180e2a799d4fcb4dd0aa012102d5ede09a8ae667d0f855ef90325e27f6ce35bbe60a1e6e87af7f5b3c652140fdffffffff080100000000000000010101000000000000000202010100000000000000014c0100000000000000034c02010100000000000000014d0100000000000000044dffff010100000000000000014e0100000000000000064effffffff0100000000

also try decoding mainnet tx f6e637d3469a552174b80bd10972d137a4ce4be519910e455bde91339514dc18 (very long scriptpubkey, too long to paste here):

bitcoin-cli decoderawtransaction `bitcoin-cli getrawtransaction f6e637d3469a552174b80bd10972d137a4ce4be519910e455bde91339514dc18`
`

Diff Detail

Repository

rABC Bitcoin ABC

Branch

revit

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8451
Build 14913: Default Diff Build & Tests
Build 14912: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Dec 8 2019, 05:00

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 8 2019, 05:00

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg edited the test plan for this revision. (Show Details)Dec 8 2019, 05:01

markblundeberg edited the test plan for this revision. (Show Details)Dec 8 2019, 05:04

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8451: Diff 14689.Dec 8 2019, 05:11

markblundeberg added inline comments.Dec 8 2019, 05:21

src/core_read.cpp
188	In principle we could keep these input checks, since no valid transaction violates these. However, it is nice to be able to decode transactions with intentionally weird scriptSigs even if they will never be broadcasted. Just the other day, I wanted to decode a transaction someone made with a >520 byte push. This was a mistake by them of course but not being able to decode the tx was frustrating for my analysis, and I had to use another tx decoder. A wallet such as Electrum / Electron Cash will stuff weird shit in the scriptSig for its partially signed tx format. Just because the scriptSig contains weird stuff, doesn't mean we shouldn't let a correctly formatted tx be decoded at all. If there is some worry about DoS against the RPC, then just checking the inputs (not outputs) is leaving a hole. A better approach would be to instead check the total tx size against the 1 MB consensus limit, before decoding.