Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support weight prediction in const context #1710

Merged
merged 2 commits into from Mar 19, 2023

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Mar 16, 2023

Notes for reviewers:
This is something that I want to use in my code and hopefully reasonably easy to review, so if this can get into 0.30 that'd be really nice. No hard feelings if it doesn't.
I tried to put extra effort into making review easier by:

  • intentionally "mis-formatting" the first commit so diff is smaller and easy to understand - see individual commits.
  • copying patterns from non-const fn to const fn so it's obviously correct (includes same variable names)
  • not bothering with the array trick in VarInt::len and simply accepting the limitation of Rust 1.46+ (I use 1.48 BTW).

Description

Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a const but our code didn't
support it until now.

This change adds methods that can compute the prediction in const
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).

As a side effect of this, the change also adds const to VarInt::len
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Mar 16, 2023

The failed test should be able to run tomorrow.

@Kixunil Kixunil mentioned this pull request Mar 16, 2023
8 tasks
apoelstra
apoelstra previously approved these changes Mar 17, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 3790d13

@Kixunil
Copy link
Collaborator Author

Kixunil commented Mar 17, 2023

Just removed the empty line to satisfy rustfmt.

@Kixunil Kixunil mentioned this pull request Mar 17, 2023
@Kixunil Kixunil force-pushed the const-tx-weight-prediction branch 2 times, most recently from 512c6cf to 637bd94 Compare March 17, 2023 04:55
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.

This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).

As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.

Note: this commit is intentionally unformatted to make diff easier to
understand. Formatting will be done in future commit.
This fixes indentatiion that was intentionally "messed up" to make code
review easier.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 00b46d6

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 00b46d6

@apoelstra apoelstra merged commit e7521fa into rust-bitcoin:master Mar 19, 2023
22 checks passed
@Kixunil Kixunil deleted the const-tx-weight-prediction branch March 19, 2023 16:40
apoelstra added a commit that referenced this pull request Mar 22, 2023
ffee8ad Bump version to v0.30.0 (Tobin C. Harding)

Pull request description:

  Add changelog notes and bump the version number to v0.30.0.

  ## TODO - pre-merge

  - [x] Release `bitcoin_hashes` 0.12: #1694
  - [x] Release secp 0.27: rust-bitcoin/rust-secp256k1#588
    - rust-bitcoin/rust-secp256k1#590
  - [x] Update `secp256k1` dependency to use newly released v0.27: #1714
  - [x] Merge
    - ~#1696
    - #1695
    -  #1111
  - [x] If time permits merge these:
    - #1710
    - #1705
    - #1713
  - [x] Set the release date in changelog header
  - [x] And merge these:
    - #1721
    - #1720
    - #1719
    - #1717

  ## TODO  - post release
  - [ ] Release the blogpost: rust-bitcoin/www.rust-bitcoin.org#2
     - ~Set the date in the blog post to match the date 0.30 is released~

ACKs for top commit:
  sanket1729:
    reACK ffee8ad
  Kixunil:
    ACK ffee8ad
  apoelstra:
    ACK ffee8ad

Tree-SHA512: b0ea113ee1726fd9b263d0e01fe14bd544c007c05a9ac43b6c2d4edbeef3bb3ad456b061ef086626e1e1b27a0cda49cb6bc28aac3ad1691d72ffe00400ed5b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants