Add check to max difficulty transition threshold #2337

tcharding · 2024-01-14T22:39:56Z

When computing the maximum difficulty transition threshold we forgot to check that the returned Target is not bigger than the maximum. This value is network specific so keep the original logic but with _unchecked on the function name.

This was noted in the discussion on #2161

tcharding · 2024-01-14T22:40:23Z

Note, this is an API breaking change. Done on purpose so users have to decided what to do about the network.

tcharding · 2024-01-14T22:44:04Z

Note this cannot be backported because we didn't have the max attainable consts yet in the last release.

apoelstra · 2024-01-15T15:19:06Z

We also need to check that our minimum shift doesn't go below 1. (And unlike the maximum, the minimum limit has actually been hit on the mainchain many times.)

Kixunil

ACK 205b8c7

Missing minimum check

Kixunil · 2024-01-15T19:13:05Z

Yeah, ACK with fixed minimum.

Note this cannot be backported because we didn't have the max attainable consts yet in the last release.

We could backport those. But it can't be backported because it's a breaking change. :) And while it's a bugfix it's easily solvable in user code. We should probably actually backport the constants - but only the constants and tell people to manually implement the check for now.

tcharding · 2024-01-24T04:05:31Z

Add check for not less than 1 to minimun transition threshold.

coveralls · 2024-01-24T04:05:54Z

Pull Request Test Coverage Report for Build 8515679554

Details

