Refactor and expand script verification tests#13
Open
stringintech wants to merge 1 commit intomainfrom
Open
Conversation
Refactor script verification tests to use stateful object references (transaction, script pubkey, transaction output, precomputed tx data) instead of inline hex params. Expand coverage from a single file to 11 dedicated files, one per output type (P2PKH, P2SH multisig, CLTV, CSV, P2SH-P2WPKH, P2SH-P2WSH, P2WPKH, P2WSH, P2TR keypath, P2TR scriptpath), each testing valid, corrupted, and flag-sensitivity variants. Adapt dependency tracker to detect refs nested inside array-valued params. Document the new/changed methods in handler-spec.md.
Owner
Author
|
Tested pre-release |
janb84
reviewed
Mar 2, 2026
Contributor
janb84
left a comment
There was a problem hiding this comment.
ACK 66e1f3c
This was straightforward to implement and the extension of the test is welcome :)
2 suggestions:
(not related to the current changes) In the makefile L21, add build so that for a test no separate build command has to be given but only a make test will suffice :
Details
From:
go build -o $(MOCK_HANDLER_BIN) ./cmd/mock-handler
test:
@echo "Running runner unit tests..."To
go build -o $(MOCK_HANDLER_BIN) ./cmd/mock-handler
test: build
@echo "Running runner unit tests..."other nit see comment
Contributor
There was a problem hiding this comment.
NIT: suggesting to add test that shows that deep refs are not extracted, as per current contract
func TestExtractRefsFromParams(t *testing.T) {
tests := []struct {
description string
params string
wantRefs []string
}{
{
description: "direct ref at top level",
params: `{"input": {"ref": "$ref_a"}}`,
wantRefs: []string{"$ref_a"},
},
{
description: "refs inside array param",
params: `{"items": [{"ref": "$ref_a"}, {"ref": "$ref_b"}]}`,
wantRefs: []string{"$ref_a", "$ref_b"},
},
{
description: "deeply nested ref inside array element is not extracted",
params: `{"items": [{"nested": {"ref": "$ref_a"}}]}`,
wantRefs: nil,
},
{
description: "deeply nested ref in array is not extracted but direct ref alongside it is",
params: `{"direct": {"ref": "$ref_a"}, "items": [{"nested": {"ref": "$ref_b"}}]}`,
wantRefs: []string{"$ref_a"},
},
{
description: "non-ref values in array are ignored",
params: `{"items": [{"ref": "$ref_a"}, "plain_string", 42]}`,
wantRefs: []string{"$ref_a"},
},
{
description: "mixed direct and array refs",
params: `{"direct": {"ref": "$ref_a"}, "items": [{"ref": "$ref_b"}]}`,
wantRefs: []string{"$ref_a", "$ref_b"},
},
}
for _, tt := range tests {
t.Run(tt.description, func(t *testing.T) {
got := extractRefsFromParams([]byte(tt.params))
slices.Sort(got)
wantSorted := slices.Clone(tt.wantRefs)
slices.Sort(wantSorted)
if !slices.Equal(got, wantSorted) {
t.Errorf("extractRefsFromParams(%s) = %v, want %v", tt.params, got, tt.wantRefs)
}
})
}
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors script verification tests to use stateful object references (transaction, script pubkey, transaction output, precomputed tx data) instead of inline hex params.
Expands coverage from a single file to 11 dedicated files, one per output type (P2PKH, P2SH multisig, CLTV, CSV, P2SH-P2WPKH, P2SH-P2WSH, P2WPKH, P2WSH, P2TR keypath, P2TR scriptpath), each testing valid, corrupted, and flag-sensitivity variants (based on how I expanded the tests in sedited/rust-bitcoinkernel#137).
Adapts the dependency tracker to detect refs nested inside array-valued params, and documents the new/changed methods in
handler-spec.md.