Page MenuHomePhabricator

partial revert of tx decode sanity check backport
ClosedPublic

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

Details

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

Event Timeline

src/core_read.cpp
188 ↗(On Diff #14689)

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.
This revision is now accepted and ready to land.Dec 8 2019, 15:42

Adding tests would be good or you might find this reverting back to a behavior you don't like.