commit 1deaefff4cb49cb6bb7fcb74444a41797011b0b3
parent 8794de1c168bba2e98f3f822d99df3252fb36614
Author: Jared Tobin <jared@jtobin.io>
Date: Sat, 16 May 2026 23:06:46 -0230
sighash: address review feedback on codeseparator strip
- Fast path: skip BS.unpack/pack round-trip when scriptCode contains
no 0xab byte (memchr via BS.elem; the common case for modern
P2PKH/P2SH/segwit scripts)
- Remove redundant length check in push helper: when chunk is shorter
than expected (malformed), splitAt yields rest' = [], and go [] = []
produces the same bytes as the explicit short-circuit
- Mark strip_codeseparators INLINABLE
- Document the out-of-range-idx asymmetry between sighash_legacy
(deterministic but consensus-undefined) and sighash_segwit (Nothing)
Tests:
- Add OP_PUSHDATA2 and OP_PUSHDATA4 unit tests (cover the 0x100 and
0x1000000 multipliers in length decoding)
- Add malformed-tail unit test (locks down the docstring contract)
- resize 500 on prop_strip_codesep_no_0xab_unchanged to exercise
longer scripts than the default Arbitrary sizing produces
Diffstat:
2 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/lib/Bitcoin/Prim/Tx/Sighash.hs b/lib/Bitcoin/Prim/Tx/Sighash.hs
@@ -123,7 +123,9 @@ hash256 = SHA256.hash . SHA256.hash
-- On a malformed script (truncated push data), the malformed tail is
-- copied verbatim without further codeseparator processing.
strip_codeseparators :: BS.ByteString -> BS.ByteString
-strip_codeseparators = BS.pack . go . BS.unpack
+strip_codeseparators !script
+ | not (0xab `BS.elem` script) = script -- fast path: nothing to strip
+ | otherwise = BS.pack (go (BS.unpack script))
where
go :: [Word8] -> [Word8]
go [] = []
@@ -149,13 +151,14 @@ strip_codeseparators = BS.pack . go . BS.unpack
_ -> b : rest
| otherwise = b : go rest
- -- | Copy a push header and N data bytes verbatim. If the script is
- -- truncated, copy whatever's available and stop processing.
+ -- | Copy a push header and N data bytes verbatim. On truncation,
+ -- @splitAt@ yields @(available, [])@ so @go []@ closes the
+ -- recursion naturally; the malformed tail is preserved.
push :: Int -> [Word8] -> [Word8] -> [Word8]
push !len !header !rest =
let (chunk, rest') = splitAt len rest
- in header ++ chunk ++
- if length chunk == len then go rest' else []
+ in header ++ chunk ++ go rest'
+{-# INLINABLE strip_codeseparators #-}
-- legacy sighash -------------------------------------------------------------
@@ -176,6 +179,12 @@ strip_codeseparators = BS.pack . go . BS.unpack
-- For base SIGHASH_SINGLE with input index >= output count, returns
-- the special \"sighash single bug\" value (0x01 followed by 31 zero
-- bytes).
+--
+-- The input index is /not/ validated against the input count; an
+-- out-of-range @idx@ produces a deterministic but
+-- consensus-undefined hash. Matches Bitcoin Core, which @assert@s on
+-- the same precondition. Contrast 'sighash_segwit', which validates
+-- and returns 'Nothing'.
sighash_legacy
:: Tx
-> Int -- ^ input index
diff --git a/test/Main.hs b/test/Main.hs
@@ -14,7 +14,7 @@ import Test.Tasty
import qualified Test.Tasty.HUnit as H
import Test.Tasty.QuickCheck as QC hiding (Witness)
import Test.QuickCheck
- ( Gen, Arbitrary(..), elements, oneof, chooseInt, forAll, (==>) )
+ ( Gen, Arbitrary(..), elements, oneof, chooseInt, forAll, resize, (==>) )
-- main ------------------------------------------------------------------------
@@ -66,6 +66,9 @@ main = defaultMain $
, codesep_strip_simple
, codesep_inside_push
, codesep_inside_pushdata1
+ , codesep_inside_pushdata2
+ , codesep_inside_pushdata4
+ , codesep_malformed_tail
]
, testGroup "Bitcoin Core sighash.json" [
bc_sighash_1
@@ -1081,6 +1084,32 @@ codesep_inside_pushdata1 =
expected = BS.pack [0x4c, 0x03, 0xab, 0xab, 0xab, 0x51]
in H.assertEqual "" expected (strip_codeseparators input)
+-- OP_PUSHDATA2 with 2 bytes of 0xab data (LE length = 0x0002), then a
+-- lone 0xab opcode and OP_1. Exercises the n0 + n1 * 0x100 arithmetic.
+codesep_inside_pushdata2 :: TestTree
+codesep_inside_pushdata2 =
+ H.testCase "OP_PUSHDATA2 data preserved, trailing 0xab stripped" $
+ let input = BS.pack [0x4d, 0x02, 0x00, 0xab, 0xab, 0xab, 0x51]
+ expected = BS.pack [0x4d, 0x02, 0x00, 0xab, 0xab, 0x51]
+ in H.assertEqual "" expected (strip_codeseparators input)
+
+-- OP_PUSHDATA4 with 1 byte of 0xab data (LE length = 0x00000001), then
+-- a lone 0xab opcode. Exercises the 4-byte LE length decode.
+codesep_inside_pushdata4 :: TestTree
+codesep_inside_pushdata4 =
+ H.testCase "OP_PUSHDATA4 data preserved, trailing 0xab stripped" $
+ let input = BS.pack [0x4e, 0x01, 0x00, 0x00, 0x00, 0xab, 0xab, 0x51]
+ expected = BS.pack [0x4e, 0x01, 0x00, 0x00, 0x00, 0xab, 0x51]
+ in H.assertEqual "" expected (strip_codeseparators input)
+
+-- Malformed tail: OP_PUSHDATA2 with a truncated length header (only
+-- one byte available). Per docstring, copied verbatim.
+codesep_malformed_tail :: TestTree
+codesep_malformed_tail =
+ H.testCase "malformed tail copied verbatim" $
+ let input = BS.pack [0x4d, 0x00]
+ in H.assertEqual "" input (strip_codeseparators input)
+
-- | Arbitrary ByteString generator (QuickCheck has no built-in instance).
genByteString :: Gen BS.ByteString
genByteString = BS.pack <$> arbitrary
@@ -1095,6 +1124,6 @@ prop_strip_codesep_idempotent =
prop_strip_codesep_no_0xab_unchanged :: TestTree
prop_strip_codesep_no_0xab_unchanged =
QC.testProperty "strip_codeseparators is no-op without 0xab bytes" $
- forAll (BS.pack . filter (/= 0xab) <$> arbitrary) $ \s ->
+ forAll (resize 500 $ BS.pack . filter (/= 0xab) <$> arbitrary) $ \s ->
strip_codeseparators s === s
