Skip to content

A couple improvements to BIP 54 test vectors#110

Open
darosior wants to merge 4 commits intobitcoin-inquisition:29.xfrom
darosior:2603_inquisition_bip54_tests_improvements
Open

A couple improvements to BIP 54 test vectors#110
darosior wants to merge 4 commits intobitcoin-inquisition:29.xfrom
darosior:2603_inquisition_bip54_tests_improvements

Conversation

@darosior
Copy link
Copy Markdown

Following up on some review feedback from #99 and from bitcoin/bips#2015, this:

  • ensures newline before EOF in generated test vectors;
  • corrects a typo pointed out by Chris Stewart;
  • adds a test case requested by Antoine Riard.

Copy link
Copy Markdown

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

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

ACK 27957d2

I support including a transaction spending Taproot-inputs that (after stripping the witness data) ends up at the invalid 64 bytes.

Comment thread src/test/bip54_tests.cpp
Copy link
Copy Markdown

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Light Code Review ACK fc24db4

Comment thread src/test/bip54_tests.cpp
RecordTestCase(test_vectors, CTransaction{tx_copy}, /*valid=*/false, "A 64-byte Segwit transaction (1 p2tr input, 1 p2a output).");
}

// A semi-realistic 64-byte transaction: 1 Taproot input with an annex, 1 OP_RETURN output.
Copy link
Copy Markdown

@ariard ariard Mar 19, 2026

Choose a reason for hiding this comment

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

maybe a test case for a segwit v2+ to verify the validity is not changed by the novel BIP54 checks.

ofc, no reason why it would change anything for v2 - v16.

don’t remember if there is already another test case for it elsewhere.

Copy link
Copy Markdown

@hodlinator hodlinator left a comment

Choose a reason for hiding this comment

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

re-ACK fc24db4

@darosior
Copy link
Copy Markdown
Author

cc @ajtowns could you have a look here?

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.

3 participants