Remove usage of `opcodes::all` #1295

tcharding · 2022-09-20T08:13:04Z

We have all of the opcodes defined in a submodule called all, this allows wildcard imports without bringing in the other types in the opcodes module.

Use wildcard import use crate::blockdata::opcodes::all::* instead of fully qualifying the path to opcodes.

Original PR description (left here so the thread of discussion below makes sense)

The all module adds no value, we can remove it without loss of meaning or clarity, doing so makes the code less verbose.

EDIT: After review, now includes importing with wildcard and removing the opcodes:: path from any type starting with OP_.

Idea stolen from: #525 (patch 7)

apoelstra · 2022-09-20T17:06:04Z

I think we should also drop the OP_ prefix. Thoughts?

tcharding · 2022-09-20T23:03:07Z

Perhaps rename module to op and drop the OP_ then common usage becomes, for example,op::VERIFY.

apoelstra · 2022-09-21T00:03:58Z

Yeah, I like that.

tcharding · 2022-09-21T23:40:03Z

Drats, we can't do it because of things like OP_2DROP. We can do the module rename though.

apoelstra · 2022-09-24T12:50:31Z

I'm a little tempted to name 2DROP etc _2DROP. There are only a few of these opcodes and they are practically never used.

@Kixunil what would you think about this? It would be similar to renaming crate to krate elsewhere in the ecosystem.

Kixunil · 2022-09-24T16:35:00Z

_ at the beginning means "don't warn if this is unused" so I'd prefer to not do that. Raw identifiers can't begin with numbers either. If renaming 2DROP to DROP2 is no-go then we probably need to keep the OP_ prefix.

apoelstra · 2022-09-28T17:36:13Z

We should probably add a commit that adds a deprecated module opcodes and opcodes::all and just re-export everything from both.

(If we want to be more careful I guess opcodes::all should only contain opcodes, but it's easy to just write pub use crate::op::* and I don't think we need to be careful.)

apoelstra

ACK e1eb641eacf2e1340b5f4b9135ff8550986aaf33

tcharding · 2022-09-28T18:26:32Z

We should probably add a commit that adds a deprecated module opcodes and opcodes::all and just re-export everything from both.

By "deprecated" you mean just deprecated to use Rust Bitcoin devs, right? Remembering that I don't currently know of a way to deprecated modules.

apoelstra · 2022-09-28T21:16:17Z

Remembering that I don't currently know of a way to deprecated modules

Ah, damn, I forgot this.

tcharding · 2022-09-29T23:21:34Z

know of a way to deprecated modules

That's good English, right there :)

tcharding · 2022-10-18T19:18:29Z

Friendly bump, is this one good to go in? Needs a second ACK if so please.

Kixunil

This is moving in the right direction but not sure if we should change this just to change it again if/when we change to enum.

I'm also considering this design to solve #1187:

/// Types that can be used as opcodes
pub trait Opcode: sealed::Opcode + Into<AnyOpcode> + Copy {
    fn to_u8(self) -> u8;
}

macro_rules! all_opcodes {
    ($($name:ident = $val:expr (, $subclass:ident)*);+ $(;)?) => {
        /// May contain any opcode of bitcoin script.
        #[allow(non_camel_case_types)]
        pub enum AnyOpcode {
            $(
                $name = $val,
            )+
        }
        $(
            /// The zero-sized opcode.
            ///
            /// Zero-sized opcodes can statically guarantee some properties. e.g. (not) carrying bytes parameter. These are represented as conversion traits being implemented.
            #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
            pub struct $name;

            impl From<$name> for AnyOpcode {
                fn from(opcode: $name) -> Self {
                    AnyOpcode::from_u8(opcode.to_u8())
                }
            }

            impl Opcode for $name {
                fn to_u8(self) -> u8 {
                    $value
                }
            }

             impl From<$name> for $subclass {
                fn from(opcode: $name) -> Self {
                    $subclass(opcode.to_u8())
                }
            }
        )+
    }
}

/// Opcodes that don't take a byte array parameter
pub struct NonParametrised(u8);

Then the Instructions iterator could use NonParametrised instead of All. Script::push_opcode would become: fn push_opcode<Op: Opcode + Into<NonParametrised>(&mut self, op: Op)

Kixunil · 2022-10-21T10:39:14Z

bitcoin/src/address.rs

-                WitnessVersion::try_from(version - opcodes::all::OP_PUSHNUM_1.to_u8() + 1),
+                if version >= op::OP_PUSHNUM_1.to_u8()
+                    && version <= op::OP_PUSHNUM_16.to_u8() =>
+                WitnessVersion::try_from(version - op::OP_PUSHNUM_1.to_u8() + 1),


Since all the opcodes begin with OP_ maybe just use op::*; and drop op::? The main reasons for not having ::* is name conflicts and not being obvious where things come from but this situation seems like a good exception.

We can't do this because some opcodes start with 2.

I'm suggesting the other change - keep OP_ and drop op::. Now we have op::OP_ which is kinda silly.

Ah, I see. agreed.

Kixunil · 2022-10-21T10:44:17Z

bitcoin/src/blockdata/opcodes.rs

@@ -26,632 +26,627 @@ pub struct All {
    code: u8,
 }


Why this isn't an enum? Maybe we should change it while we're at it?

Because conversion from a u8 to an enum requires either a 256-arm match or a transmute.

Neither of them sounds too bad to me.

apoelstra · 2022-10-21T14:42:26Z

@Kixunil I tend to agree "let's not change this just to change it again". But I remain unconvinced that we can find a much better API than the current messy one.

Maybe if our only goal is to distinguish opcodes which are followed by other non-opcode data, from opcodes which are "zero sized", that would work since this is an unambiguous partition of all the opcodes. But "push vs non-push", "numeric vs non-numeric", cannot be so clearly separated.

This also maybe mixes in with #1324 because we may want some opcodes CHECKMULTISIG/CHECKSIGADD to only be available in certain contexts. I'm not sure if that'd affect this API.

It all seems like a big difficult problem.

Kixunil · 2022-10-21T17:11:11Z

Is your idea to just merge it nd iterate on that? That's not too bad for something you doubt can be done (I think it can).

But "push vs non-push", "numeric vs non-numeric", cannot be so clearly separated.

My design idea has no problem with that. The intention is mainly to verify an opcode is usable in certain context.

This also maybe mixes in with #1324

Yes, it does and my idea should work with it too. It'll be a bit more fiddly but not a huge issue, I think.

apoelstra · 2022-10-21T17:13:41Z

Is your idea to just merge it nd iterate on that? That's not too bad for something you doubt can be done (I think it can).

Yes, that's my vote -- though I expect that whatever we release in 0.30 won't be the final form, and that we'll have more breakage in 0.31.

If you want to avoid repeated breakage like that, we should hold off on merging this until we've come up with a more comprehensive design. I'd rather just move forward and iterate, though I don't feel super strongly.

Kixunil · 2022-10-21T17:15:58Z

OK, no problem with merging and I will try to find some time to sketch up alternative before 0.30 is released. But even then I don't expect 0.30 to become stable. The whole stabilization project is an insane monstrosity and will take time.

tcharding · 2022-10-21T19:13:08Z

The whole stabilization project is an insane monstrosity and will take time.

Lolz, the same thought had occurred to me of late.

tcharding · 2022-10-22T19:19:44Z

Mentioned in PR description but double posting, now includes importing with wildcard and removing the opcodes:: path from any type starting with OP_.

apoelstra · 2022-10-22T20:14:04Z

Ok, weak concept ACK on this design. I agree it's annoying to write op::OP_WHATEVER but I also really dislike wildcard imports because it mean that when you see OP_WHATEVER you can't tell where it came from by searching the current file.

But for opcodes maybe it's okay to make an exception because there's so many of them and they're all easily identifiable.

apoelstra

ACK d5d34a2a7611a6c3fe984df8397cde2b6567e386

tcharding · 2022-10-23T20:53:43Z

But for opcodes maybe it's okay to make an exception because there's so many of them and they're all easily identifiable.

