dbb5917128 Automated update to Github CI to rustc nightly-2024-07-21 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK dbb5917128
Tree-SHA512: 6f9a7939ff2eea6d07338cb75e393e319b2d38009dbad35e408bd23534e6240ef8c95a268b2316f7b81045eba7031e4269918a3841fe3353da966605ea752407
0beefda7ea CONTRIBUTING: error/expect messages (Jose Storopoli)
Pull request description:
- adds an item that all error messages
should be lower case, except for proper nouns and variable names.
- adds a section on expect messages,
following discussion in #3019 and #3053.
Closes#3053.
ACKs for top commit:
tcharding:
ACK 0beefda7ea
Kixunil:
ACK 0beefda7ea
apoelstra:
ACK 0beefda7ea
Tree-SHA512: b924245e5d18d4f627d4998073422003def789c87502bca121d5db443f5bbe387bfe951664458c9495a6ea0995b6ccf6e28d1784e19dfb876d7eddf3c6209727
ab581a90f8 Remove re-export of ParseIntError (Tobin C. Harding)
Pull request description:
In d242125 I claimed that `ParseIntError` was somehow special, I no longer thing this is the case. As we pin down the re-export policy (for errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to discuss the policy, for now just remove the unusual re-export.
ACKs for top commit:
Kixunil:
ACK ab581a90f8
shinghim:
ACK ab581a90f8
apoelstra:
ACK ab581a90f8
Tree-SHA512: 5ac4123aeb27c8cee78e5760f21e70be8035d526ba7e14e72759cba27f98b51cc2cba9b2bf0eeb99e0f6b7210ec4a750986bb6c5dc0725ed892730fdec8a7e06
54c30556a2 Move params to network module (Tobin C. Harding)
045a661ebe Create network directory (Tobin C. Harding)
Pull request description:
Discussed in #2779. Patch one moves `network.rs` to `network/mod.rs`, and patch 2 moves the `params` module over there.
ACKs for top commit:
apoelstra:
ACK 54c30556a2 Yeah, this seems like a good place for it
Kixunil:
ACK 54c30556a2
Tree-SHA512: 134813419db21323d303d465b12fcbf37fd61dc1baf3305e156d324eafd822379e63dede02877ee99dce41540193a29e6e13acd13f9121f3e2fe11096524aa5e
04bcb7950d Automated update to Github CI to rustc nightly-2024-07-17 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 04bcb7950d
Tree-SHA512: 2b2a02fd5eb5dfcc1e8b1551ef66180d76019655262a5eebeb6e849885db5e37c495cd9d1108ce550b9401c446a5abf2ca30508eb3d1139a807188b668d232ef
386ad93253 Manually format function parameters (Tobin C. Harding)
871f4398b9 Add optional comma to function parameter list (Tobin C. Harding)
Pull request description:
During #2955 I was lazy and did not think through why the macro didn't handle function parameters on individual lines, I just manually re-formatted all function calls onto a single LOC. This was a bit slack of me.
- Patch 1: Fix the macro.
- Patch 2: Revert the manual formatting.
ACKs for top commit:
jamillambert:
ACK 386ad93253
Kixunil:
ACK 386ad93253
apoelstra:
ACK 386ad93253
Tree-SHA512: 577154668a4cd0bad225a5b306193a5cd3d38d275dd5df22b4a29737e0d2c910c695da33c0ed77265f6c5d876c162316e444f8eca80f9c30788d110d989493e3
beea3c1e5d Remove public error re-export (Tobin C. Harding)
Pull request description:
We do not have a policy to re-export things from other modules just because they are in the public API - I don't see any other reason to re-export this error, users should go to the `validation` module directly to get the error type.
Raising this trivial change as a separate PR so that we can really pin down our re-export policy. Please review the policy implications as well as the code change.
Note please that this change was introduced in 7d695f6b4 by me, and buried in a PR that did not mention the change. This was wrong, as in the code change was wrong and also the patching method was wrong.
ACKs for top commit:
Kixunil:
ACK beea3c1e5d
Tree-SHA512: 5fc072f3fb8a727f30751211c6bc85dc268d413ee62937c714bdf9f47405dfdbc93cfff3df76c201493c39f49d5d315907fc9e7e4fa0d927652c01038815fdc5
a76c13f675 Add more unit test coverage for relative LockTime (Shing Him Ng)
Pull request description:
Adding some test cases for `from_seconds_floor` and one more for `from_seconds_ceil`
ACKs for top commit:
tcharding:
ACK a76c13f675
Kixunil:
ACK a76c13f675
Tree-SHA512: 5d773a30fa56842af21109d232ec3eae7fde7ce38aca93dadccc7bfbf14d5207ea329ef737f847e45ba755fd22e1862c3e78e7e64a8a0babebcb0e27a2bb7a90
- adds an item that all error messages
should be lower case, except for proper nouns and variable names.
- adds a section on expect messages,
following discussion in #3019 and #3053.
In d242125 I claimed that `ParseIntError` was somehow special, I no
longer thing this is the case. As we pin down the re-export policy (for
errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to
discuss the policy, for now just remove the unusual re-export.
The `Params` struct is currently defined in the `consensus` module which
has become a collection of orthogonal consensus-ish things. We would
like to put things in more descriptive places.
The `Params` struct defines constants that are network specific so it
makes sense to put it in the `network` module. As soft proof of this
argument note in this patch how often the `Params` type is imported
along with the `Network` type.
API break:
The type is no longer available at `bitcoin::consensus::Params` but
rather is re-exported at `bitcoin::network::Params`.
We do not have a policy to re-export things from other modules just
because they are in the public API - I don't see any other reason to
re-export this error, users should go to the `validation` module
directly to get the error type.
6ab8f99731 ci: fix semver-checks master (Jose Storopoli)
Pull request description:
Ok now I think I've nailed this! 🚀Closes#3018.
## Summary
This PR fix the permissions by adding a secondary job, `semver-checks-pr-label`, that runs only if the `semver-checks` completes and does not fail, i.e. we have either Semver API breaks.
This is a secure job according to internal discussions and https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run
## Implementation notes
We split the semver checks into the actual check and labeling. This is due to the fact that we were having several permission issues and the original implementation was insecure. Also it is currently failing on several PRs.
1. `semver-checks.yml`: checks for API semver breaks. Upon finding API semver-checks, it creates a file named `semver-break` that has the PR number inside. If this file is present we upload an artifact representing that the API semver was broken with the PR number.
2. `semver-checks-pr-label.yml`: runs after the `semver-checks.yml` is completed. It uses `actions/github-script` (in JavaScript) to list the previous workflow artifacts and, if finds that an API semver break is present, adds a corresponding label and a comment to the PR that is inside the file `semver-break`. We sanitize the hell out of the thing to be a number only, both in BASH but also in JavaScript.
## How I tested this?
1. I merged this PR to my `master` branch in my `storopoli/rust-bitcoin` repo.
2. I've set the following settings in the repo settings to mimic what we have for `rust-bitcoin`:
3. I've made a PR (cherry-picked from Toby) that breaks semver, see https://github.com/storopoli/rust-bitcoin/pull/10. It correctly labelled as `API break` and added a comment to the PR
ACKs for top commit:
Kixunil:
ACK 6ab8f99731
apoelstra:
ACK 6ab8f99731 thanks for sticking with this\!\!
Tree-SHA512: 27385d6d4a40b5ffc3b95f00ad34010fabe5f1f9442d258b4cb433807bc663fc70b98c7c632048f05929cad073fd81ac6217f72016740b72db7575dac9d3939b
Now that the `define_extension_trait` can handle function parameters on
individual lines revert the manual formatting that was introduced in
PR #2955.
Refactor only, no logic changes.
The `define_extension_trait` macro currently does not allow for a
trailing comma in the function paramater list, this occurs when the
function paramaters are put on individual lines (eg by `rustfmt`).
Extend the macro to support function paramaters being on individual
lines. This should have been done originally instead of my manual
override of formatting - bad Tobin no biscuit.
86f82b487d Remove unused variable from LockTime example (Shing Him Ng)
Pull request description:
Following up on [a comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/3026#discussion_r1676744486) from #3026 and removing an unused variable from the LockTime example
ACKs for top commit:
tcharding:
ACK 86f82b487d
Kixunil:
ACK 86f82b487d
Tree-SHA512: 526d42e68297e9670953f7aadfbf624b7bc20f1bf95b3e5ff66a58871967e38eca917d9ee9c385cf40ed7d9a867cb081ef17f2aa3683138f6fc55a74414a83f6
2169b75bba Use lower case error messages (Jamil Lambert, PhD)
Pull request description:
Error messages should be lower case, except for proper nouns and variable names. These have all been changed.
~~They should also state what went wrong. Some expect error messages were positive, giving the correct behaviour or correct input. These have been changed so that they are now negative, i.e. saying what went wrong.~~
EDIT: After further discussion it was decided not to change the expect messages.
ACKs for top commit:
Kixunil:
ACK 2169b75bba
tcharding:
ACK 2169b75bba
Tree-SHA512: 92442c869e0141532425f6fca5195fd319b65026f68c4230a65ad70253565d98931b2b44ee202975c307280525c505147e272297dc81207312e40c43d007021c
7874db8fc3 Throw error instead of panic when from_second_ceil input is too large (Shing Him Ng)
Pull request description:
Update `from_second_ceil` per [comment](https://github.com/rust-bitcoin/rust-bitcoin/issues/3029#issuecomment-2227459248) and add unit tests
Resolves#3029
ACKs for top commit:
Kixunil:
ACK 7874db8fc3
tcharding:
ACK 7874db8fc3
Tree-SHA512: 0d60397f867d4536ff00b81441c27ea3dc4bf9e5ecc2fb5afbe50cb3153101df54c2bb5056da1916d83c3e84367f987f5bac07a381b7433a5701a0ea8a587f95
580c2cf246 Automated update to Github CI to rustc nightly-2024-07-14 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 580c2cf246
Tree-SHA512: 0b01575733d6b4d70be9b00d55d53670db33324498cef991e5beca052a3b59bcbb128351a677ab83773855b3b57cacd4c7f08e93079387d67229d1014666cfae
84dd04cf60 Rename variable assignment (yancy)
Pull request description:
The type created after assignment is a Weight type. Using a var name vb which is short for virtual byte is incorrect.
Pulled this out of stale PR 2215
ACKs for top commit:
shinghim:
ACK 84dd04cf60
apoelstra:
ACK 84dd04cf60
Kixunil:
ACK 84dd04cf60
Tree-SHA512: 1bfc875f53037b2c1e8da25fe44e9ca3303981bdce4e48661a8fb2061833e3cd90318d854f7119c805e861cd8a591697378f829f32eb74ac99a71dc4c947abde
301fe9fad9 Move impl above tests (yancy)
Pull request description:
It's common in other modules for the tests to be at the end of the file. Moving the tests to the bottom helps the code base uniformity.
Pulled from stale PR 2215
ACKs for top commit:
shinghim:
ACK 301fe9fad9
tcharding:
ACK 301fe9fad9
Kixunil:
ACK 301fe9fad9
Tree-SHA512: c749c4c2c5fba2c8a57fe034d4ef5d34ff26baddb1b2148b9778dc554c955ad31c7bc83b89a3647228bf5f175adddb03105a1e494b28f5a7955369abaa11cfd1
64c31cfb97 Move locktimes and Sequence to primitives (Tobin C. Harding)
Pull request description:
The `absolute` and `relative` locktimes as well as the `Sequence` are all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in `bitcoin` and we re-export everything from `blockdata`.
Note please `Sequence` is no longer publicly available at `bitcoin::transaction::Sequence` but it is available at `bitcoin::Sequence`.
ACKs for top commit:
Kixunil:
ACK 64c31cfb97
apoelstra:
ACK 64c31cfb97
Tree-SHA512: 968aa595bfb53e8fcfa860dae797ec5381ed49c67326a8ef9494086ec65d737502dffe4b24143e159042d07c59c5109d50103029082f87e1c3c26671e975f1b3
02e7933fe4 Rename key field in Key to key_data (Shing Him Ng)
Pull request description:
Follow-up change from [a comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/3022#issuecomment-2226166039) in #3022 that updates the `key` field in the PSBT `Key` to `key_data`, which more closely follows the doc
ACKs for top commit:
tcharding:
ACK 02e7933fe4
Kixunil:
ACK 02e7933fe4
Tree-SHA512: 67cc621e9f5b326f96c3f08e340acecf60b1d03d25bbd012998bab34f49e96c4dca9f5dc24047f743f1a1b6fcaa5d3c166f2e4cf2ea96d4375c9b1b041d3196f
2b56f763d0 hashes: Remove to/from/as_raw_hash (Tobin C. Harding)
Pull request description:
In an effort to shrink the API of `hashes` remove the `from_raw_hash`, `to_raw_hash`, and `as_raw_hash` inherent functions from types created with the `hash_newtype` macro.
There are a few reasons why this is favourable:
- It allows stable crates to use the macro and not expose unstable `hashes` types in their API.
- It makes types created with the macro less "general" in the sense that its more obscure to just hash any data into them. This allows us to write cleaner APIs in `rust-bitcoin`.
ACKs for top commit:
Kixunil:
ACK 2b56f763d0
apoelstra:
ACK 2b56f763d0
Tree-SHA512: 3d73aa8250dd775994623c9201dd819256acf2ec82526b3537da74c9e19c2ac5e8bba358a2171f7b02342804cb6b4d5ac4dca88d912b3d46d14e3bc35dd5cb91
The `absolute` and `relative` locktimes as well as the `Sequence` are
all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in
`bitcoin` and we re-export everything from `blockdata`.
164b72e07b Update Key documentation (Shing Him Ng)
Pull request description:
The documentation for `Key` was a bit confusing since `<key> := <keylen> <keytype> <keydata>` was right above the `key` field, when in reality the `key` field represents the key data. Moving the documentation that describes a `Key` as a whole to the struct itself and specifying that `key` represents key data will hopefully clear things up a bit
ACKs for top commit:
Kixunil:
ACK 164b72e07b
tcharding:
ACK 164b72e07b
Tree-SHA512: 33c2b24d3006a0ed85a5d96351e0a01c1365c8ea01472233b024de54941319cdbefa0126f8b9538385e8f33ba7e2e3895f0dc5b6a36a1c501c8b97ebbede6502
8dcecfc144 Remove midstate from the GeneralHash and HashEngine traits (Tobin C. Harding)
Pull request description:
Midstates are not generic objects; they don't have universal cryptographic properties and if you are using them you should be using a specific midstate type. Therefore it shouldn't be part of `GeneralHash` or `HashEngine`. Furthermore, in practice it seems like `sha2` midstates are the only ones that anybody uses, at least in bitcoin.
Remove the midstate stuff from the `GeneralHash` and `HashEngine` traits. Keep the `midstate` functionality as inherent functions if it is used internally. Keep the functionality on `sha256` as inherent public functions.
Done as a first step towards #2918.
ACKs for top commit:
apoelstra:
ACK 8dcecfc144
Kixunil:
ACK 8dcecfc144
Tree-SHA512: 495fe6a4d852ecf51c21acc1ae7215f1914f6d12377eb7e6da678b8ffb2c3c8b06976aa5290010a88d5a9cbc56752fcbdf740cb27b22ada8079a69dcf2e32dc0
bcf6d2839e Introduce scriptPubkey extension traits (Tobin C. Harding)
ee333defa4 Remove path from ScriptBuf (Tobin C. Harding)
Pull request description:
Done in preparation for moving the script types to `primitives`.
Add three public traits, one each for the three `script` types: `Builder`, `ScriptBuf`, and `Script`.
ACKs for top commit:
Kixunil:
ACK bcf6d2839e
apoelstra:
ACK bcf6d2839e
Tree-SHA512: 0ad11e474ddf1183d0119e36454cb4fd18d49a68655d274df800c6ef20afa7f8d0fdecd415c02595ea67a011e3a842b7ccc23c2d58f92ed9acbdc7f277fbd217
e2b0fd33be Specify required_height in variable name when comparing to other height (Shing Him Ng)
Pull request description:
Update the variable names for `h`/`height` to `required_height` in instances where it was being used to create another height or used comparatively i.e. `required_height + 1` or `required_height < height`
Resolves#2553
ACKs for top commit:
Kixunil:
ACK e2b0fd33be
tcharding:
ACK e2b0fd33be
Tree-SHA512: 05fa8df721148d9bbc6eaa2b14c2583d3415c3ab0d53b8fdf39d7b9e9a28463265e43abfc430dfc7f6d0b53cd17767f5cad83c2de73b8a153aeb6befeeac84bc
In an effort to shrink the API of `hashes` remove the `from_raw_hash`,
`to_raw_hash`, and `as_raw_hash` inherent functions from types created
with the `hash_newtype` macro.
There are a few reasons why this is favourable:
- It allows stable crates to use the macro and not expose unstable
`hashes` types in their API.
- It makes types created with the macro less "general" in the sense that
its more obscure to just hash any data into them. This allows us to
write cleaner APIs in `rust-bitcoin`.
merge-script
Tobin C. Harding
Update Nightly Rustc Bot
Jose Storopoli
Shing Him Ng
yancy
Jamil Lambert, PhD
Ryan Breen