Skip to content

Implement Taproot Sighash - #1002

Merged
apoelstra merged 7 commits into
ElementsProject:masterfrom
sanket1729:taphash
Jul 7, 2021
Merged

Implement Taproot Sighash#1002
apoelstra merged 7 commits into
ElementsProject:masterfrom
sanket1729:taphash

Conversation

@sanket1729

@sanket1729sanket1729 commented May 20, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@sanket1729sanket1729 changed the title Implement TaphashImplement Taproot SighashMay 20, 2021
@sanket1729
sanket1729force-pushed the taphash branch 3 times, most recently from fde71a0 to 4a7685aCompareMay 21, 2021 04:04
@sanket1729sanket1729 mentioned this pull request May 21, 2021
Closed
@sanket1729

sanket1729 commented May 21, 2021

Copy link
Copy Markdown
ContributorAuthor

The test seems flaky

sanket1729
sanket1729 commented May 21, 2021
View reviewed changes

@sanket1729sanket1729 left a comment •
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

For reference failed logs: https://cirrus-ci.com/task/6028890096271360?logs=ci#L3628

@sanket1729

sanket1729 commented May 21, 2021

Copy link
Copy Markdown
ContributorAuthor

No diff between 4a7685a8202d39.

apoelstra
apoelstra reviewed May 25, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp Outdated
apoelstra
apoelstra reviewed May 25, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp Outdated
sanket1729
sanket1729 commented May 26, 2021
View reviewed changes

@sanket1729sanket1729 left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed the comments

Comment threadsrc/script/interpreter.cpp Outdated
@sanket1729
sanket1729force-pushed the taphash branch 3 times, most recently from f2452a3 to afa015dCompareJune 7, 2021 22:24
apoelstra
apoelstra reviewed Jun 8, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp Outdated
apoelstra
apoelstra reviewed Jun 8, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp Outdated
apoelstra
apoelstra reviewed Jun 8, 2021
View reviewed changes
Comment threadtest/functional/feature_taproot.py Outdated
apoelstra
apoelstra reviewed Jun 8, 2021
View reviewed changes
Comment threadtest/functional/feature_taproot.py Outdated
@sanket1729
sanket1729force-pushed the taphash branch 2 times, most recently from 40f50e8 to f1ac1beCompareJune 8, 2021 15:22
@apoelstra

apoelstra commented Jun 8, 2021

Copy link
Copy Markdown
Member

I think, as long as our test suite passes, there is no need to add new g_con_elementsmode uses. We should just drop this and always only do Taproot sighashes in Elements mode.

apoelstra
apoelstra reviewed Jun 8, 2021
View reviewed changes
Comment threadtest/functional/test_framework/script.py Outdated
ss += struct.pack("<i", txTo.nVersion)
ss += struct.pack("<I", txTo.nLockTime)
if in_type != SIGHASH_ANYONECANPAY:
ss += sha256(b"".join(struct.pack("B", ((not i.assetIssuance.isNull()) >> 7) + (i.m_is_pegin >> 6)) for i in txTo.vin))

@apoelstraapoelstraJun 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should be << rather than >> (same below)

@sanket1729sanket1729Jun 8, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

One more reason to write tests :)

@apoelstra

apoelstra commented Jun 17, 2021

Copy link
Copy Markdown
Member

e7b4bba looks great! Just needs some more tests and we're good to go

apoelstra
apoelstra reviewed Jun 18, 2021
View reviewed changes
Comment threadtest/functional/test_framework/messages.py Outdated
return r

# serialization used in taproot sighash
def tap_sighash_serialize(self):

@apoelstraapoelstraJun 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you rename this (and change the comment) to indicate that it is specifically for asset issuance data?

@sanket1729sanket1729Jun 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It is inside the class CAssetIssuance, so it is implicit that it is for asset issuance?

@sanket1729sanket1729Jun 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

edited the name to taphash_asset_issuance_serialize

apoelstra
apoelstra reviewed Jun 18, 2021
View reviewed changes
Comment threadtest/functional/test_taphash_pegins_issuances.py Outdated
#
# Test use of assetdir to locally label assets.
# Test listissuances returns a list of all issuances or specific issuances based on asset hex or asset label.
#

@apoelstraapoelstraJun 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great! Can you add it to test_runner.py so it gets run in CI?

sanket1729
sanket1729 commented Jun 18, 2021
View reviewed changes

@sanket1729sanket1729 left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Changed the function name and added test to test_runner.py

apoelstra
apoelstra approved these changes Jul 7, 2021
View reviewed changes

@apoelstraapoelstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 14f9357

Ran all tests (including fuzz tests) several times; did some basic mutation testing on the functional tests to check that obscure things were covered; also poked at the extended tests in #1003 which we are unable to merge.

@apoelstra
apoelstra merged commit 1ba24fe into ElementsProject:masterJul 7, 2021
@apoelstraapoelstra mentioned this pull request Jul 9, 2021
Merged
stevenroose added a commit that referenced this pull request Jul 12, 2021
@stevenroose
Merge#1017: test: updatecreaterawtransactioncall in feature_taph…
aad04d0
…ash_pegins_issuances.py
1c883db test: update `createrawtransaction` call in feature_taphash_pegins_issuances.py (Andrew Poelstra)
Pull request description:
Should describe the outputs as an array rather than as an object. (The old behavior has long been deprecated but was eliminated entirely in #900.)
Fixes functional tests which were broken in master by simultaneous merge of #1002 and #900.
ACKs for top commit:
stevenroose:
utACK 1c883db
Tree-SHA512: f7963c34e7006a25ac5515e30966ef46777fa22d6125d219345731aef603e5f3179fc316134d59694af8e753f7cb48c825ad3434f2bceda0a10b8d5a52c32cd3
roconnor-blockstream
roconnor-blockstream reviewed Jul 26, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp
{
CHashWriter ss(SER_GETHASH, 0);
for (const auto& txin : txTo.vin) {
ss << (unsigned char) ((!txin.assetIssuance.IsNull() << 7) + (txin.m_is_pegin << 6));

@roconnor-blockstreamroconnor-blockstreamJul 26, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider instead using (COutPoint::OUTPOINT_ISSUANCE_FLAG >> 24) and (COutPoint::OUTPOINT_PEGIN_FLAG >> 24) (you can compose the flags and shift the result if you prefer).

I believe | is prefered over + for flag composition.

roconnor-blockstream
roconnor-blockstream reviewed Jul 26, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp
const uint8_t spend_type = (ext_flag << 1) + (have_annex ? 1 : 0); // The low bit indicates whether an annex is present.
ss << spend_type;
if (input_type == SIGHASH_ANYONECANPAY) {
ss << (unsigned char) ((!tx_to.vin[in_pos].assetIssuance.IsNull() << 7) + (tx_to.vin[in_pos].m_is_pegin << 6));

@roconnor-blockstreamroconnor-blockstreamJul 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably abstract this flag composition into its own function as it occurs in two places.

roconnor-blockstream
roconnor-blockstream reviewed Jul 26, 2021
View reviewed changes
Comment threadsrc/script/interpreter.cpp
ss << tx_to.vin[in_pos].prevout;
ss << cache.m_spent_outputs[in_pos];
ss << cache.m_spent_outputs[in_pos].nAsset;
ss << cache.m_spent_outputs[in_pos].nValue;

@roconnor-blockstreamroconnor-blockstreamJul 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe comment here that we are not using the nonce and maybe even why we aren't?

@sanket1729sanket1729 mentioned this pull request Jul 26, 2021
Merged
@sanket1729

sanket1729 commented Jul 26, 2021

Copy link
Copy Markdown
ContributorAuthor

@roconnor-blockstream Addressed in #1023

apoelstra added a commit that referenced this pull request Aug 17, 2021
@apoelstra
Merge#1023: Address post merge feedback for Taphash
0b8174b
2612017 Address post merge feedback for Taphash (sanket1729)
Pull request description:
Addressing the review by @roconnor-blockstream on #1002 .
ACKs for top commit:
apoelstra:
ACK 2612017
Tree-SHA512: 62121ba33cf1fccda75cd2402c22799ccee437ba64575e6f5561b0aa1c571b6d94f3981fb4c1260a8c2848a26e1790d770364ade2af3edb5a98be29c23d6e0a2
This was referenced Sep 5, 2021
Closed
Closed
@gwillengwillen mentioned this pull request Apr 12, 2022
Closed
gwillen pushed a commit that referenced this pull request Jun 1, 2022
@apoelstra
Merge5c2e2afinto merged_master (Bitcoin PRbitcoin/bitcoin#21365)
0afcb6d
This introduces Taproot wallet support. I fixed all the merge conflicts
and ensured that the tests pass, but this is still using the old sighash
(before Russell/Sanket/I redid it) so is not actually production ready.
Will be fixed when we bring Elements #1002 in.
gwillen pushed a commit that referenced this pull request Jun 1, 2022
@apoelstra
Merge1ba24feinto merged_master (Elements PR#1002)
7d1c77f
This forward-ports the new Taproot sighash but does not fix a couple
22-blocked TODOs related to the MissingDataBehavior enum. Should be fixed
in a followup commit.
One nontrivial change I had to make was feeding the genesis hash to SignTransaction
(the "global" one in script/sign.cpp) so that it could correctly compute
the sighash at signing time.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roconnor-blockstreamroconnor-blockstreamroconnor-blockstream left review comments
@apoelstraapoelstraapoelstra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sanket1729@apoelstra@roconnor-blockstream