OP_CODESEPARATOR bug fix

[sirdeggen]

This took about 16 hours to find 😫

Problem

The problem is that when using ARC to broadcast transactions, go-bt interpreter is used. Therefore the tx below is rejected because the signature will not validate. This is caused by the preimage being wrong within the interpreter around OP_CODESEPARATOR. It should slice off the previous parts of the script including the OP_CODESEPARATOR itself.

Transactions like this will broadcast via WoC and the nodes accept it, but ARC thinks it invalid.

0100000001abdbd5873fbda1b08c19d899993301fd44c0aa735064ebb2248260b7adadf795000000006b483045022100e7813394c7a55941c1acf3c7032046c2aa5bf3a506b4ee09e4cb5761c1850f960220154769af29eef81d56d69eba1d7a5ab37eed15beb9eadcd2cb608ff2e09b3147c321035941a219bcd9688318028afeef55183634f010a933de9d8469ff6e702d96c238ffffffff010271000000000000220687623971234575ab76a914fbcf31b659334eeb086693fc3b4005ce29e1c21788ac00000000

ARC responds:

{
	"detail": "Transaction is malformed and cannot be processed",
	"extraInfo": "arc error 461: script execution failed: false stack entry at end of script execution",
	"instance": null,
	"status": 461,
	"title": "Malformed transaction",
	"txid": "d25ba24ceabf3e6a57010a250225b0e5b80b2140d59dd77a8ac87eb63fb8d317",
	"type": "https://arc.bitcoinsv.com/errors/461"
}

Solution

Off by one bug, the subscript does not include the opcode separator itself, so the slice of the script should be from just after the op code separator - not before.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (29189f4) 80.61% compared to head (55d53b3) 80.63%.
Report is 1 commits behind head on master.

❗ Current head 55d53b3 differs from pull request most recent head 9ef4e75. Consider uploading reports for the commit 9ef4e75 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   80.61%   80.63%   +0.01%     
==========================================
  Files          38       38              
  Lines        4112     4115       +3     
==========================================
+ Hits         3315     3318       +3     
  Misses        546      546              
  Partials      251      251              

Files Changed	Coverage Δ
bscript/interpreter/thread.go	94.35% <100.00%> (+0.04%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sirdeggen]

Noticing this block here which conditionally removes the OPCODESEPARATOR afterwards but is conditional on NON-forkID sighashes:

// Remove the signature since there is no way for a signature
// to sign itself.
if !t.hasFlag(scriptflag.EnableSighashForkID) || !shf.Has(sighash.ForkID) {
    subScript = subScript.removeOpcodeByData(fullSigBytes)
    subScript = subScript.removeOpcode(bscript.OpCODESEPARATOR)
}

[sirdeggen]

We find in the ABC documentation:

• If the script contains any OP_CODESEPARATOR, the scriptCode is the script but removing everything up to
  and including the last executed OP_CODESEPARATOR
• before the signature checking opcode being executed [...]
  Notes:

1. Contrary to the original algorithm, this one does not use FindAndDelete to remove the signature from the script.

Therefore I would recommend that we keep the code as is within this PR.