btcd icon indicating copy to clipboard operation
btcd copied to clipboard

Allow multiple push statement for OP_RETURN

Open dkatzan opened this issue 11 months ago • 4 comments

Currently, when checking if a script is isNullDataScript, it is expliciclty checks for OP_RETURN or OP_RETURN followed by a single push which happens here https://github.com/btcsuite/btcd/blame/master/txscript/standard.go#L498

how ever since bitcoin v.12.0, the restriction of having only a single push was lifted, and there can now be multiple push data, and the limit on the output size is now enforced on the entire script

Would it be possible to add support for this?

dkatzan avatar Jan 19 '25 19:01 dkatzan

Seems ok at a glance.

Are you guys using https://github.com/btcsuite/btcd/blob/09ba026580f4041bae30d732f26fa1a2036af003/txscript/script.go#L103-L10?

kcalvinalvin avatar Jan 20 '25 08:01 kcalvinalvin

Hi @kcalvinalvin , thx for the quick reply

We are, but IIUC, this test should pass

func TestScriptType(t *testing.T) {
	s, err := hex.DecodeString("6a5d0f160100e6a233fc078088a5a9a30700")
	require.NoError(t, err)
	isNull := txscript.IsNullData(s)
	require.True(t, isNull)
}

but it in fact fails I believe the issue is this script start with 6a which is OP_RETURN followed by 5d which is OP_13 and then some data

when reaching here https://github.com/btcsuite/btcd/blob/09ba026580f4041bae30d732f26fa1a2036af003/txscript/standard.go#L523, and calling Next of the ScriptTokenizer, op.length of OP_13 is 1, so t.offset is set to 1 https://github.com/btcsuite/btcd/blob/09ba026580f4041bae30d732f26fa1a2036af003/txscript/tokenizer.go#L79 which ends up returning false on tokenizer.Done here

as a side note, it also contains this check len(tokenizer.Data()) <= MaxDataCarrierSize which I believe should be len(tokenizer.Data()) <= MaxDataCarrierSize + 3

please lmk if I missunderstood something?

dkatzan avatar Jan 21 '25 16:01 dkatzan

@dkatzan if you want to fix this, then you'll need to add a new loop to continue parsing the pushes to validate that they're all minimal, etc. The final statement in that function has tokenizer.Done() which helps to assert that there's only a single push.

Roasbeef avatar Jan 24 '25 00:01 Roasbeef

Hi @Roasbeef and @kcalvinalvin , I made an attempt at fixing the issue https://github.com/btcsuite/btcd/pull/2309

dkatzan avatar Jan 24 '25 16:01 dkatzan