I've rebased and pushed the changes that I understood from the review. I've also added the testnet work as future work, as discussed with @apoelstra.

Regarding some of the feedback that @junderw provided, I couldn't find a solution. I was only told that my approach was incorrect, but I'm unsure how to modify my code to address the identified issues.

Could you please provide a clearer review to guide me in fulfilling your requirements?

Thank you.

Could you please provide a clearer review to guide me in fulfilling your requirements?

The requirements aren't special. Just "perform correct calculations."

I don't understand how much clearer I can be without re-writing your code completely.

Adding tests might help prove your calculations are right. You can even take some values from mainnet blocks and plug them in and make sure you get the same results.

Here's a single test to help you.

Currently it fails

running 1 test
thread 'pow::tests::test_calculate_next_work_required' panicked at 'assertion failed: `(left == right)`
  left: `179`,
 right: `386197775`',

#[test]
fn test_calculate_next_work_required() {
    use std::str::FromStr;
    let mainnet_params = Params::new(crate::Network::Bitcoin);
    // Difficulty 808416 -> 810432 on mainnet

    // Expected result bits = 0x1704e90f (bits of 810432)
    let expected = 0x1704e90f;
    // We take the timestamp of 810431 (the block before)
    let height_810431 = Height::from_consensus(810431).unwrap();
    let timestamp_810431 = 0x651bc919;

    // This closure should return the header for 808416 since 810431 - 2015 is 808416
    let fetch_header_808416 = |_height| {
        // Header for 808416
        Ok(Header {
            version: crate::block::Version::from_consensus(0x29322000),
            prev_blockhash: BlockHash::from_str(
                "000000000000000000027ecc78c2da1cc5c0b0496706baa7e4d7c80812c10bf3",
            )
            .unwrap(),
            merkle_root: crate::TxMerkleNode::from_str(
                "b920d5b5ebef4e9d106072944e0729cea8bf6defc583a7d87063041a316a757b",
            )
            .unwrap(),
            time: 0x650964b5,
            bits: CompactTarget(0x1704ed7f),
            nonce: 0xc637a163,
        })
    };
    let result = calculate_next_work_required(
        mainnet_params,
        height_810431,
        timestamp_810431,
        fetch_header_808416,
    )
    .unwrap();
    assert_eq!(result, expected);
}

Please see vincenzopalazzo#1 for fixes.

Found a bug in the Work constants #2103

This calculation is ok, but the variable names should change between mid-state and final result.

You are right, I get completely confused! now review your code I get where I was going. Thanks for doing that

Hey mate, I spent the afternoon playing with this PR. The only way I understand hard things is to code them myself so I re-wrote a new buggy version. I can write a review here but it is probably easier for you to just take a look and grab anything you like and leave out anything you don't. Then I'll re-review again.

https://github.com/tcharding/rust-bitcoin/blob/606bfe93d0268f7f29c3234ee4072b8c39ae79ee/bitcoin/src/pow.rs#L955

EDIT: Oh sick, while checking the link I saw the bug!

@Kixunil suggested new functions min, clamp help a lot, IMO. The code I wrote yesterday still looks a lot like C++.

copied from lightningdevkit/rust-lightning#2124 (comment)

So you guys need #2090 merged and backported to 0.30.1 as well as 0.31.1, right?

Is ready for review, this PR is blocking me on nakamoto, but I think I will have a followup regarding the API.

I don't see any force pushes in the last 5 days, is the github UI glitching?

suggested new functions min, clamp help a lot, IMO. The code I wrote yesterday still looks a lot like C++.

What is wrong with being like C++? I do not see anything wrong with C++ style. However, the bad thing about the C++ version is that there are more than 4 people trying to understand the core code, so the only thing that would be good to improve is the documentation and the code, other black magic rust is just making the core difficult to read when the time to say "I wrote yesterday still looks a lot like Rust." will come.

I will go through the review over the weekend

What is wrong with being like C++? I do not see anything wrong with C++ style.

There's a reason we use quite different style in Rust. Aside from many bugs, Rust is much more readable.

Min, max a and clamp are not black magic.

Min, max a and clamp are not black magic.

