Expose valid (min, max) difficulty transition thresholds #1820

wpaulino · 2023-04-28T17:41:24Z

Once U256 was made private, we lost the ability to check whether a valid difficulty transition was made in the chain, since Target doesn't expose any operations. We only choose to expose Shl<u32> and Shr<u32> such that we can compute the min and max target thresholds allowed for a difficulty transition.

This is something we realized was missing after bumping to rust-bitcoin v0.30.0 in rust-lightning, specifically for our lightning-block-sync crate. It may also be worth having a helper in rust-bitcoin that checks a header properly builds upon the previous, but that can be left for future work.

tcharding

Thanks for the contribution! Sorry for rugging you :)

My review is just for code correctness, I have no knowledge on whether or not Target should expose these, the use case sounds sane to me though.

ACK 669f248

Kixunil · 2023-04-29T07:19:04Z

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.

apoelstra · 2023-04-29T13:34:34Z

@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.

apoelstra · 2023-04-29T13:35:09Z

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).

apoelstra

ACK 669f248

Kixunil · 2023-04-30T19:07:05Z

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.

apoelstra · 2023-04-30T19:15:51Z

@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.

wpaulino · 2023-05-01T18:58:03Z

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.

TheBlueMatt · 2023-05-01T20:25:19Z

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.

apoelstra · 2023-05-01T21:06:50Z

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.)

wpaulino · 2023-05-01T21:59:35Z

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

tcharding · 2023-05-02T00:05:53Z

Needs cargo +nightly fmt running please mate.

tcharding

ACK 6187983

tcharding · 2023-05-02T00:22:37Z

Thanks man!

apoelstra · 2023-05-02T01:49:08Z

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

wpaulino · 2023-05-02T02:16:57Z

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.

tcharding · 2023-05-02T04:43:49Z

bitcoin/src/pow.rs

+        if min > Self::MAX {
+            Self::MAX
+        } else {
+            min
+        }


I'm not a fan of this, it reads funnily. If we are not upholding the invariant that Self is less than or equal to Self::MAX then we should fix that and not band aid over it here.

I raised an issue: #1828

Is this something we want to block this PR on, or can it come after this lands?

I also realized that MAX here is network dependent. The current value is valid for mainnet and testnet, but regtest and signet have higher values, invalidating the current MAX constant. This makes me think that until we have network dependent max values, we shouldn't do any bounds checking, unless we only plan to support mainnet for such operations.

o.O you can have difficulty < 1 for regtest and signet? What is the corresponding target value?

Or do you mean that the conversion between difficulty and target is different on these networks....which would require we modify our conversion functions.

The conversion is the same, but the highest target/PoW limit varies by network. IIUC, the difficulty is just the ratio between the highest target and the current target, so a difficulty of 1 just represents the highest target for that given network. With that said, it seems Target::difficulty currently only works for mainnet and testnet values, since they both share the same max target.

