Skip to content

Update miniscript to bitcoin core e8f85e0e86e92e583b8984455b7bf9d0a777578a(July 18th) #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 2, 2021
Merged

Update miniscript to bitcoin core e8f85e0e86e92e583b8984455b7bf9d0a777578a(July 18th) #58

merged 2 commits into from
Dec 2, 2021

Conversation

sanket1729
Copy link
Contributor

This builds on top #41. The PR consists also has a second commit that adds tests for timelock mix detection.

@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch 2 times, most recently from 017fe31 to 6cbec31 Compare July 23, 2021 13:42
darosior
darosior reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Doesn't 6cbec31 belong to #41 ?

bitcoin/test/miniscript_tests.cpp Outdated
@@ -498,6 +499,7 @@ void Test(const std::string& ms, const std::string& hexscript, int mode, int ops
if (hexscript != "?") BOOST_CHECK_MESSAGE(HexStr(computed_script) == hexscript, "Script mismatch: " + ms + " (" + HexStr(computed_script) + " vs " + hexscript + ")");
BOOST_CHECK_MESSAGE(node->IsNonMalleable() == !!(mode & TESTMODE_NONMAL), "Malleability mismatch: " + ms);
BOOST_CHECK_MESSAGE(node->NeedsSignature() == !!(mode & TESTMODE_NEEDSIG), "Signature necessity mismatch: " + ms);
BOOST_CHECK_MESSAGE(node->CheckTimeLocksMix() == !(mode & TESTMODE_TIMELOCKMIX), "Timelock mix mismatch: " + ms);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing an exclamation mark :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one exclamation is required since the ChecktimelocksMix returns if there is a timelock mix. So, the single exclamation mark is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, i was confused. You are testing for the presence of locktime mix!

@sanket1729
Copy link
Contributor Author

Doesn't 6cbec31 belong to #41 ?

It does, but it's sort of more involved. All tests in miniscript_tests.cpp are based on an old version of miniscript where pk() and multi nodes were updated. I could create a test based on the old version of miniscript, but it seems a waste when I have to update to test again.

@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch 3 times, most recently from 886f9a8 to ffc4dc6 Compare July 24, 2021 14:12
@sanket1729 sanket1729 mentioned this pull request Jul 24, 2021
Merged
@darosior darosior mentioned this pull request Jul 25, 2021
Merged
@darosior darosior mentioned this pull request Aug 4, 2021
Merged
meshcollider
meshcollider reviewed Aug 18, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

Looks good, and commit "Update miniscript to bitcoin core ..." matches exactly what I had to change when rebasing 16800 too

bitcoin/util/spanparsing.h Outdated
*
* If sep does not occur in sp, a singleton with the entirety of sp is returned.
*
* Note that this function does not care about braces, so splitting
Copy link
Contributor

Choose a reason for hiding this comment

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

braces -> brackets/parentheses

bitcoin/test/miniscript_tests.cpp Outdated
}
};

//! Singleton instance of KeyConverter.
Copy link
Contributor

Choose a reason for hiding this comment

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

outdated typo from copy and paste

sipa added a commit that referenced this pull request Sep 28, 2021
@sipa
Merge #61: Sanity check aliases
8b110f4 
e0ddd7c test: sanity checks for pk() and pkh() aliases (Antoine Poinsot)

Pull request description:

  Based on #58.

  Stole the test cases for a Python implementation i'm working on, noticed these aliases weren't covered.

ACKs for top commit:
  sipa:
    ACK e0ddd7c

Tree-SHA512: 4aab684fb6131b0141a4f5fbb7edfea4bd5062a1b49a561b3ba5941430c533cc8248a7abcc7957fa666914df46565d1b8b99e1a1c3a551abcba90c357908010f
sipa added a commit that referenced this pull request Sep 28, 2021
@sipa
Merge #59: Test lax thresh bounds
d2f497f 
3c9a837 test: inclusive bounds for thresh()'s threshold (Antoine Poinsot)

Pull request description:

  #55 didn't come with any test. This fixes it.

  Based on #58 to benefit from the update and avoid needless conflicts.

ACKs for top commit:
  sipa:
    ACK 3c9a837

Tree-SHA512: 38d5a271271c665267d50750b563c4312ccda497f42a6a4bf6750542232880a8839ad68cd0348cb3158b236a51c3439ebee1ee278c0c0a66195acc7606c35a5d
@darosior
Copy link
Contributor

Good news is that this update is valid up to Bitcoin Core commit 33e31f8df920acd3a4b13aab5bd6a6733480133a :)
Didn't notice any new conflict rebasing darosior/bitcoin#2 to this head!

@sipa
Copy link
Owner

sipa commented Sep 29, 2021

Rebase?

@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch from ffc4dc6 to 8c8384c Compare September 29, 2021 21:57
@sanket1729
Copy link
Contributor Author

Rebased

@sanket1729
Copy link
Contributor Author

This also fixes the tests on stack height(with the +1) with a minimal diff for now. I think we should resolve #77 (comment) to avoid conflicts with this PR

@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch from 8c8384c to 5e019ae Compare September 30, 2021 18:27
@sanket1729
Copy link
Contributor Author

This is ready for review.

sipa
sipa reviewed Oct 14, 2021
View reviewed changes
@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch from 5e019ae to d762225 Compare October 15, 2021 13:54
@sanket1729 sanket1729 force-pushed the update_bitcoin_master branch from d762225 to 2dba8d6 Compare October 15, 2021 13:57
@sipa
Copy link
Owner

sipa commented Dec 2, 2021

ACK 2dba8d6

@sipa sipa merged commit 64ae52d into sipa:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sipa sipa sipa left review comments

+2 more reviewers

@meshcollider meshcollider meshcollider left review comments

@darosior darosior darosior left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sanket1729 @darosior @sipa @meshcollider