hmmm, if you have any apprehension over wildcard imports then perhaps this PR is not good because it allows use of naked All, I maintained usage of opcodes::All even in a file with wildcard import but this is not enforced by the compiler. I'm ambivalent, both op::OP_FOO and wildcard with OP_FOO seem ok to me. Perhaps we should add re-name of opcodes module to op back into this PR then users have the choice of doing either? (And for our code we can just use the module prefix by convention for any type that does not have OP_ prefix.)

Kixunil · 2022-10-23T22:04:25Z

Oh, yes, accidental import of All is annoying. But maybe if it was named AnyOpcode instead it wouldn't be that bad.

One thing about wildcard imports: if you have only one it's easy to conclude "there's one wildcard import so the item must coms from it". It's still harder than not having it but the tradeoff seems in favor of using it here.

We have all of the opcodes defined in a submodule called `all`, this allows wildcard imports without bringing in the other types in the `opcodes` module. Use wildcard import `use crate::blockdata::opcodes::all::*` instead of fully qualifying the path to opcodes.

tcharding · 2022-10-24T02:14:39Z

Huh, hilariously the wildcard usage is currently supported in a super clean way because we have the all submodule, instead of removing it, just use it as is currently possible.

Excuse the noise, this was a very roundabout way to find a pretty basic improvement, amusingly no one else saw it either :)

Kixunil · 2022-10-24T08:00:37Z

Do we want to keep the name all?

tcharding · 2022-10-24T09:47:51Z

Yeah I rekon this usage by users of the lib reads nicely

use bitcoin::opcodes::all::*
use bitcoin::opcodes;

Kixunil

ACK c3e4399

I'd also ACK a future commit/PR explicitly promising in the doc that the all module will only ever contain opcodes prefixed with OP_ so it's OK to glob-import as long as the user doesn't have any other OP_ items in the module.

apoelstra

ACK c3e4399

2157e69 Document the `all` module (Tobin C. Harding) Pull request description: Improve documentation on the `all` module by doing: - Document guarantee that `all` will only ever contain opcode constants - Fix stale/incorrect code comment Done as follow up to #1295 ACKs for top commit: apoelstra: ACK 2157e69 Kixunil: ACK 2157e69 Tree-SHA512: 4df091bbdce7b9ba73caabd74b80f9e8c0a30fa2f9a20ed9b75542e71a204e5cd82698a74bebbd6f0beab55ecd807154d1b7d27a787cc9dede7abbd20a0a4ad5

tcharding mentioned this pull request Sep 20, 2022

Rearrange all the modules #525

Closed

tcharding force-pushed the 09-20-rm-all-module branch from 19d2f4b to a31b9c9 Compare September 21, 2022 23:44

tcharding force-pushed the 09-20-rm-all-module branch 2 times, most recently from 4e2b944 to e1eb641 Compare September 27, 2022 18:15

apoelstra previously approved these changes Sep 28, 2022

View reviewed changes

apoelstra mentioned this pull request Sep 28, 2022

Start to flatten util #1169

Merged

Kixunil reviewed Oct 21, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via d5d34a2 October 22, 2022 19:18

tcharding force-pushed the 09-20-rm-all-module branch from e1eb641 to d5d34a2 Compare October 22, 2022 19:18

apoelstra previously approved these changes Oct 23, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via c3e4399 October 24, 2022 02:11

tcharding force-pushed the 09-20-rm-all-module branch from d5d34a2 to c3e4399 Compare October 24, 2022 02:11

tcharding changed the title ~~Remove the opcodes::all module~~ Remove usage of opcodes::all Oct 24, 2022

Kixunil approved these changes Oct 24, 2022

View reviewed changes

apoelstra approved these changes Oct 24, 2022

View reviewed changes

apoelstra merged commit afcdfa4 into rust-bitcoin:master Oct 24, 2022

tcharding deleted the 09-20-rm-all-module branch October 24, 2022 21:50

tcharding mentioned this pull request Oct 24, 2022

Improve documentation on the all module #1339

Merged

Remove usage of opcodes::all #1295

Remove usage of opcodes::all #1295

Uh oh!

Conversation

Uh oh!

Original PR description (left here so the thread of discussion below makes sense)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Remove usage of `opcodes::all` #1295

Remove usage of `opcodes::all` #1295