script: BIP341 txdata cannot be precomputed without spent outputs #27122
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Note that this shouldn't affect any consensus logic, as |
Simplicity needs this fix.
maflcko
changed the title
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
ACK 95f12de
I do wonder if it makes sense to rename force to something like signing_context or even more drastically split Init up into two obvious types ConsensusInit/SigningInit to make things clearer to future readers.
|
ACK 95f12de This was directly addressed in the comments earlier; should that be updated too?
The immediately following comment seems wrong too, as the branch is gated by a
Also seems weird to have the |
I believe this comment is saying it will trigger in the case the the scriptPubKey is OP_1 followed by a series of opcodes of an appropriate length, but is not a single push opcode, and a witness is (illegally) provided. |
I believe this comment is trying to say that (under the implicit circumstances that |
|
ACK 95f12de |
Simplicity needs this fix.
…out spent outputs 95f12de BIP341 txdata cannot be precomputed without spent outputs (Pieter Wuille) Pull request description: In `PrecomputedTransactionData::Init`, if `force` is set to `true`, `m_bip341_taproot_ready` is always set to true, suggesting that all its BIP341-relevant members (including `m_spent_amounts_single_hash`) are correct. If however no `spent` array of spent previous `CTxOut`s is provided, some of these members will be incorrect. This option was introduced in bitcoin#21365. That doesn't actually hurt, as without prevout data, it's fundamentally impossible to generate correct BIP341 signatures anyway, and https://github.com/bitcoin/bitcoin/blob/f722a9bd132222d9d5cd503b5af25c905b205cdb/src/script/sign.cpp#L71 should prevent the logic from being used anyway. Still, don't set `m_bip341_taproot_ready` variable when we clearly don't have enough data to compute it. Discovered by Russell O'Connor. ACKs for top commit: ajtowns: ACK 95f12de achow101: ACK 95f12de instagibbs: ACK 95f12de Tree-SHA512: 90acd2bfa50a7a0bde75a15a9f6c1f5c40f48fb5b870b1bbc4082777e24a482c8282463ef7d1245e53201dbcb5c196ef0386352f8e380e68cdf00c2111633b77
6 participants
In
PrecomputedTransactionData::Init, ifforceis set totrue,m_bip341_taproot_readyis always set to true, suggesting that all its BIP341-relevant members (includingm_spent_amounts_single_hash) are correct. If however nospentarray of spent previousCTxOuts is provided, some of these members will be incorrect. This option was introduced in #21365.That doesn't actually hurt, as without prevout data, it's fundamentally impossible to generate correct BIP341 signatures anyway, and
bitcoin/src/script/sign.cpp
Line 71 in f722a9b
Still, don't set
m_bip341_taproot_readyvariable when we clearly don't have enough data to compute it.Discovered by Russell O'Connor.