Constify as much as possible #1770

Kixunil · 2023-04-01T23:06:53Z

This changes many functions to be const-enabled and marks them as such.

apoelstra · 2023-04-02T13:19:22Z

concept ACK, though this is a 1200-line diff which does a few different things.

Could you split it up into

using the matches! macro
introducing and using the new const_tools macros
all the "easy" const changes which were mostly just adding the const keyword and doing minor tweaks to function code
whatever you're doing with ProgramBuf

Kixunil · 2023-04-02T14:08:31Z

Yeah, I'm also thinking of doing hashes first since it trivially conflicts with #1773

bitcoin/src/address.rs

Kixunil · 2023-04-19T16:01:33Z

Just rebase and bugfix, will split it up later.

stevenroose · 2023-04-19T16:02:52Z

concept ACK, though this is a 1200-line diff which does a few different things.

Could you split it up into

* using the `matches!` macro

* introducing and using the new `const_tools` macros

* all the "easy" `const` changes which were mostly just adding the const keyword and doing minor tweaks to function code

* whatever you're doing with `ProgramBuf`

This please :)

Kixunil · 2023-04-24T07:14:57Z

(Just running CI now.)

These functions were trivially calling another function so `#[inline]` is useful.

We are sure these will never do syscalls.

This removes the requirement of importing `Hash` trait for trivial conversions and allows the methods to be `const`. The original methods remain on the trait and do the same thing.

Methods of these can be obviously `const` but need to handle errors using `match`/`if` instead of combinators.

Kixunil · 2023-04-25T09:17:55Z

Decided to push part of this since I'm not in the mood of finishing whole thing but didn't feel like keeping this open too long. Feel free to review. I might or might not add new commits until there are no reviews and open new PR(s) later depending on how things turn out.

tcharding

I'd like to see the rustdocs fixed but the code changes are good to go IMO.

tcharding · 2023-04-26T01:14:54Z

hashes/src/internal_macros.rs

