While the hash value of the Error variant is meaningless, the variant
still conforms to all other Inventory messages and requires a 32
byte hash to be sent over the wire. This is how bitcoin core operates.
This patch adds the 32 byte array to the Error variant in order to make
its Encoding and Decoding paths symmetrical. This also allows a reader
to discard the 32 bytes when decoding a message. The hash is still not
exposed to the caller.
This was never a problem before because the top level RawNetworkPackage
pulls all the required bytes off a reader before decoding. But this is
not as easy to do with the v2 p2p network messages.
f4617e71f5 kani: Verify no out of bounds for ArrayVec (Tobin C. Harding)
e378cdd8fa kani: Don't bother checking signed to unsigned conversion (Tobin C. Harding)
50224eecc2 kani: Don't overflow the tests (Tobin C. Harding)
Pull request description:
PR does two things because a recent upgrade of `kani` broke our setup. I'm not sure why it just showed up.
We fix the verification for `amount` types which recently broke because of `MAX_MONEY` changes and then we add a verification function to `internals` because build fails and one fix is to just add something.
- Patch 1: units: Assumes correct values
- Patch 2: units: Removes a stale check now that MAX_MONEY is being used for MAX
- Patch 3: Add verification to `internals::ArrayVec`.
ACKs for top commit:
apoelstra:
ACK f4617e71f5fd074000d1e3a9376644c744210562; successfully ran local tests
Tree-SHA512: dfef05a7bbb5372415efa8acab7f79801aa7326ac298c007b173786f00bcccd0b1b81d327113723c359fb2797895414a586cc3fb86e495476a03fcac02a96899
I'm not super confident that I know exactly what kani does but I believe
this test verifies that the `ArrayVec` can add and access elements less
than capacity and upto capacity.
f08d8741d3 Test types MIN/MAX instead of i64::MIN/i64::MAX (yancy)
Pull request description:
The MIN/MAX for SignedAmount recently changed from i64::MIN and i64::MAX to MAX_MONEY/MIN_MONEY. Update the tests to reflect this new MIN/MAX since it is no longer valid to create a value above or bellow MAX_MONEY/MIN_MONEY.
ACKs for top commit:
apoelstra:
ACK f08d8741d39685b636830680bb891bd414826e88; successfully ran local tests
tcharding:
ACK f08d8741d3
Tree-SHA512: 563408240dffaf95f88a9d570e56f9b9b161b422cb59a89828c18b9c784e7acb717f57fe55c80411f104443ac2a3f908f2a98ab1a4b34edab69b6946a723b30c
2513e05501 api: Run just check-api (Tobin C. Harding)
9619f68956 units: Move excluded lints to manifest (Tobin C. Harding)
5290a93a38 units: Add all pedantic lints (Tobin C. Harding)
Pull request description:
Add all pedantic clippy lints but leave a few marked as TODO.
The coment on the list claims to it is an exhaustive list of pedantic lints, that claim is going to go stale. I would be nice if we had tooling to catch new lints as they were added.
ACKs for top commit:
apoelstra:
ACK 2513e05501e3a014c097f24eb9178c291785db81; successfully ran local tests
Tree-SHA512: 33b2a7448d49d6a5571c9e4e9922b6042ab03aaaa9f7acad243a926f8a03a0ffed75d4f5f37be4705f23862c32f96879582214cd17c7e5ab81e47517a84745e0
01af266335 Remove deprecated tests (yancy)
Pull request description:
Unchecked methods have been removed in a previous commit, therefore the tests are no longer applicable.
ACKs for top commit:
tcharding:
ACK 01af266335
apoelstra:
ACK 01af2663356f9cf1f00e7aa0359aca7b9515e96a; successfully ran local tests
Tree-SHA512: f45049750b3b3d2b6528cb9c892cf6ba9a95bd3fd30e375bb6c22722b0e46bcaddf315b80e6255b8f2451d250adf5536d7b3d91eb5ae4e496782cc75940674ea
Amount add and sub now enforce the MAX_MONEY invariant when doing
addition and subtraction. We need to tell kani to assume we don't
overflow before doing actual tests.
Note also that `ops::Add` calls through to `checked_add` and
`ops::Sub` calls through to `checked_sub` so separate kani tests for
these are unnecessary.
Add all the pedantic lints to the repository by way of the repository
manifest. Then enable these lints in the `units` manifest.
Some things worth mentioning:
- Fix `needless_pass_by_value` by adding derives to `FormatOptions`.
- Fix lint `cast_lossless` using `cargo clippy --fix``
- While fixing `lint enum_glob_use` introduce a new style to the
codebase; import enums using a single character. Doing so prevents
namespace clashes, improves clarity, and maintains terseness.
Audit:
Use the following lints locally and audit all the warnings, they produce
many false positives so we can't enable them permentently.
- `cast_possible_truncation`
- `cast_possible_lint`
- `cast_sign_loss`
The MIN/MAX for SignedAmount recently changed from i64::MIN and
i64::MAX to MAX_MONEY/MIN_MONEY. Update the tests to reflect this new
MIN/MAX since it is no longer valid to create a value above or bellow
MAX_MONEY/MIN_MONEY.
e56f461916 Make capitalization of SegWit uniform in strings (Jamil Lambert, PhD)
3520e832ac Make capitalization of SegWit uniform in rustdocs (Jamil Lambert, PhD)
Pull request description:
Fixes#3770
- Searched and replaced all occurrences of `//` * `segwit` (case insensitive) with `//` * `SegWit`
- Searched and replaced all occurrences of `"` * `segwit` (case insensitive) with `"` * `SegWit`
ACKs for top commit:
apoelstra:
ACK e56f461916b85928139950074d0df52caf4f919b; successfully ran local tests
Tree-SHA512: c56704102d8e86f26378bb302d5fdc7ea21d41fd49f55606e589ec32ff896f1adfb1960d8fb29abccfbf298b90fc357ed609d80fd0163dcb4ff01573646b02c3
b5f553d866 hashes: Bump version to 0.16.0 (Tobin C. Harding)
Pull request description:
We need to do a quick release because our current `bitcoin` manifest does not work with hashes `v0.15.0` and we'd like to release an alpha version with the current state of `master`. https://github.com/rust-bitcoin/rust-bitcoin/pull/3718
```
the package `bitcoin` depends on `bitcoin_hashes`, with features: `hex`
but `bitcoin_hashes` does not have these features. It has a required
dependency with that name, but only optional dependencies can be used as
features.
```
Add a changelog, bump the version, depend on the new version repo wide, and update the lock files - like its our job.
ACKs for top commit:
sanket1729:
utACK b5f553d866
apoelstra:
ACK b5f553d8662836e2bf4cd8fc134e5538782b37f6; successfully ran local tests
Tree-SHA512: dfd85a658a7274e934bd9508a7529a4076a34a922c12ac97d952302a3e7e2d60ac3fca676a2c950482cec36f6c4818a9fd0cec8935be2783fa193cb90701f371
68f3c3e5d7 api: Run just check-api (Tobin C. Harding)
b956940a6d Add ops unit tests for amount types (Tobin C. Harding)
d88306fb68 units: Implement ops for amount types (Tobin C. Harding)
4a3aa4a08b Add ops unit tests for Weight (Tobin C. Harding)
43ef9a618c units: Implement ops for Weight (Tobin C. Harding)
c5fbc7e117 units: Add assign macros (Tobin C. Harding)
20a79da344 units: Use one level of path for ops traits (Tobin C. Harding)
6ce5385914 units: Add macros for Add and Sub (Tobin C. Harding)
b31ca72b33 units: Use rhs instead of other (Tobin C. Harding)
cd0fa2d1dc Make FeeRate ops impls more terse (Tobin C. Harding)
Pull request description:
Sort out `Add`, `AddAssign`, `Sub`, and `SubAssign` for the following types:
- `Amount`
- `SignedAmount`
- `FeeRate`
- `Weight`
And clean up as we go.
Note that `BlockInterval` isn't touched in this PR - it looks correct already.
The unit tests in the two patches "Add ops tests ..." can be moved if you want to verify that they don't build without the patch they follow.
ACKs for top commit:
apoelstra:
ACK 68f3c3e5d728a60a4b52ca64523770bcdd32f957; successfully ran local tests
Tree-SHA512: a82d3bf288f61b169b8cff498e81bd2cd123c8dcbf534413233ff03f06102a42508e09b2f7e5b268b21f82d4bf2b3612cd88dea1231b4d3e6455c7e99f82e729
f01f071751 api: document the need of cargo nightly (Jose Storopoli)
ff67eadc7f contributing: clarify API changes (Jose Storopoli)
04852958b9 contrib: check if the user has cargo-public-api (Jose Storopoli)
Pull request description:
Closes#3764.
yancyribbens can you test it locally? It works in my end both with and without `cargo-public-api`. Of course without gives the error message that I was supposed to see.
EDIT: You can run it with `just check-api`.
ACKs for top commit:
apoelstra:
ACK f01f071751332bb6aa8b9affa4677f913035de7f; successfully ran local tests
tcharding:
ACK f01f071751
Tree-SHA512: 7acf8332731191de47afc02e24c151bc53c1f38685217eccbea1b64c216674f31729ba703e132765cba67183dff95757aa949d653c5cbe538240371c2db52c14
183ecf1fe6 Add units tests for cBTC (Tobin C. Harding)
Pull request description:
We added the "cBTC" denomination but forgot to test it.
Add units test for "cBTC" as we do for the other denominations. Put all the tests lines of code in the same order as the enum variants so a reader can see easily that all cases are tested.
ACKs for top commit:
apoelstra:
ACK 183ecf1fe6944187151036e36d6726fae9b64392; successfully ran local tests
Tree-SHA512: 93a3f3e5430f7b6f525d7c55ad3d318f8b30fef8cdef24ea92a1687e898f73d3829875861f026af70dde013743d310d87331b5654b524f4ee90c20f7e6ed7f7c
Add macros for implementing `ops::Add` and `ops::Sub` for various
combinations of references. Use the macro in `fee_rate`.
These are internal macros so no need to protect `core` using
`$crate::_export::_core`.
815330dad2 Clean up weight unit tests (Tobin C. Harding)
3f4365ee0c api: Run just check-api (Tobin C. Harding)
75594c7299 Add Weight::to_kwu_ceil (Tobin C. Harding)
Pull request description:
It is not immediately obvious why we have floor and ceil versions of `to_vbytes` but not for `to_kwu`.
Add a conversion function to get weight by kilo weight units rounding up.
And then clean up the `weight` unit tests.
ACKs for top commit:
apoelstra:
ACK 815330dad2d2026ecf11c998dd560217c8d05d52; successfully ran local tests
Tree-SHA512: 8ba6c255fb9a3bc4a3dd485d6875663976870805a639b0aa309727fd211460029d05154351522d2b753afd478df1187abd92b212512b140c1ddb24b34b7e2bc0
f97c04cf07 api: Run just check-api (Tobin C. Harding)
99b32c95b8 Change paramater type used for whole bitcoin (Tobin C. Harding)
Pull request description:
We have multiple constructors for `Amount`, one of which accepts an integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type used for whole bitcoin to `u32` instead of a `u64` which may well be mapped to sats in the mind of developers (it is in mine anyway).
ACKs for top commit:
apoelstra:
ACK f97c04cf07c2ac5acb5d312b573d100afa009b57; successfully ran local tests
Tree-SHA512: fb22f08f182a96ff8d067337e32174137e3d73acd2faeddc2bac974ea41f8de9ec06baa2e6fb76a2fa1a7afa7bf6b4c8f606666d966413a14d39b129e36d0674
b895c9aad4 units: Deprecate unchecked ops (Tobin C. Harding)
Pull request description:
These functions do not add value because one can just call `to_sat` and `from_sat` if doing ops in a tight loop.
Note we need to remove these deprecated functions before we release `units v1.0`.
ACKs for top commit:
apoelstra:
ACK b895c9aad4e56ff7cf5420f71ae501f942a81326; successfully ran local tests
Tree-SHA512: c42a177aa955cbddf523e0acd2385a0cece468ec9c8a0d1fd1cd8ceb085b9fad34c79d52cf2f16d94945bf2ae2f6934fac8e9efc879a649670bc48c20bd0a344
We added the "cBTC" denomination but forgot to test it.
Add units test for "cBTC" as we do for the other denominations. Put all
the tests lines of code in the same order as the enum variants so a
reader can see easily that all cases are tested.
We have multiple constructors for `Amount`, one of which accepts an
integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type
used for whole bitcoin to either `Into<u64>` or `u32` instead of a
`u64` which may well be mapped to sats in the mind of developers (it
is in mine anyway).
It is not immediately obvious why we have floor and ceil versions of
`to_vbytes` but not for `to_kwu`.
Add a conversion function to get weight by kilo weight units rounding
up.
These functions do not add value because one can just call `to_sat` and
`from_sat` if doing ops in a tight loop.
Note we need to remove these deprecated functions before we release
`units v1.0`.
We need to do a quick release because it turns out I was wrong in
thinking that making `hex` an optional dependency is not a breaking
change.
```
the package `bitcoin` depends on `bitcoin_hashes`, with features: `hex`
but `bitcoin_hashes` does not have these features. It has a required
dependency with that name, but only optional dependencies can be used as
features.
```
Add a changelog, bump the version, depend on the new version repo wide,
and update the lock files - like its our job.
3ed4acdd43 api: Run just check-api (Tobin C. Harding)
Pull request description:
Lets see if we can get `master` working before the week starts in the US. I take full responsibility for this weeks turmoil. My bad.
ACKs for top commit:
apoelstra:
ACK 3ed4acdd434df5e9ee3b71e4e99c4693b220daf5; successfully ran local tests
storopoli:
ACK 3ed4acdd43
Tree-SHA512: 1608dff2bf8741a48fdbcac14cd65c4c01da4070438789f2db128dd6de834227653a2e0c38ae5f67dee90826eacb6193cf43f9161f7c69aa2bae0bf2179aa3ae
f85456a726 Improve rustdocs on addresses module (Tobin C. Harding)
Pull request description:
These docs are stale, update them.
- Mention segwit and legacy
- Improve example code to show feature gating
- Fix headings to be as typical
ACKs for top commit:
storopoli:
ACK f85456a726
apoelstra:
ACK f85456a726a201b26eac58d22eb0eb6a58ad411b; successfully ran local tests
Tree-SHA512: c1cdde744fb960c4119805e6b3fb0cc971eaa94173fb85f8f7a34daab7985a0f14c9be477d7dda430c6873fed7265c598b8dcfbffe0ed6a99ca8590a2b2b724e
55092aab47 Use Self in amount consts (Tobin C. Harding)
Pull request description:
Currently the consts in the `amount` modules repeat the type. For `Amount` this isn't a big deal but for `SignedAmount` its quite noisy.
Use `Self` as the type in const declarations inside `Amount` and `SignedAmount`.
Internal change, no logic changes.
ACKs for top commit:
apoelstra:
ACK 55092aab47051e740c1ee43de73e367a2f9299a3; successfully ran local tests
Tree-SHA512: 8220541f3bac85f0a40382e9545500709dac6d96496cfae76c0ff41afd937b6452e3fa12390b38f80e586a2b52baa384c580c5fe3e8ac9e80ea394b3b3ee26db
Jamil Lambert, PhD
Nick Johnson
merge-script
Tobin C. Harding
yancy