:( so the conversion is different for different networks.

That is really obscure and does not seem necessary. But ok, we'll find a way to deal with it.

How would you like to proceed then? Since we're not doing any bounds checking at all, and it depends on the active network, we could defer that to future work and not hold back the changes proposed here?

Agree let's not hold this up.

Kixunil

I think the overflow should be fixed unless I'm missing something.

bitcoin/src/pow.rs

Kixunil · 2023-05-02T12:07:59Z

bitcoin/src/pow.rs

+        if min > Self::MAX {
+            Self::MAX
+        } else {
+            min
+        }


bitcoin/src/pow.rs

Once `U256` was made private, we lost the ability to check whether a valid difficulty transition was made in the chain, since `Target` no longer exposes any arithmetic operations.

Kixunil

ACK 8e6f953

Kixunil · 2023-05-04T19:50:15Z

bitcoin/src/pow.rs

+    ///
+    /// The difficulty can only decrease or increase by a factor of 4 max on each difficulty
+    /// adjustment period.
+    pub fn max_difficulty_transition_threshold(&self) -> Self { Self(self.0 << 2) }


This will debug-panic on large values, is this OK/correct?

What makes you say that @Kixunil ? We use wrapping_shl to implement << so I thought this couldn't panic, what am I missing?

@wpaulino why did the check against MAX get removed here? In the previous iteration I only meant to comment that I didn't like the check in the min_difficulty_transition_threshold method, in the max one I thought it was correct.

why did the check against MAX get removed here? In the previous iteration I only meant to comment that I didn't like the check in the min_difficulty_transition_threshold method, in the max one I thought it was correct.

Because MAX currently is only applicable to mainnet and testnet. I guess that's fine since this is also the case with Target::difficulty, but I figured since we want this to be network agnostic at some point, it doesn't make sense to bound by MAX until we have the network specific max constants.

Oh yes, sorry, my bad for not remembering all the context when reviewing. I did read that before.

Oh, sorry I got confused, it'd only panic if the argument was too large, 2 is not too large.

I constantly have this same confusion about left-shift. It's a little weird, this notion of "overflow" is different from that for the arithmetic opcodes.

sanket1729

ACK 8e6f953.

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

wpaulino · 2023-05-04T22:05:02Z

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.

sanket1729

ACK 8e6f953 . Sorry, was confused by some details.

sanket1729 · 2023-05-06T04:16:11Z

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.

apoelstra · 2023-05-06T15:33:47Z

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

cc #1839 #1828 which discuss this discrepancy.

apoelstra · 2023-05-06T15:40:02Z

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..

apoelstra

ACK 8e6f953

tcharding · 2023-11-02T19:55:50Z

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

tcharding · 2023-11-02T20:00:32Z

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

We just backported PR rust-bitcoin#1820, add a changelog entry and bump the crate version.

ba99aa4 Bump version to 0.30.2 (Tobin C. Harding) 3c9bbca Expose valid (min, max) difficulty transition thresholds (Wilmer Paulino) Pull request description: Patch one backports #1820, I believe this is all that is needed for LDK to be able to upgrade to `rust-bitcoin v0.30`. Context: - #1820 - lightningdevkit/rust-lightning#2124 Patch 2 adds a changelog entry and bumps the minor version. ACKs for top commit: Kixunil: ACK ba99aa4 Tree-SHA512: 93adb52bbc8e61ece63e5653dea7cf6d2d25028f25cb8bbda14606d9ebf74becaee0a0c4f4f3d7fde56c0e2b8a57f8c9be263e04833161a1da0baaf8fcca8764

wpaulino mentioned this pull request Apr 28, 2023

Bump rust-bitcoin to 0.30.0 lightningdevkit/rust-lightning#2124

Closed

tcharding previously approved these changes Apr 29, 2023

View reviewed changes

apoelstra previously approved these changes Apr 29, 2023

View reviewed changes

wpaulino dismissed stale reviews from apoelstra and tcharding via 3357db8 May 1, 2023 21:56

wpaulino force-pushed the impl-shift-target branch from 669f248 to 3357db8 Compare May 1, 2023 21:56

wpaulino changed the title ~~Expose Shl<u32> and Shr<u32> impl for Target~~ Expose valid (min, max) difficulty transition thresholds May 1, 2023

wpaulino force-pushed the impl-shift-target branch from 3357db8 to 6187983 Compare May 2, 2023 00:14

tcharding previously approved these changes May 2, 2023

View reviewed changes

wpaulino dismissed tcharding’s stale review via f95a3e9 May 2, 2023 02:17

wpaulino force-pushed the impl-shift-target branch from 6187983 to f95a3e9 Compare May 2, 2023 02:17

tcharding reviewed May 2, 2023

View reviewed changes

Kixunil reviewed May 2, 2023

View reviewed changes

Expose valid (min, max) difficulty transition thresholds

8e6f953

Once `U256` was made private, we lost the ability to check whether a valid difficulty transition was made in the chain, since `Target` no longer exposes any arithmetic operations.

wpaulino force-pushed the impl-shift-target branch from f95a3e9 to 8e6f953 Compare May 4, 2023 19:42

Kixunil approved these changes May 4, 2023

View reviewed changes

sanket1729 previously approved these changes May 4, 2023

View reviewed changes

sanket1729 approved these changes May 6, 2023

View reviewed changes

apoelstra approved these changes May 6, 2023

View reviewed changes

apoelstra merged commit 4abbdc2 into rust-bitcoin:master May 6, 2023
10 checks passed

wpaulino deleted the impl-shift-target branch May 6, 2023 19:49

vincenzopalazzo mentioned this pull request Aug 13, 2023

Adds possibility to make math operations on the Target type #1993

Closed

This was referenced Nov 2, 2023

WIP: start trying to upgrade bitcoin lightningdevkit/rust-lightning#2701

Closed

rust-lightning is using << and >> on pow target values #2161

Closed

tcharding mentioned this pull request Nov 16, 2023

Backport #1820 #2195

Closed

tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Nov 16, 2023

Bump version to 0.30.2

ba99aa4

We just backported PR rust-bitcoin#1820, add a changelog entry and bump the crate version.

tcharding mentioned this pull request Nov 16, 2023

Backport #1820 and bump minor version 0.30.2 #2196

Merged

Expose valid (min, max) difficulty transition thresholds #1820

Expose valid (min, max) difficulty transition thresholds #1820

Conversation

wpaulino commented Apr 28, 2023

tcharding left a comment

Choose a reason for hiding this comment

Kixunil commented Apr 29, 2023

apoelstra commented Apr 29, 2023

apoelstra commented Apr 29, 2023 • edited

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Apr 30, 2023

apoelstra commented Apr 30, 2023

wpaulino commented May 1, 2023

TheBlueMatt commented May 1, 2023

apoelstra commented May 1, 2023

wpaulino commented May 1, 2023

tcharding commented May 2, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented May 2, 2023

apoelstra commented May 2, 2023

wpaulino commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

wpaulino commented May 4, 2023

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 commented May 6, 2023 • edited

apoelstra commented May 6, 2023

apoelstra commented May 6, 2023 • edited by RCasatta

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Nov 2, 2023

tcharding commented Nov 2, 2023

apoelstra commented Apr 29, 2023 •

edited

sanket1729 commented May 6, 2023 •

edited

apoelstra commented May 6, 2023 •

edited by RCasatta