@@ -207,6 +206,24 @@ macro_rules! hash_type {
            fn internal_new(arr: [u8; $bits / 8]) -> Self { Hash(arr) }

            fn internal_engine() -> HashEngine { Default::default() }
+
+            /// Construts this wrapper type from an array.


Typo Construts here and other places as well.

This comment seems like it was cut'n paste but doesn't quite fit, the Hash is not a wrapper type, right?

It can be considered a wrapper type around an array but I do not like that perception (even though it's being promoted by Deref/Index traits) so I'm happy to fix that.

tcharding · 2023-04-26T01:15:57Z

hashes/src/internal_macros.rs

+            #[inline]
+            pub const fn to_byte_array(self) -> [u8; $bits / 8] { self.0 }
+
+            /// Returns the reference to raw hash bytes.


I'm not sure about the use of the term 'raw' when we have other functions that use raw in the identifier from_raw_bytes but I couldn't think of a better wording.

"underlying" may be better.

Ah yes, much better. Thanks

If we want to stick to more outdoorsy words, we could consider "bare" :)

tcharding · 2023-04-26T01:24:02Z

bitcoin/src/amount.rs

@@ -499,7 +499,7 @@ impl Amount {
    pub const fn from_sat(satoshi: u64) -> Amount { Amount(satoshi) }

    /// Gets the number of satoshis in this [`Amount`].
-    pub fn to_sat(self) -> u64 { self.0 }
+    pub const fn to_sat(self) -> u64 { self.0 }


Just an observation; in a way, this implicitly commits us to the internal representation used in Amount because we are committing to never using non-const code to get the sats equivalent of the inner representation. In this case I doubt we will ever use anything other than a u64 but for other types this may be something to keep in mind?

It commits us into never making representation so obscure that this would require a syscall (note: allocators also make syscalls sometimes). I can't imagine the drug that would make us so insane to do this for Amount.

I also don't see it as likely for other types. I've recently realized that there's no IO in bitcoin except for communicating with peers and storing data both of which we don't do so the only things that may become non-const are those that would start unconditionally allocating. For instance if a new address variant is added that has to allocate - but even then we can construct all other variants.

Oh, crap, there's another reason: language limitations. They may make us unable to add some features on current MSRV.

Yes, but that only means that we can't change the representation of Amount without changing the MSRV. So in some sense it's not a permanent limitation, and will inconvenience us at most.

tcharding · 2023-04-26T01:25:51Z

bitcoin/src/blockdata/weight.rs

+        match kwu.checked_mul(1000) {
+            Some(weight) => Some(Weight(weight)),
+            None => None,
+        }


Kind of sad, this feels like a coding regression to not be able to use map. I guess that is a language problem.

Indeed. :( Will get fixed in maybe 10 years...

May be worth commenting something to the effect of "can use map when MSRV allows it in constfns". Maybe not. Maybe we should just audit all the constfns every time we do a MSRV bump.

Such a comment seems noisy but on the other hand if it includes "MSRV" in it then it may remind us at MSRV bump time to do it since grepping for MSRV is likely to happen at that time.

These code snippets also aren't horrible and usually contained in a very small method. So I wouldn't sweat it too much.

That MSRV bump will come in maybe 5 years. const FnOnce trait needs to exists before that happens and there's currently hot debate going on about how to do it. It won't stabilize anytime soon. So "MSRV" is misleading, it's a (hopefully temporary) language limitation.

Kixunil added enhancement API break This PR requires a version bump for the next release labels Apr 1, 2023

Kixunil force-pushed the big-constification branch 3 times, most recently from 2bd58a4 to 67b3784 Compare April 2, 2023 06:14

jeandudey reviewed Apr 19, 2023

View reviewed changes

bitcoin/src/address.rs Outdated Show resolved Hide resolved

Kixunil force-pushed the big-constification branch from 67b3784 to e9f4150 Compare April 19, 2023 16:00

Kixunil marked this pull request as draft April 19, 2023 16:01

Kixunil force-pushed the big-constification branch from e9f4150 to 3578dc1 Compare April 24, 2023 07:14

Kixunil added 4 commits April 24, 2023 09:31

Add missing #[inline] attributes

3e8766f

These functions were trivially calling another function so `#[inline]` is useful.

Make {to,from}_raw_hash methods const

dc00cc7

We are sure these will never do syscalls.

Make {to,from,as}_byte_array methods inherent

507e070

This removes the requirement of importing `Hash` trait for trivial conversions and allows the methods to be `const`. The original methods remain on the trait and do the same thing.

Constify Amount, Weight, and FeeRate

30c838d

Methods of these can be obviously `const` but need to handle errors using `match`/`if` instead of combinators.

Kixunil force-pushed the big-constification branch from 3578dc1 to 30c838d Compare April 25, 2023 09:14

Kixunil marked this pull request as ready for review April 25, 2023 09:15

tcharding reviewed Apr 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constify as much as possible #1770

Constify as much as possible #1770

Kixunil commented Apr 1, 2023

apoelstra commented Apr 2, 2023

Kixunil commented Apr 2, 2023

Kixunil commented Apr 19, 2023

stevenroose commented Apr 19, 2023

Kixunil commented Apr 24, 2023

Kixunil commented Apr 25, 2023

tcharding left a comment

tcharding Apr 26, 2023

Kixunil Apr 26, 2023

tcharding Apr 26, 2023

Kixunil Apr 26, 2023

tcharding Apr 26, 2023

stevenroose Apr 30, 2023

tcharding Apr 26, 2023

Kixunil Apr 26, 2023

Kixunil Apr 26, 2023 •

edited

apoelstra Apr 26, 2023

tcharding Apr 26, 2023

Kixunil Apr 26, 2023

apoelstra Apr 26, 2023 •

edited

tcharding Apr 26, 2023

stevenroose Apr 30, 2023

Kixunil Apr 30, 2023 •

edited

Constify as much as possible #1770

Are you sure you want to change the base?

Constify as much as possible #1770

Conversation

Kixunil commented Apr 1, 2023

apoelstra commented Apr 2, 2023

Kixunil commented Apr 2, 2023

Kixunil commented Apr 19, 2023

stevenroose commented Apr 19, 2023

Kixunil commented Apr 24, 2023

Kixunil commented Apr 25, 2023

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Apr 30, 2023 • edited

Choose a reason for hiding this comment

Kixunil Apr 26, 2023 •

edited

apoelstra Apr 26, 2023 •

edited

Kixunil Apr 30, 2023 •

edited