Skip to content

Improve witness unit tests#3409

Merged
apoelstra merged 5 commits intorust-bitcoin:masterfrom
tcharding:09-24-witness-refactor-tests
Sep 30, 2024
Merged

Improve witness unit tests#3409
apoelstra merged 5 commits intorust-bitcoin:masterfrom
tcharding:09-24-witness-refactor-tests

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Sep 25, 2024

In preparation for moving the Witness type over to primitives refactor and improve all the unit tests that will be moved, do not touch the ones that will stay behind.

The first five patches are from #3406, the last is just a re-name of the test function I tried to refactor in ac6fe3a

@tcharding
Improve witness unit tests for single empty element
fe96727 
The current unit test is incorrect, the indices field of a witness with
a single element starts at 1 because 0 is encode as a single
byte (compact encoded integer).

Fix the debug test and add a test that pushes an empty slice.
@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Sep 25, 2024
@tcharding tcharding force-pushed the 09-24-witness-refactor-tests branch from 6d48eaa to 42a962e Compare September 25, 2024 01:56
@tcharding tcharding mentioned this pull request Sep 25, 2024
Merged
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Sep 25, 2024

I think the CI failure is a bug that the new tests expose, though I can't tell what exactly it is.

It may be related to our use of to_ne_bytes and from_ne_bytes, given that it triggers only on the big-endian test system. But in reading the code I can't tell exactly what's wrong here.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Sep 25, 2024

code review ACK 42a962ee082778b781154885bd04796642e726de but we need to address this failure before we can merge this.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 25, 2024 •
edited
Loading

Mad, this might be a bug. In Encodable for Witness we are use to_ne_bytes (via encode_cursor), surely this is incorrect, a consensus serialization should be able to be passed between machines without relying on the endianness of the machine, right? Funny that I noticed to_ne_bytes in the unit tests but not in the codebase.

EDIT: I have a nagging feeling that we have discussed this before in the past.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 25, 2024

Related comment you made (before my time): https://github.com/rust-bitcoin/rust-bitcoin/pull/627/files#r660852407

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Sep 25, 2024

@tcharding we don't ever use that in the consensus encoding though. The consensus encoding uses varints. AFAICT we symmetrically use encode_cursor and decode_cursor, which in turn use to_ne_bytes and from_ne_bytes. So the endianness-specificness shouldn't be visible.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Sep 25, 2024

I don't think the comment you linked is related to this. That was about using swap_bytes to go between BE and LE. This is about using ne_bytes to "cast" 4 bytes to a u32, in a way that shouldn't be visible to consumers.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 26, 2024

Ah you are right we are only encoding the index in the index area of content, no drama - my bad.

@tcharding
Improve Witness::push unit test
7e2899d 
Make an effort to clean up the `Witness::push` unit test. This patch
dose not change the test, it only attempts to make it easier to read.
@tcharding
Improve Witness consensus encode unit test
9860453 
Make an effort to clean up the encoding unit test, by doing:

- Remove element accessor assertions (tested already above)
- Add roundtrip encoding assertion
@tcharding
Refactor the serde Witness unit tests
f6a74ef 
In preparation for moving unit tests to `primitives` give the serde
tests some love by doing:

- Split them up to do one thing only
- Round trip arbitrary witness
- Use better names
@tcharding
Rename iter len unit test
5fab6b1 
Rust convention is to not use `test_` prefix on unit tests. Also this
unit test is testing that the `ExactSizedIterator` trait is implemented
and working.

Re-name unit test to `exact_sized_iterator`.
@tcharding tcharding force-pushed the 09-24-witness-refactor-tests branch from 42a962e to 5fab6b1 Compare September 26, 2024 05:32
apoelstra
apoelstra approved these changes Sep 26, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 5fab6b1 successfully ran local tests

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Sep 29, 2024

Merge please. This is holding up more primitives work, onwards and upwards!

@apoelstra apoelstra merged commit 7379eba into rust-bitcoin:master Sep 30, 2024
@tcharding tcharding deleted the 09-24-witness-refactor-tests branch October 14, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

C-bitcoin PRs modifying the bitcoin crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tcharding @apoelstra