This is also possible in C++ with std::min and std::max but other than that (I did not write the code that you reviewed, the commit is Co-Developed if you noted) I do not see why we should change the structure of the code that we do not understand well enough to change and simplify from the core version.

Other than using the min, man and clamp simplification I do not expect to see this function structure different from the core version, except if there is someone who will step up and take ownership of the code by also fulfilling the needs of the user libraries like nakamoto, ldk and friends

I don't see any force pushes in the last 5 days, is the github UI glitching?

Sorry @tcharding I got overwhelmed by the rust-bitcoin notification and I lost it inside my notification view. You should see the force push now

I, and likely most Rustceans, find those methods more readable. That's a good enough reason already. But it's not something I would block a PR for. But it's possible we'll have to change it anyway if a future version of clippy has a lint for this.

    Checking secp256k1 v0.28.0
error[E0658]: use of unstable library feature 'clamp'
    --> bitcoin/src/pow.rs:1018:45
     |
1018 |     let adjusted_timespan = actual_timespan.clamp(pow_target_timespan / 4, pow_target_timespan * 4);
     |                                             ^^^^^
     |
     = note: see issue #44095 <https://github.com/rust-lang/rust/issues/44095> for more information

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.
error: could not compile `bitcoin`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error[E0658]: use of unstable library feature 'clamp'
    --> bitcoin/src/pow.rs:1018:45
     |
1018 |     let adjusted_timespan = actual_timespan.clamp(pow_target_timespan / 4, pow_target_timespan * 4);
     |                                             ^^^^^
     |
     = note: see issue #44095 <https://github.com/rust-lang/rust/issues/44095> for more information

error: aborting due to previous error

cc @tcharding @Kixunil

Oh, it has MSRV 1.50, sorry about that. Skip that one and consider adding // TODO: update to clamp in MSRV >= 1.50

I do not see why we should change the structure of the code that we do not understand well enough to change and simplify from the core version.

Who is the second "we" referring to? After spending time on this last week I am more than comfortable to say I understand the core code and can re-write it in Rust style.

except if there is someone who will step up and take ownership of the code by also fulfilling the needs of the user libraries like nakamoto, ldk and friends

I can take ownership of this, I was intentionally not doing so because I didn't want to steam roll over you since you got here first. If you don't mind how this gets done and just want to see the functionality in rust-bitcoin then lets close this PR and I'll push it in through #2180. I'm super keen to get this in because LDK is waiting on it so they can upgrade.

If there is any other functionality missing that nakamoto needs please do say so, I'm working me hardest to support folk these days. I have not personally looked at the nakamoto codebase though.

To clarify, if you leave this open I'll assume you want to do the work and will review and wait for you. If you close this PR I'll push forward with #2180.

I think we should close in favor of 2180. I am willing to review this PR again though, if it needs reviewing.

I think the sync API is much more concerning than any code style around min/max.

@tcharding up to you, I really do not care who is the author of the PR or how the code will be written more rusty or not Cpp style. I care just that if I have some bugs the code is clear that I can read it without cry

The code that will be merged will have no API user, so we do not know if with this API I will be able to sync nakamoto with the network and fully validate the chain or if the user of the API needs some different UX. So my priority is to have this functionality so I can continue to do my work and provide more tests about the code.

When we have this code merged then I need to way the wallet user to update to the new version of rust bitcoin and then I can test nakamoto. So this sounds a work that will take months.

I think the sync API is much more concerning than any code style around min/max.

We can also think of moving the function in an async style due to that in the rust compiler we are doing some work to make the async programming usable. However, this is not good for people who are using not standard async IO (e.g nakamoto) or low-level API with mio and I do not like forcing this async style without having some concrete user. e.g: If nakamoto is going to use this API we do not need an async function

Maybe we can modify the API to be completion based? I know it can have performance issues but if it's done right it might be fine. Like what I did with push_decode.

@tcharding up to you, I really do not care who is the author of the PR

Can you close this if you are ok for me to work on #2180, I don't want to close your PR.

or how the code will be written more rusty or not Cpp style. I care just that if I have some bugs the code is clear that I can read it without cry

Don't worry, writing code that is brain-dead simple to read is my thing :)