Debug ms attributes test #413
Closed
Conversation
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
Add a macro so that each test vector is run in a separate test. This makes a failing test easily identifiable.
We use the test vectors from Sipa's `miniscript/bitcoin/test/miniscript_test.cpp`. Two new vectors seem to have been added, so add those here (they both pass). Two of the vectors we have have different values for `ops` (also one of ours uses `nd:and_v` instead of `d:and_v`), add those two in as well (both fail). Currently the following tests are failing failures: miniscript::tests::check_attributes_15a miniscript::tests::check_attributes_16 miniscript::tests::check_attributes_16a miniscript::tests::check_attributes_26 miniscript::tests::check_attributes_27 miniscript::tests::check_attributes_7
|
I will look into this later, but I just looked at 16a for now. This relates to calculations about malleable scripts. The c++ code outputs incorrect values for malleable scripts. There may be other reasons why there is a discrepancy in other cases and we should resolve this. I would suspect this again relates to the calculation when scripts are malleable. |
|
Thanks for looking @sanket1729. |
|
Based on some discussion here: bitcoin/bitcoin#24147 (comment), we should only care for these values if the scripts are non-malleable. We should either
|
|
In the future, if you need any assistance with a PR, feel free to tag me. I typically ignore the PRs in the marked draft. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Try to do issue: #347
First patch is preparation, adds a macro so each of the test vectors is checked in a separate test so we can debug.
We use the test vectors from Sipa's
miniscript/bitcoin/test/miniscript_test.cpp. Two new vectors seem to have been added, patch 2 adds those here (they both pass).Two of the vectors we have have different values for
ops(also one of ours usesnd:and_vinstead ofd:and_v, however I do not know the implications of this), patch 2 adds the original ones as well (both fail).With this patch set applied we have:
I don't have any idea how to proceed. If anyone has any suggestions I can follow them. Thanks