Isn't block::Header::validate_pow what you need?

If not, I would still prefer having a method to do the verification rather than ad-hoc shr/shl because presumably more people will want to do this kind of verification, not just LDK. Also this shouldn't block you because you can implement it manually:

// this is shl, shr is similar
let bytes = target.to_be_bytes();
let high = u128::from_be_bytes(bytes[..16].try_into().unwrap());
let low = u128::from_be_bytes(bytes[16..].try_into().unwrap());
let transferred_bits = low >> (128 - bits_to_shift);
let high = high << bits_to_shift | transferred_bits;
let low = low << bits_to_shift;
let mut shifted_bytes = [0u8; 64];
shifted_bytes[..16].copy_from_slice(&high.to_be_bytes());
shifted_bytes[16..].copy_from_slice(&high.to_le_bytes());
Target::from_le_bytes(shifted_bytes)

I know it's not nice but at least it's not impossible and we can clean it up soon.

@Kixunil I think the shift operators make sense here, since they shift "how many bits of difficulty" the target is.

I'm leaning toward ACKing this because the operators do make conceptual sense, and they don't make sense for Difficulty which is the only type I think people might get confused with, so they're not likely to cause accidents.

But yes, the specific "determine difficulty rails after an adjustment" algorithm that @wpaulino is describing is something we should have first-class support for!

Note that the difficulty adjustment is bounded by "divide by 4" below and "multiply by 4" above. So this is a nearly trivial operation even if your only operators are shifts. It would be a shame to turn 2 lines into 20.

Edit It's actually a little more subtle, I remembered, since there's a hard upper bound at difficulty 1. We probably should provide such a method (and maybe one for Difficulty too).

Isn't it a bit awkward to have only shift exposed and not other operations? Also can't find it now but IIRC division was also needed to adjust difficulty.

One thing that bothers me about this is that lowering target by an amount that can not be expressed as shift is still valid operation and does increase difficulty but we don't allow it.

@Kixunil for sure, computing an actual difficulty adjustment will require more than shifts. But computing the maximum and minimum adjustment for a given period (which apparently is what @wpaulino is trying to do) does not.

Also this shouldn't block you because you can implement it manually:

Fair enough. I figured since it was something already supported by the API previously, and it's required for a valid use-case, it would make sense to expose it again.

Isn't it a bit awkward to have only shift exposed and not other operations?

Yeah, I thought so too. I guess it depends on what plans you all have for rust-bitcoin's API. IMO, it should either expose the operations to do anything you may need to yourself, add new helper methods on Target/Difficulty covering the most common use cases, or implement both.

I'd also strongly prefer if rust-bitcoin had a method for this, but the context-less validate_pow isn't what we need - we want to verify that a difficulty adjustment is sane. This is a really critical operation to prevent all kinds of DoS attacks on clients which download headers, and while it would be great if we could call out to rust-bitcoin and have it tell us if a difficulty transition given a pile of headers is correct, just checking the 4x bounds at least addresses the DoS issues for the most part.

I think we should expose both functions (compute min/max bounds, and compute exact adjustment). And we shouldn't expose the shifts.

@wpaulino could you adjust this PR to expose a min/max function instead? (You could also implement a full difficulty calculation if you're feeling generous, but that'd be 10x the work and it sounds like you'd be satisfied with just the min/max for now.)

@apoelstra only exposed the thresholds for now, but I may come back around to implement the exact adjustment computation in the future.

Needs cargo +nightly fmt running please mate.

Thanks man!

@wpaulino the max is not correct, since the target is bounded above by the minimum difficulty being 1.

Whoops, thanks. Also bounded the min result, even though it shouldn't happen in practice, since you can currently get a Target above Target::MAX with Target::from_be_bytes.

Retracting my ACK. I might need some more time to check this completely. bitcoin core does these operations of in64_t. and then converts to uint256_t. Need to make sure if the way we are doing it is correct

@sanket1729 could you point to what you were looking at? There should only be a conversion from the compact representation (Header::bits) as a u32 to a u256.

Implementors of difficulty transition algorithms should take care to only operate on CompactTarget types as there are many conversions from u32 to uint256_t and vice-versa.

Some of these conversions are lossy, and could lead to subtle bugs.

Implementors of difficulty transition algorithms should take care to only operate on CompactTarget types

cc #1839 #1828 which discuss this discrepancy.

I'm happy to merge this as-is. We don't expose enough functionality yet for somebody to compute actual difficulty targets so I'm not too worried about somebody using the incorrect-on-extreme-values logic to cause consensus faults (also, even if you did, the first 10000 or so Bitcoin blocks hit the min-difficulty clamp so this would be immediately visible). And as written, this function serves well as a simple anti-DoS sanity check on a provided chain.

Interesting that Target and Difficulty are network-specific values ... I guess philosophically so are Block, Header, Transaction, Txid, etc. I wonder if there's some general way we could handle this that would be reasonably ergonomic and also fix our difficulties with Address..

Have I gone mad, where are these changes gone? They are not on master anymore.

git show 8e6f953aa72eabbdad53937765e33c52c1a63c8b shows the patch but we've lost them somehow?

cc @apoelstra

ugh, I am a total wombat, I had an old tag checked out from yesterday. Face palm.