Skip to content

Refactor and expand script verification tests#13

Open
stringintech wants to merge 1 commit intomainfrom
script-verify-tests
Open

Refactor and expand script verification tests#13
stringintech wants to merge 1 commit intomainfrom
script-verify-tests

Conversation

@stringintech
Copy link
Owner

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.

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.
@stringintech
Copy link
Owner Author

Tested pre-release v0.0.3-alpha.4 in stringintech/go-bitcoinkernel#12 (comment).

Copy link
Contributor

@janb84 janb84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
			}
		})
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants