Conversation
Removes unsafety when converting u8 -> All
55ec6eb to
2a9b66f
Compare
2a9b66f to
9fee72c
Compare
|
We should do some automatic validation. I'm pretty sure our existing tests do not cover all 256 opcodes. |
|
@sgeisler added a commit with an exhaustive unit test; found a couple typos in the serializations. Can you take a look and ACK that commit? ACK from me. |
| /// Synonym for OP_RETURN | ||
| OP_RETURN_255 = 0xff, | ||
| #[derive(Copy, Clone, PartialEq, Eq)] | ||
| pub struct All { |
There was a problem hiding this comment.
If I may make a suggestion:
Now that this is no longer an enum but a struct, the name All doesn't make much sense to me. How about renaming it to OpCode or something like that?
There was a problem hiding this comment.
The name All is because I wanted to be able to use opcodes::All (in contrast to opcodes::Class and opcodes::Ordinary). I don't see why the restructuring changes that?
I'm not really opposed to using OpCode or something, it just feels a bit nicer to me to leave things as is.
There was a problem hiding this comment.
I thought because the change is already breaking, one might as well tackle this bit. My idea would be to export the struct OpCode next to a module opcodes, i.e. something like this:
use bitcoin::blockdata::Opcode;
use bitcoin::blockdata::opcodes::OP_CHECKSIG;star-imports also looks quite nice then:
use bitcoin::blockdata::opcodes::*;It would have the added benefit that referring to an OpCode inside a parameter/struct would be slightly more descriptive:
fn foo(opcode: OpCode)There was a problem hiding this comment.
opcodes::All and OpCode seem exactly as descriptive to me.
I really hate star-imports, they make it impossible to tell where symbols come from, and I don't have any interest in making them easier to use. But sure, we can rename All to OpCode and re-export it at the crate level.
|
@apoelstra I'll take a look at it tomorrow. My plan was to reproduce most of the changes with some |
|
Except for the weird 2 space indentation this PR looks good (did some |
96cb2ec Add comment to decoding x-only keys (sanket1729) 498bad2 Address review feedback (sanket1729) 954df35 Check stack execution height in tapscript execution (sanket1729) 95ab99d Add height check to miniscript types (sanket1729) 3eb63d5 Add parse/parse_insane for schnorr keys (sanket1729) 888cd62 Add support for parsing Tapscript Miniscripts (sanket1729) 9a5384d Add TapCtx (sanket1729) 4da439f Fix warnings (sanket1729) Pull request description: This is still a draft WIP for taproot PR. There are several API changes and multiple design decisions, and I am open to other possible design opinions. In particular, feedback about changes to `ToPublicKey` and the `ScriptContext` trait is appreciated. This does **not** include support for P2TR descriptors, multi_a fragment in miniscript. Still todo: - [ ] Track height during execution to make sure we don't exceed 1000 elements. This was not a concern previously because it was impossible to hit the limit without exceeding 201 opcodes. But with taproot, this is now possible. See rust-bitcoin#198 . - [ ] Implement support for multi_a fragment in Taproot. Depends on support for new opcodes from rust-bitcoin. We can use NOPs meanwhile as this is not a hard blocker. - [ ] Implement taproot descriptor. This would require some design considerations about the tree representation. - [ ] Integrate taproot with interpreter and compiler. The interpreter is probably straightforward but carrying around `bitcoin:::PublicKey` and `schnorr::Publickey` would be tricky. - [ ] Integrate with descriptorPublicKey so that descriptor wallets like bdk can actually use it. ACKs for top commit: apoelstra: ACK 96cb2ec Tree-SHA512: 261173c3a6f8c980d204fce1f868dcd17c4219fd12a8e6e197b503efb47ae9b9371d7a9208c43008f1c21db55ffc3f68e6d7fb8badbf2bf397b2970292f8ffe4
* chore: clippy auto fixes * more auto fixes in another package
This is the same as #193 from @kazcw but with my patch for rust-1.14.0 compatibility. I didn't check every opcode constant, but generally @kazcw's changes look good. @apoelstra should I do some automated validation or should our tests catch faulty opcode constants?