Pull Request Test Coverage Report for Build 9089264878

Details

• 126 of 127 (99.21%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 83.318%

Totals
Change from base Build 9088761779:	0.1%
Covered Lines:	19303
Relevant Lines:	23168

💛 - Coveralls

cc #1820 which added some different-but-related missing functionality

In 206bc621d34f0ef25b5d91a512d2e67e635f77e4:

There is trailing whitespace on several lines that I'd like you to remove. You can find them with git grep ' $'.

And could you add a couple unit tests, maybe by taking headers from the actual blockchain, that demonstrate that the adjustment does the right thing under normal circumstances, when it hits the "minimum 1 difficulty" rule, and when it hits one of the factor-of-4 rails? (The latter has never happened on Bitcoin but it should be easy to gin up a test vector by editing block timestamps, since we're not validating the PoW here.)

But concept ACK -- this functionality is missing AFAICT. Thanks for your contribution!

I approach NACKed myself over the weekend. After thinking about it I decided that the Bitcoin Core approach is less susceptible to user misinterpretation. Also when considering the difficulty adjustment it is more appropriate to be dealing with CompactTarget since it is used in the headers and has lower precision. I updated the documentation with reference to the Bitcoin Core implementation and an example. Also added 5 test cases for each scenario.

I couldn't manage to get a link to a line number for some reason so I did this instead:
https://github.com/rust-bitcoin/rust-bitcoin/pull/2180/files#r1594975932

This is what I meant by my question about the timespan parameter pushing the knowledge of the off-by-one error onto users.

@tcharding I don't understand exactly what you're suggesting. Whether the user passes a header or header.timestamp does not affect whether they will be burned by the off-by-one error.

It seems like our choices are either (a) take the whole blockchain, or at least the most recent few thousand blocks, and compute the block intervals ourselves, protecting the user, or (b) take just the two relevant blocks, trusting the user to choose the right ones.

This PR currently has both (though maybe they could be named more clearly to distinguish them), and I think we need both, since we don't want to require the user to have tons of blockheaders available when they're not even relevant.

@tcharding I don't understand exactly what you're suggesting. Whether the user passes a header or header.timestamp does not affect whether they will be burned by the off-by-one error.

Mine is just a question on API design and whether we want/need the additional functionality next_target (#2180) provides. I think this PR is good.

In #2180 the user passes current header to next_target, that function then either returns the header.bits if the next block is not a retarget block or does the calculation by calling calculate_next_target, the caller does not need to be aware of the the off-by-one stuff.

FTR #2180 suffers the same issue I'm describing because both CompactTarget::from_next_work_required (this PR) and calculate_next_target (#2180) are basically the same. @rustaceanrob has done better than me already by documenting it, I just cargo-cult-programmed the -1.

Ok, yes, you can encapsulate this by requiring the user provide an accessor for every single block in the chain. (Though you can cheat by knowing exactly which blocks it is going to access, of course.)

This PR does this, but also provides a method in which you only need to have the two relevant headers. So I think it's strictly better.

ACK ed5574f5b511ca803a9fb041d820631e7ae57158 except that we should restore the "just pass the whole chain" functionality that was present in the original PR.

restore the "just pass the whole chain" functionality

Okay, back to two Header? I was struggling to find good names for them where the ordering is unambiguous. How is current, previous?

I think you want something like current and last_epoch_boundary.

Okay, back to two Header?

Yeah, I think we should have both.

In 0aa279aa33791dcc462f90e1574bd8d443871fbb:

• If you are going to cite Core line numbers, please specify which commit of Core you are using
• Sorry, I meant that we should have both a "compute from two headers" method (and I don't care if it takes timestamps or whole headers or what) and also a "compute from all the headers" method (which chooses the right headers to use).

With the compute from all the headers approach, I've only come up with two solutions, both concerned with chain height. The only height-implicit approach I know of is to pass a &[Header] representing the current header chain. For low resource devices that might be a challenge, but then again the other methods of this PR would have them covered if they take 5 minutes to research how the adjustment works. Otherwise, the explicit approach is to pass a current chain height and some function to index into the chain at a specific height as in #2180. Implied in that approach would be pinning down the Height structure. I am compelled to just go with &[Header], is that alright?

I'm leaning towards thinking this is mergable as is and we do the additional functionality in a separate PR. This PR is already a big improvement.

The only height-implicit approach I know of is to pass a &[Header] representing the current header chain.

You can take a closure which takes a usize and returns the header at that index. We have an existing PoW-related function that does this, I forget which one.

@tcharding

I'm leaning towards thinking this is mergable as is and we do the additional functionality in a separate PR. This PR is already a big improvement.

Sure, I'm happy to do this -- but we're a little ways from release and we're close to having this PR having the API we want. It's up to you @rustaceanrob. If you want me to just merge this I'll do it.

If you are going to cite Core line numbers, please specify which commit of Core you are using

Updated to included the specific commit

Sure, I'm happy to do this -- but we're a little ways from release and we're close to having this PR having the API we want. It's up to you @rustaceanrob. If you want me to just merge this I'll do it

I think this is an appropriate stopping point for now. Users are able to perform the adjustment and any additions would be additive. I am unsure how much time I will have to work on this in the next couple days. I am envisioning something similar to #2180 where there is a nice FnMut that takes a Height. In a follow up PR we can discuss this further.

git range-diff didn't work, I had to use the Compare link above.

used range-diff

Bollocks :)

@apoelstra @tcharding how hard would it be to backport this? I want to get rid of my out-of-tree version of bitcoin in floresta and this would be very useful to have on 0.32

I think it'd be super straightforward. @tcharding can you open a PR? I suspect you can just cherry-pick this commit.

Thanks @apoelstra . If @tcharding can't do it I can give it a shot.

@Davidson-Souza that would be great actually, since then both of us can ACK it and merge it quickly.

Hey @apoelstra I see you added a new label, we have three backport labels already that are version specific. When I added those (eg port-0.32.x) I thought we would add all three if needed and remove them as we did the backports.

@tcharding oh, good to know, thanks. I will rename them so they have "backport" in the name and drop mine.

Backported in #3494