d9cf7270eb Move `bitcoin/p2p` into `p2p` (rustaceanrob)
Pull request description:
Could use a rebase on #4568
Somehow lost the branch of my original draft so some of the review context is removed, but all comments were addressed in separate PRs.
First commit moves everything required to implement encoding within `p2p`, so we have a reference for functions to add to future moves to `internals`. Second commit does the relocation and third commit removes the `bitcoin/p2p` module.
ACKs for top commit:
apoelstra:
ACK d9cf7270eb457fd660fa505701895ab4756e394d; successfully ran local tests
tcharding:
ACK d9cf7270eb
Tree-SHA512: 31bf960788f45bb60b04dd73793a06828a5540e1e6118376776494bad9330dc8ebcb57749bc84b70a00e4d1d2a5686506e089269833ed99c4f9fa0c3e3b2e5b7
Moves all of the content from `bitcoin/p2p` into `p2p`.
`TryFrom<Network>` must be implemented for `Network -> Magic` now that
`Network` is a foreign, non-exhaustive type. Ser/de test is updated
accordingly, as well as the `Magic::from_network` constructor, which I
have opted to return an `Option<Self>`. The `TryFrom` implementation
uses a new `UnknownNetworkError(Network)` that mirrors the `Magic ->
Network` error.
The example handshake does not generate a random nonce for the version
message, as there is no `rand` dependency in this crate and the program
only makes a single, user-defined connection.
It appears we can do better than copying and pasting the consensus
encoding macros and functions from `bitcoin` without doing weird
cross-crate macros that require some special knowledge of the crate to
know when they will compile.
4284deed29 DerivationPath: support 'h' in Display output for hardened components (vicjuma)
Pull request description:
DerivationPath now supports a display format using both 'h' a single quote (') to indicate hardened components.
This aligns it with its ChildNumber's output style.
Resolves: #4618
ACKs for top commit:
apoelstra:
ACK 4284deed29114c5e31ef7c29e28e352b860f74e7; successfully ran local tests
tcharding:
ACK 4284deed29
Tree-SHA512: 24e053c22ec94b851935debbab83d15ad9f41ccfca0a7c34a061450989b176295512e8ffb187b2a54c6c6926f82ec7e4a4186ccdba2ec2616a6e0603d90d2a9b
0ff8d82193 Make FeeRate from sat constructors infallible (Tobin C. Harding)
3b0286bd56 Return NumOpResult for FeeRate and Weight (Tobin C. Harding)
15065b78c7 Return NumOpResult when calculating fee on Amount (Tobin C. Harding)
75106e6d82 Remove checked_ prefix from fee functions (Tobin C. Harding)
Pull request description:
This was 14dc950b54 from #4610.
Remove the `checked_` prefix from `fee` functions then make them all return `NumOpResult`.
ACKs for top commit:
apoelstra:
ACK 0ff8d82193ef562c1aedabe98a6283d09f803c0c; successfully ran local tests; nice!
Tree-SHA512: 62f2af6701f2a0e17b9f0a0ee132ca4a9289fe00e22c94f746602ed3c1f3758bb22323b224362cb659432ff5297391f3106ee460e9a8f65a39f904d72bc98aeb
We now have constructors that take an arbitrary size fee
rate (`Amount`). The `from_sat_per_foo` constructors can be made
infallible by taking a `u32` instead of `u64`. This makes the API more
ergonomic but limits the fee rate to just under 42 BTC which is plenty.
Note we just delete the `from_sat_per_vb_u32` function because it is
unreleased, in the past we had `from_sat_per_vb_unchecked` so we could
put that back in if we wanted to be a bit more kind to downstream. Can
be done later, we likely want to go over the public API before release
and add a few things back in that we forgot to deprecate or could not
for some reason during dev.
Fuzz with a new function that consumes a `u32`.
Currently we call the `Amount` fee calculation functions `div_by_foo`
and return an `Option` to indicate error. We have the `NumOpResult`
type that better indicates the error case.
Includes a bunch of changes to the `psbt` tests because extracting the
transaction from a PSBT has a bunch of overflow paths that need testing
caused by fee calculation.
A bunch of changes have been implemented lately in the fee calculation
logic. As a result of this a at once time `unreachable` statement is now
reachable - bad rust-bitcoin devs, no biscuit.
Saturate to `FeeRate::MAX` when calculating the fee rate, check against
the limit arg, and win.
Co-developed-by: Andrew Poelstra <apoelstra@wpsoftware.net>
aa76108315 bitcoin: secp256k1 global-context feature re-export (Jose Storopoli)
Pull request description:
Often we want to have the `global-context` feature in `secp256k1` without having to add manually the `secp256k1` dependency and enabling the `global-context` feature.
Having the ability to do that directly from `bitcoin` without having to add `secp256k1` and do the whole tango of tightly coupling the two dependecies versions together, e.g. `bitcoin` `0.32.x` and `secp256k1` `0.29.x` would be really nice and would also simplify a lot code maintainability for anyone who depends on bitcoin.
This needs to be backported to `0.32.x`, which I'll gladly do as well.
ACKs for top commit:
luisschwab:
ACK aa76108315
tcharding:
ACK aa76108315
Tree-SHA512: ff0e0016a6bf7cadf42cb40857393acace538fc3fb5214b89d4222a2fe570e9ae79617ae276273eefc34c3b68a64100bb22a9c01f9075cc3ed0705d7a264b9b3
New clippy lint in rustc nightly "lifetime flowing from input to output
with different syntax can be confusing".
Apply the suggested fix and use the anonymous lifetime for paths.
ebb9861c58 Add backticks around OPCodes (yancy)
Pull request description:
Item in documentation is missing backticks
ACKs for top commit:
tcharding:
ACK ebb9861c58
apoelstra:
ACK ebb9861c58419c1f2e3cce2ef4ecdfa1dee144e0; successfully ran local tests
Tree-SHA512: 1a06db90c026544efda2b248d2e1fd2ca49af9e9828c52b427077edf65e8a6fa789b691e9411d704459a55aae7f28b4bd784a493e89506b83c872b4ecbb9fe1e
dd3f3e44bc Split into_derivation_path tests out (Tobin C. Harding)
0c9dd31f53 Test with m prefix (Tobin C. Harding)
c5073f4c51 Refactor simple valid path tests into a loop (Tobin C. Harding)
3e7fdad5fd Split empty master test out (Tobin C. Harding)
ed36a980f8 Refactor invalid derivation path tests (jamillambert)
015fb1be3b Split invalid derivation path test (jamillambert)
76dd6100a2 Split derivation path test into valid and invalid (jamillambert)
Pull request description:
Refactor tests like its our job.
Co-developed-by: Jamil Lambert <jamil.lambert@proton.me>
ACKs for top commit:
apoelstra:
ACK dd3f3e44bc3329830aacacb252162c57858b0029; successfully ran local tests
Tree-SHA512: 8907ff127c11767a600cf856fb34645eb78392c755998565675a026dc4ddd9db00083963414f7e45fc7a2194ad3c5f251f7249bc7d7a2150a5840d1ec8b5d4da
a1e8fabbbe Remove unnecessary closure (Tobin C. Harding)
Pull request description:
We can just pass the function directly. Found by clippy bizarrely after running `rustfmt` (in bot-created PR #4586).
Internal change only, no logic change.
ACKs for top commit:
yancyribbens:
cr ACK a1e8fabbbe
apoelstra:
ACK a1e8fabbbe26cbe62e1a966cc0c760407bfb063f; successfully ran local tests
Tree-SHA512: 19d5ee65da7cfdae82a9a6609afac6d6b03d1c44c0c5bed21916ec31665e76adfc836ac6a1980251db2b9d5704baa577783dc967b1f0035cf662389c87fc6a0a
028a0d6558 Remove conversion impl macro for `Magic`/`Network` (rustaceanrob)
Pull request description:
Closes#4560 (or at least one way to close it)
Handle the coversion of new networks directly in the `From` and `TryFrom` implementations, as new networks are added infrequently.
ACKs for top commit:
tcharding:
ACK 028a0d6558
apoelstra:
ACK 028a0d6558129f554de8c4247481bc0ad80f1c27; successfully ran local tests
Tree-SHA512: 07b768e229305878849f23e54d3fb4940a736ce44122950e4f4bf68ddeb4f82f2d35020840e8176bd7b562726e46055650ba6de8559bae7559c881b64a437169
4621d2bde1 Modify locktime serde implemenations (Tobin C. Harding)
200c276315 bitcoin: Make test code spacing uniform (Tobin C. Harding)
Pull request description:
Patch 1 is preparatory clean up. Patch 2 is the meat and potatoes. See commit log there for full explanation.
Briefly:
- Remove `serde` stuff from `units::locktime`
- Manually implement `serde` traits on `relative::LockTime`
- Fix the regression test to use the new format
ACKs for top commit:
apoelstra:
ACK 4621d2bde14d71b3d6ef0b14258a7479c049ba3b; successfully ran local tests
Tree-SHA512: bc96d79dd4d9f5a124c22f0d3be4750cb7b6e86ba448b31e233982567578337d331b2582c6e1f953c00e8393c4a4552c4b574fe8a55323c911115b269b24090e
56516757ad Add code comment to amount calculation (Tobin C. Harding)
8cf1dc39b5 Fix incorrect code comment (Tobin C. Harding)
7c186e6081 Refactor fee functions (Tobin C. Harding)
bf0776e3dd Remove panic using checked arithmetic (Tobin C. Harding)
b65860067f Make fee functions const (Tobin C. Harding)
9b2fc021b2 Improve rustdocs on FeeRate (Tobin C. Harding)
1bd1e89458 Re-introduce FeeRate encapsulate module (Tobin C. Harding)
b27d8e5819 Change the internal representation of FeeRate (Tobin C. Harding)
2e0b88ba76 bitcoin: Fix dust 'fee' identifiers (Tobin C. Harding)
399bca531c Reduce the FeeRate::MAX value (Tobin C. Harding)
d174c06a4a Saturate to_fee to Amount::MAX (Tobin C. Harding)
64ac33754f Add missing argument docs (Tobin C. Harding)
fe0a448e78 Temporarily remove const from fee calc function (Tobin C. Harding)
b929022d56 Add floor/ceil versions of to_sat_per_kwu (Tobin C. Harding)
64098e4578 Remove encapsulate module from fee rate (Tobin C. Harding)
Pull request description:
The `FeeRate` is a bit entangled with amount and weight. Also we have an off-by-one bug caused by rounding errors and the fact that we use kwu internally.
We can get more precision in the fee rate by internally using per million virtual bytes.
- Fix: #4516
- Fix: #4497
- Fix: #3806
ACKs for top commit:
apoelstra:
ACK 56516757ad8874d8121dba468946546bb8fd7d4e; successfully ran local tests
Tree-SHA512: 0c6e7e2c9420886d5332e32519838d6ea415d269abda916e51d5847aa2475c87fa4abfc29b5f75e8b10c44df67ae29d823aa93f7cfabbc89eb27f2173b103242
We can just pass the function directly. Found by clippy bizarrely after
running `rustfmt` (in bot-created PR #4586).
Internal change only, no logic change.
We have a single test case that tests for the m prefix while all the
others do not.
Move one test case into the loop and then test on each iteration the
path with m prefix added.
cbe04b00c6 Remove all `p2p` dependency from `network` (rustaceanrob)
Pull request description:
Motivated by moving the `p2p` module to its own crate. `TryFrom` and `From` are already implement for converting to and from `Network`/`Magic`. The methods related to `Magic` are removed from `Network`, as well as any reference to `p2p` in the documentation, as `bitcoin` would no longer depend on `p2p`.
The deser roundtrip test are relocated to `p2p/mod.rs`
ACKs for top commit:
tcharding:
ACK cbe04b00c6
apoelstra:
ACK cbe04b00c67eab896b7ec0535194771ec36cb68f; successfully ran local tests
Tree-SHA512: ac3aa2eef4d78efd719ccc032a4266827faff8c87997111fa0050765b732462e5e5811c8aa923aedba335cbaad9a374fd54dbbe0f4978a1111d0839a5942af7d
The `units::locktime` types are used for two things:
- They are the inner types of `primitives` `LockTime`s
- They are used ephemerally for checking satisfaction
Neither of these use cases requires `serde` impls for the `units` types.
Since we are trying to release 1.0 with minimal amounts of code we
should remove them.
For `LockTime`s that need to be stored on disk or go over the wire we
can manually implement the `serde` traits. For `absolute::LockTime` this
is done already and there is no reason the `relative::LockTime` impl
cannot be the same [0]. This differs from the current `serde` trait impls
but we have already decided that in 0.33 we are going to accept breakage
and direct users to use 0.32 to handle it.
- Remove `serde` stuff from `units::locktime`
- Manually implement `serde` traits on `relative::LockTime`
- Fix the regression test to use the new format
While we are at it use a uniform terse call in `serialize`.
[0] This is because there is an unambiguous encoding for the whole set
of locktimes - consensus encoding.
20779bdbc8 Move `p2p` encodings out of `encode` (rustaceanrob)
94bcff28b1 p2p: Add wrappers for messages that use `Vec` (rustaceanrob)
Pull request description:
To move `p2p` into a crate:
1. All `Encodable` implementations for `p2p` must be defined in `p2p`. To accomplish this, and anything else in the future related to encoding, I added a `deser` module with the necessary macro to implement `Encodable` for `Vec<T> where T: Encodable`.
2. Because the orphan rule would apply for implementing `Encodable`for `Vec` within `p2p`, three types defined within `p2p` are used to wrap the `Vec` and snake around the orphan rule.
ACKs for top commit:
tcharding:
ACK 20779bdbc8
apoelstra:
ACK 20779bdbc8ebcac2365d122e857c23b3d8e8d1e7; successfully ran local tests
Tree-SHA512: 7abfe5b853e84bb465579309f80a0687c325217f6f342651278bedf540c4d17ae16683ab02dea5e3d98d90790deb12c6bc7d572a92cec8e19ff94e395bc0d29c
4ccecf5dec Fix stale Height type link (Tobin C. Harding)
caebb1bf73 units: relative: Do minor rustdocs fixes (Tobin C. Harding)
40bb177bc2 Put is_satisfied_by functions together (Tobin C. Harding)
480a2cd62a Favour new function `from_mtp` over deprecated (Tobin C. Harding)
f9d6453d5b Shorten locktime type term (Tobin C. Harding)
727047bd39 Fix off-by-one-bug in absolute locktime (Tobin C. Harding)
3ffdc54ca5 Fix off-by-one bug in relative locktime (Tobin C. Harding)
a2ff8ddbbb Improve relative::LockTime is_satisfied_by_{height, time} (Tobin C. Harding)
Pull request description:
Make the APIs uniform in relative and absolute locktimes in relation to the `is_satisfied_by` functions. In doing so improve the API and fix an off-by-one bug when checking satisfaction of locks by height.
Done in three patches but maybe should be squashed? Probably easiest to review by looking at all the `is_satisfied_by*` functions and convincing yourself we got it right.
EDIT: Now has 5 cleanup patches also (mostly docs cleanups).
ACKs for top commit:
apoelstra:
ACK 4ccecf5decfead9818b74fbdee73115c349e2f3e; successfully ran local tests
Tree-SHA512: 9206cb464a06647510a35a7d564062823117e75df60251969be458616f4f5d04acf0aada53dbf7d493a2a2a72d26b3a300417a6499e45413d5f2a011538b7826
To get more precision use sats per million virtual bytes.
To make review easier keep most calls in tests using
`FeeRate::from_sats_per_kwu` and just unwrap. These can likely be
cleaned up later on if we want to.
For `serde` just change the module to `_floor` and leave it at that. The
serde stuff likely needs re-visiting before release anyways.
Currently we get the fee_rate per kwu then multiply it by 4. Instead
lets add a per_kvb function to `FeeRate`. We are about to change the
internal representation of `FeeRate` to use MvB so for now just panic on
ovelflow.
Also these are fee _rates_ - we already have suboptimal names from Core
in the consts in `policy`, no need to let them infect other identifiers.
In preparation for changing the internal representation of `FeeRate` to
use MvB reduce the max value by 4_000.
Done separately to make the change explicit.
The `FeeRate::checked_mul_by_weight` function currently returns a
`NumOpResult` but all other `checked_` functions return an `Option`.
This is surprising and adds no additional information.
Change `checked_mul_by_weight` to return `None` on overflow. But in
`to_fee` saturate to `Amount::MAX` because doing so makes a few APIs
better without any risk since a fee must be checked anyway so
`Amount::MAX` as a fee is equally descriptive in the error case.
This leads to removing the `NumOpResult` from `effective_value` also.
Note that sadly we remove the very nice docs on `NumOpResult::map`
because they no longer work.
Fix: #4497
In preparation for changing the inner representation of `FeeRate` add
floor and ceil versions of the getter function `to_sat_per_kwu`.
For now both functions return the same thing but still call the
correct one so that when we change the representation we do not need
to re-visit them.
Motivated by moving the `p2p` module to its own crate. `TryFrom` and
`From` are already implement for converting to and from
`Network`/`Magic`. The methods related to `Magic` are removed from
`Network`, as well as any reference to `p2p` in the documentation, as
`bitcoin` would no longer depend on `p2p`.
The deser roundtrip test are relocated to `p2p/mod.rs`
6335c623f6 fix(p2p): remove `AddrV2` <> `SocketAddr` conversions (Luis Schwab)
Pull request description:
This PR reverts #4521 due to loss/creation of port information during the conversion. Users should use the `AddrV2` <> `IpAddr` conversion instead.
Closes#4566
ACKs for top commit:
Kixunil:
ACK 6335c623f6
tcharding:
ACK 6335c623f6
Tree-SHA512: f183756467ca17bb7e7078023a769ec33d3deb4146ab7ec63ef961107ab93ec975c9adffd9eeddc79250abfa1cee9eccbbcaef7466cf4c2c21205b69d8e9eb4f
9d956e8643 test: remove redundant `ServiceFlags` test (rustaceanrob)
Pull request description:
The `ServiceFlags` type is already tested within p2p/mod.rs with a nearly identical test. This type also has nothing to do with `network`
ref: https://github.com/rust-bitcoin/rust-bitcoin/blob/master/bitcoin/src/p2p/mod.rs#L400
ACKs for top commit:
tcharding:
ACK 9d956e8643
apoelstra:
ACK 9d956e8643214b3c1ad0e42cc630e2f35f7b7994; successfully ran local tests
Tree-SHA512: d113609070e3df6d44b1ed57c40c1bce251d59fa22eb1abbf6c576fd84e21464553bee7fa97eba000ad9c1b8ef98e2ba04aec7077486273de9b7e54341ba894b
9dac4d69e0 Implement CheckedSum for Weight iterator (yancy)
0dbcd09bbc Move CheckedSum trait to crate root (yancy)
Pull request description:
* Move CheckedSum to the crate root so that other types can be added
* Add Weight to CheckedSum
ACKs for top commit:
tcharding:
ACK 9dac4d69e0
apoelstra:
ACK 9dac4d69e0f142e9a2dc4b61ea49365a8cae3f4b; successfully ran local tests
Tree-SHA512: 3e4b7f79074e23493ccd17a026542081f0d7a811f4f35edb7994ada95bf414a166531f142ad4986d27fcec448209b2ffa56813b495b5df6b6e8fcd589392e0c1
All of the `Encodable` implementations are defined within `p2p` with the
exception of `p2p` types that are a `Vec`. To fully decouple `p2p` from
`encode` we can define these in `p2p` like the others. This is the final
step in removing anything `p2p` related from the rest of `bitcoin`.
In preparation to move `p2p` to its own crate, any `Vec<T>` that satisfies
`T: Encodable` will not trivially implement `Encodable` because of the
orphan rule. A type defined within p2p may implement `Encodable`,
however, and the simplest possible type is one that simply wraps the
vector. Three types that will implement `Encodable` within `p2p` are
added here.
merge-script
rustaceanrob
vicjuma
Tobin C. Harding
Fmt Bot
Jamil Lambert, PhD
yancy
Luis Schwab