12 of 22 (54.55%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 83.138%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bitcoin/src/pow.rs	4	14	28.57%

Totals
Change from base Build 8515631286:	0.002%
Covered Lines:	19101
Relevant Lines:	22975

💛 - Coveralls

Kixunil

ACK 2499a15

bitcoin/src/pow.rs

Kixunil

ACK 8f3537b

bitcoin/src/pow.rs

tcharding · 2024-01-29T21:31:48Z

I've split the PR up into multiple patches so each patch does one conceptual thing.

apoelstra · 2024-01-29T21:48:40Z

CI failure looks real.

tcharding · 2024-03-19T23:56:33Z

I added 16e64ef at the front of the PR to do the clippy allow. No other changes.

apoelstra

ACK aa4591d

tcharding · 2024-03-25T19:27:59Z

@sanket1729 this only has rebases since you last acked.

git range-diff 1165c2c...@
-:  -------- > 1:  b3c28e9c Allow needless-borrows-for-generic-args
1:  31b19e04 = 2:  c00b756e Document current behaviour of difficulty_float
2:  d8de117e = 3:  bc5bd736 Debug assert that target != zero in difficulty calc
3:  0c63d0c2 = 4:  502a76b4 Take Params instead of Network in difficulty function
4:  dc1f6514 = 5:  39c269e7 Rename Params::pow_limit to max_attainable_target
5:  1b34fb06 = 6:  1112b03d Rename difficulty transition threshold functions
6:  1165c2c4 = 7:  aa4591d3 Improve API for max target threshold calculation

0ca5a43 hashes: Bump version to v0.14.0 (Tobin C. Harding) Pull request description: In preparation for release add a changlelog entry and bump the version. Note the hashes 0.13.0 dependency stays in the dependency graph because of secp, we can update secp after releasing `hashes` then update the secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0` dependency - phew. Note we are right to release this immediately, the two open PRs (#2337 and #2541) that touch `hashes` only add a clippy attribute so can safely be ignored. ACKs for top commit: apoelstra: ACK 0ca5a43 sanket1729: ACK 0ca5a43 Tree-SHA512: d1d26acb8fbf13f785b25add3f1dac05bb392b5bdbad16ead2bc5dd26f3d668824c4b653c373f88c3562a37e775146766680606cedd19db40e0f197b26ca86b8

This lint triggers when parsing a reference to a large struct as a generic argument, which is wrong. Allow it crate wide because [subjectively] this lint never warns for anything useful.

Improve rustdocs on the `Target::difficulty_float` function, specifically the return value if self is zero.

The `difficulty` calculation requires dividing a target value by `self`. Add an assertion that `self` is not zero to help devs debug this. Note that this should never really be hit, but its possible there is a bug somewhere causing the target to be set to zero - so this may help debugging. Also, add panics section to rustdocs.

What we really want is the maximum target, but since this is a const in `Params` use an `AsRef<Params>` argument in the `difficulty` functions. Requires implementation of `AsRef<Params> for Params`.

The maximum "attainable" target is a `rust-bitcoin` thing, Core use max unattainable. Deprecated the `Params::pow_limit` field and add a new field `max_attainable_target`. The `Params` type is `non_exhaustive` so this is not an API breaking change.

These two functions calculate the min/max threshold transition which is a _target_ not a "difficulty" number. Using "difficulty" in the function name is unnecessarily confusing. Rename and deprecate the functions.

The maximum target threshold has a network dependant upper bound. Currently we are not checking this bound. One complication is that there is currently heated open debate around the `Network` type. We can bypass the `Network` issue by using `AsRef<Params>` instead. Add a function that does the checks based on the `Params` type as well as an unchecked version.

tcharding · 2024-04-02T00:42:41Z

Rebased, some changes show up in git range-diff due to rustfmt.

apoelstra

ACK fd6fedc

tcharding · 2024-04-02T01:59:52Z

This has minimal changes since @sanket1729 acked (just rebase and the stuff caused by now being on top of a formatted codebase after #2462). Can be merged by way of rule 4 of the one ack carve-out.

sanket1729

ACK fd6fedc

sanket1729 · 2024-04-02T02:26:13Z

bitcoin/src/pow.rs

@@ -245,6 +245,10 @@ impl Target {
    ///
    /// See [`difficulty`] for details.
    ///
+    /// # Returns
+    ///
+    /// Returns [`f64::INFINITY`] if `self` is zero (caused by divide by zero).


Interesting, TIL.

tcharding added the API break This PR requires a version bump for the next release label Jan 14, 2024

Kixunil previously approved these changes Jan 15, 2024

View reviewed changes

tcharding force-pushed the 01-15-difficulty-transition branch from 205b8c7 to 2499a15 Compare January 24, 2024 04:04

github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Jan 24, 2024

Kixunil previously approved these changes Jan 24, 2024

View reviewed changes

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

tcharding dismissed Kixunil’s stale review via 8f3537b January 25, 2024 00:09

tcharding force-pushed the 01-15-difficulty-transition branch from 2499a15 to 8f3537b Compare January 25, 2024 00:09

Kixunil previously approved these changes Jan 25, 2024

View reviewed changes

apoelstra reviewed Jan 25, 2024

View reviewed changes

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

tcharding dismissed Kixunil’s stale review via 7c8663b January 25, 2024 23:39

tcharding force-pushed the 01-15-difficulty-transition branch 3 times, most recently from 77ea0c3 to 1f2d561 Compare January 28, 2024 20:51

apoelstra reviewed Jan 29, 2024

View reviewed changes

bitcoin/src/pow.rs Show resolved Hide resolved

tcharding force-pushed the 01-15-difficulty-transition branch from 1f2d561 to 14ab447 Compare January 29, 2024 00:35

apoelstra reviewed Jan 29, 2024

View reviewed changes

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

apoelstra reviewed Jan 29, 2024

View reviewed changes

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

apoelstra reviewed Jan 29, 2024

View reviewed changes

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

tcharding force-pushed the 01-15-difficulty-transition branch 2 times, most recently from 477077f to 7518706 Compare January 29, 2024 21:30

tcharding force-pushed the 01-15-difficulty-transition branch from 7518706 to a5d60c7 Compare January 29, 2024 21:34

github-actions bot added C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-internals PRs modifying the internals crate C-io PRs modifying the io crate labels Mar 19, 2024

tcharding force-pushed the 01-15-difficulty-transition branch from 1ed6e54 to aa4591d Compare March 19, 2024 23:58

apoelstra previously approved these changes Mar 20, 2024

View reviewed changes

tcharding mentioned this pull request Mar 21, 2024

Release tracking PR: hashes v0.14.0 #2621

Merged

tcharding mentioned this pull request Mar 25, 2024

Release tracking PR: base58ck v0.1.0 #2595

Merged

tcharding added the P-high High priority label Mar 27, 2024

Use link to CompactTarget in rustdoc

2a6821b

tcharding dismissed apoelstra’s stale review via 1dd3444 April 2, 2024 00:38

tcharding force-pushed the 01-15-difficulty-transition branch from aa4591d to 1dd3444 Compare April 2, 2024 00:38

tcharding added 7 commits April 2, 2024 11:40

Allow needless-borrows-for-generic-args

3d01146

This lint triggers when parsing a reference to a large struct as a generic argument, which is wrong. Allow it crate wide because [subjectively] this lint never warns for anything useful.

Document current behaviour of difficulty_float

c1ba496

Improve rustdocs on the `Target::difficulty_float` function, specifically the return value if self is zero.

Take Params instead of Network in difficulty function

f0f6d3f

What we really want is the maximum target, but since this is a const in `Params` use an `AsRef<Params>` argument in the `difficulty` functions. Requires implementation of `AsRef<Params> for Params`.

Rename difficulty transition threshold functions

6e47d57

These two functions calculate the min/max threshold transition which is a _target_ not a "difficulty" number. Using "difficulty" in the function name is unnecessarily confusing. Rename and deprecate the functions.

tcharding force-pushed the 01-15-difficulty-transition branch from 1dd3444 to fd6fedc Compare April 2, 2024 00:41

apoelstra approved these changes Apr 2, 2024

View reviewed changes

sanket1729 approved these changes Apr 2, 2024

View reviewed changes

apoelstra merged commit 499f36f into rust-bitcoin:master Apr 2, 2024
168 checks passed

apoelstra mentioned this pull request Apr 2, 2024

Add hex parsing to pow types #2514

Merged

tcharding deleted the 01-15-difficulty-transition branch April 2, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check to max difficulty transition threshold #2337

Add check to max difficulty transition threshold #2337

tcharding commented Jan 14, 2024 •

edited

tcharding commented Jan 14, 2024

tcharding commented Jan 14, 2024

apoelstra commented Jan 15, 2024

Kixunil left a comment

Kixunil commented Jan 15, 2024

tcharding commented Jan 24, 2024

coveralls commented Jan 24, 2024 •

edited

Kixunil left a comment

Kixunil left a comment

tcharding commented Jan 29, 2024

apoelstra commented Jan 29, 2024

tcharding commented Mar 19, 2024 •

edited

apoelstra left a comment

tcharding commented Mar 25, 2024

tcharding commented Apr 2, 2024

apoelstra left a comment

tcharding commented Apr 2, 2024

sanket1729 left a comment

sanket1729 Apr 2, 2024

Add check to max difficulty transition threshold #2337

Add check to max difficulty transition threshold #2337

Conversation

tcharding commented Jan 14, 2024 • edited

tcharding commented Jan 14, 2024

tcharding commented Jan 14, 2024

apoelstra commented Jan 15, 2024

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil commented Jan 15, 2024

tcharding commented Jan 24, 2024

coveralls commented Jan 24, 2024 • edited

Pull Request Test Coverage Report for Build 8515679554

Details

💛 - Coveralls

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Jan 29, 2024

apoelstra commented Jan 29, 2024

tcharding commented Mar 19, 2024 • edited

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 25, 2024

tcharding commented Apr 2, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Apr 2, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 Apr 2, 2024

Choose a reason for hiding this comment

tcharding commented Jan 14, 2024 •

edited

coveralls commented Jan 24, 2024 •

edited

tcharding commented Mar 19, 2024 •

edited