Upgrade rust-bitcoin dependency to 0.31 #3063

jirijakes · 2024-05-12T07:33:33Z

Due to the commit compile check, the change comes as one big commit.

Commits do not compile on their own at the momet. I wanted to attract reviewers' attention to a few specific changes, so they have dedicated commits. Commits Upgrade rust-bitcoin to 0.31* are relatively uninvolved, so is Resolve deprecations, however the remaining commits bring some changes that I want to point out.

~~At the end, all commits will be squashed into one big.~~

Please let me know what else I forgot and what could be done better and easier for reviewing.

Closes #2745.

Notes

Bech32

v0.31 started to use bech32 v0.10, which is a complete rewrite of the library. This PR keeps using bech32 v0.9.1, it just made it explicit (previously was via rust-bitcoin). Upgrade would require many more changes (FromBase32 and u5 were removed). Dependency tree will contain both versions of bech32, but they should not be in conflict since v0.31 uses it only internally.

Amount type

v0.31 uses Amount more often, newly for example in TxOut's value. Since this field is used quite often in rust-lightning, this PR accommodates to it by changing some occurrrences of using u64 for amount of satoshi into Amount. I did not try to change all such cases; I usually set the boundary on public interfaces.

In tests, I used all existing calculations in u64, converted to Amount only at the end.

Transaction weight

Calculation of transaction weight in v0.31 was corrected and that broke some tests.

Most notable change is in Fix test_tx_change_edge commit.

PSBT

Psbt::extract_tx() newly checks for using too high a fee rate but requires prevouts. Substituted by Psbt::extract_tx_unchecked_fee_rate() which behaves the same as Psbt::extract_tx() in v0.30.

Broken tests

transaction_utils / test_tx_change_edge

Error message

---- util::transaction_utils::tests::test_tx_change_edge stdout ---- thread 'util::transaction_utils::tests::test_tx_change_edge' panicked at lightning/src/util/transaction_utils.rs:235:9: assertion left == right failed left: 42 right: 40

Due to corrected weight calculation of transactions in v0.31, the test had to be adjusted to reflect that.

lightning-block-sync

Error message

---- convert::tests::into_txid_from_json_response_with_invalid_hex_data stdout ---- thread 'convert::tests::into_txid_from_json_response_with_invalid_hex_data' panicked at lightning-block-sync/src/convert.rs:610:17: assertion left == right failed left: "bad hex string length 64 (expected 6)" right: "bad hex string length 6 (expected 64)" note: run with RUST_BACKTRACE=1 environment variable to display a backtrace ---- convert::tests::into_txid_from_json_response_with_invalid_txid_data stdout ---- thread 'convert::tests::into_txid_from_json_response_with_invalid_txid_data' panicked at lightning-block-sync/src/convert.rs:622:17: assertion left == right failed left: "bad hex string length 64 (expected 4)" right: "bad hex string length 4 (expected 64)"

~~Bug in hex-conservative in error message of parsing hash types from string. PR #88 should fix it.~~

Fixed in hex-conservative v0.1.2

Fuzz

Error message

---- onion_message::tests::test_no_onion_message_breakage stdout ---- thread 'onion_message::tests::test_no_onion_message_breakage' panicked at 'assertion failed: (left == right) left: None, right: Some(1)', src/onion_message.rs:285:13 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace ---- full_stack::tests::test_no_existing_test_breakage stdout ---- DEBUG [lightning::ln::peer_handler : /home/jiri/dev/btc/ldk/rust-lightning/bitcoin032/lightning/src/ln/peer_handler.rs, 1473] Error handling message; disconnecting peer with: Bad MAC TRACE [lightning::ln::peer_handler : /home/jiri/dev/btc/ldk/rust-lightning/bitcoin032/lightning/src/ln/peer_handler.rs, 1328] Disconnecting peer due to a protocol error (usually a duplicate connection). thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: (left == right) left: None, right: Some(1)', src/full_stack.rs:1319:9 ---- full_stack::tests::test_gossip_exchange_breakage stdout ---- DEBUG [lightning::ln::peer_handler : /home/jiri/dev/btc/ldk/rust-lightning/bitcoin032/lightning/src/ln/peer_handler.rs, 1473] Error handling message; disconnecting peer with: Bad MAC TRACE [lightning::ln::peer_handler : /home/jiri/dev/btc/ldk/rust-lightning/bitcoin032/lightning/src/ln/peer_handler.rs, 1328] Disconnecting peer due to a protocol error (usually a duplicate connection). thread 'full_stack::tests::test_gossip_exchange_breakage' panicked at 'assertion failed: (left == right) left: None, right: Some(1)', src/full_stack.rs:1430:9

Dependencies secp256k1 and bitcoin_hashes newly need dedicated flags to enable fuzzing, cfg=secp256k1_fuzz and cfg=hashes_fuzz.

TheBlueMatt

Thanks for taking a crack at this! I just skimmed the diff, looking forward to you addressing the remaining issues :). Note that our CI is insistent on each individual commit compiling, so you may end up squashing most of this PR into one commit.

TheBlueMatt · 2024-05-13T20:45:35Z

lightning/src/ln/chan_utils.rs

@@ -542,6 +545,13 @@ pub struct HTLCOutputInCommitment {
 	pub transaction_output_index: Option<u32>,
 }

+impl HTLCOutputInCommitment {
+	/// Returns value of the HTLC.
+	pub const fn amount(&self) -> Amount {


This is a confusing API given an HTLCOutput is for an amount denominated in msat, but this is rounding to sats.

In the latest version, I named it to_bitcoin_amount() but I believe there may be better names. Is there some preference?

to_btc_amount()

to_amount()

to_amount_sat()

lightning/src/ln/channel.rs

TheBlueMatt · 2024-05-14T19:37:27Z

Please let us know when you want some rounds of review by leaving a comment!

jirijakes · 2024-05-14T21:02:41Z

Please let us know when you want some rounds of review by leaving a comment!

Yes, I will. Thanks!

codecov-commenter · 2024-05-19T02:54:14Z

Codecov Report

Attention: Patch coverage is 94.75891% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 89.87%. Comparing base (df01208) to head (a8bd4c0).

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	82.14%	1 Missing and 4 partials ⚠️
lightning/src/events/bump_transaction.rs	82.75%	5 Missing ⚠️
lightning/src/ln/chan_utils.rs	95.31%	3 Missing ⚠️
lightning/src/util/ser.rs	66.66%	1 Missing and 2 partials ⚠️
lightning/src/ln/functional_tests.rs	93.33%	1 Missing and 1 partial ⚠️
lightning-invoice/src/ser.rs	0.00%	0 Missing and 1 partial ⚠️
lightning/src/events/mod.rs	0.00%	1 Missing ⚠️
lightning/src/ln/channel.rs	95.00%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_test_utils.rs	90.90%	0 Missing and 1 partial ⚠️
lightning/src/ln/shutdown_tests.rs	87.50%	1 Missing ⚠️
... and 2 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3063      +/-   ##
==========================================
- Coverage   89.90%   89.87%   -0.04%     
==========================================
  Files         117      117              
  Lines       97105    97134      +29     
  Branches    97105    97134      +29     
==========================================
- Hits        87303    87298       -5     
- Misses       7243     7270      +27     
- Partials     2559     2566       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jirijakes · 2024-05-19T09:19:54Z

This is ready for review.

I still kept the PR broken into multiple commits that do not compile on their own. This way I can point attention to a few specific changes. I will squash all into one commit after review.

Thanks, Matt, for early quick review!

tnull

Did a first pass, mostly looks good to me, just a few comments.

I really like the refactor to use bitcoin::Amount in more and more places, as it nicely prepares for #2076.

lightning/src/ln/chan_utils.rs

tnull · 2024-05-21T10:48:31Z

lightning/src/ln/chan_utils.rs

 			let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
 			    get_to_countersignatory_with_anchors_redeemscript(&countersignatory_pubkeys.payment_point).to_v0_p2wsh()
 			} else {
-			    Payload::p2wpkh(&BitcoinPublicKey::new(countersignatory_pubkeys.payment_point)).unwrap().script_pubkey()
+				ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_pubkeys.payment_point.serialize()).into())


Maybe worth to still use bitcoin::key::PublicKey to be able to use wpubkey_hash here?

Mmm, I don't know. While the version with bitcoin::key::PublicKey is a bit more understandable in the intent, it is more verbose and requires an expect().

So far I kept it but if you think that it would still be better to use bitcoin::key::PublicKey, I will change.

Thanks!

Getting rid of an unwrap() and some verbosity sounds fair and I don't think this reduces clarity that much since intent is clear from new_p2wpkh.

lightning/src/sign/mod.rs

jirijakes · 2024-05-23T14:09:52Z

Addressed tnull's comments and rebased.

dunxen

Looks good! Just some minor comments.

If @TheBlueMatt and @tnull are happy with the changes, I think this can be squashed :)

dunxen · 2024-05-28T07:23:56Z

lightning/src/ln/chan_utils.rs

 			let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
 			    get_to_countersignatory_with_anchors_redeemscript(&countersignatory_pubkeys.payment_point).to_v0_p2wsh()
 			} else {
-			    Payload::p2wpkh(&BitcoinPublicKey::new(countersignatory_pubkeys.payment_point)).unwrap().script_pubkey()
+				ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_pubkeys.payment_point.serialize()).into())


Getting rid of an unwrap() and some verbosity sounds fair and I don't think this reduces clarity that much since intent is clear from new_p2wpkh.

dunxen · 2024-05-28T08:01:59Z

fuzz/Cargo.toml

@@ -21,7 +21,8 @@ stdin_fuzz = []
 lightning = { path = "../lightning", features = ["regex", "hashbrown", "_test_utils"] }
 lightning-invoice = { path = "../lightning-invoice" }
 lightning-rapid-gossip-sync = { path = "../lightning-rapid-gossip-sync" }
-bitcoin = { version = "0.30.2", features = ["secp-lowmemory"] }
+bech32 = "0.9.1"


I believe [email protected] is also fine for [email protected], but I guess we'll bump it with the next PR for [email protected]

There was no 0.10.0 final release (only beta) and it also contains the complete rewrite. So using 0.10 would not help as it would require to rewrite everything in LDK that uses bech32. That's the main reason for sticking to 0.9, so that LDK does not need to rewrite usage of bech32 yet.

Ah I see. Thanks!

lightning/src/util/transaction_utils.rs

jirijakes · 2024-05-30T10:38:33Z

Addressed @dunxen's comments (thanks!), fixed commit compile check, rebased and finally squashed (otherwise commits would not compile on their own).

dunxen

LGTM let's see if CI is happy (besides any unrelated stuff). Then I'll let @tnull and @TheBlueMatt take a final look. Not sure if anything needs to get in before this but I doubt it will create anything more than a few simple conflicts.

tnull

LGTM

Given the size of the changeset, three pairs of eyes can't hurt though, so I'll defer to @TheBlueMatt for the final ACK.

lightning/src/util/ser.rs

tnull · 2024-05-31T13:26:33Z

lightning/src/util/transaction_utils.rs

+		// base size = version size + varint[input count] + input size + varint[output count] + output size + lock time size
+		// total size = version size + marker + flag + varint[input count] + input size + varint[output count] + output size + lock time size
+		// weight = 3 * base size + total size = 3 * (4 + 1 + 0 + 1 + 0 + 4) + (4 + 1 + 1 + 1 + 0 + 1 + 0 + 4) = 3 * 10 + 12 = 42
+		assert_eq!(tx.weight().to_wu(), 42);


Do we know where this discrepancy (40 before, now 42) comes from?

rust-bitcoin changed the way how weight was calculated. Probably the previous version was not accurate: rust-bitcoin/rust-bitcoin#2049

TheBlueMatt

Thanks! IMO we should rename to_bitcoin_amount in a followup, and should also obviously upgrade bech32.

TheBlueMatt · 2024-05-31T18:06:31Z

fuzz/src/bin/base32_target.rs

@@ -15,6 +15,12 @@
 #[cfg(not(fuzzing))]
 compile_error!("Fuzz targets need cfg=fuzzing");

+#[cfg(not(hashes_fuzz))]


These changes should have been in a separate commit.

TheBlueMatt · 2024-05-31T18:33:37Z

lightning-invoice/src/lib.rs

@@ -878,8 +878,7 @@ impl SignedRawBolt11Invoice {

 	/// Recovers the public key used for signing the invoice from the recoverable signature.
 	pub fn recover_payee_pub_key(&self) -> Result<PayeePubKey, secp256k1::Error> {
-		let hash = Message::from_slice(&self.hash[..])
-			.expect("Hash is 32 bytes long, same as MESSAGE_SIZE");
+		let hash = Message::from_digest(self.hash);


Same here, the old fn still exists, its just deprecated, so we should have fixed it in a separate commit.

TheBlueMatt · 2024-05-31T19:18:41Z

lightning/src/ln/chan_utils.rs

+	/// Converts HTLC's value with millisatoshi precision into [bitcoin::Amount] with satoshi precision.
+	/// Typically this conversion is needed when transitioning from LN into base-layer Bitcoin,
+	/// e. g. in commitment transactions.
+	pub const fn to_bitcoin_amount(&self) -> Amount {


I'd kinda prefer if we called this satoshi_amount (its not a to_ because we're not converting the object, and its using satoshi (or sats or whatever) improves readability at the callsites (where bitcoin_amount might imply the amount in whole bitcoins).

TheBlueMatt reviewed May 13, 2024

View reviewed changes

jirijakes force-pushed the upgrade-bitcoin-031 branch from 629ebcb to 9b2ca1c Compare May 14, 2024 13:52

jirijakes force-pushed the upgrade-bitcoin-031 branch 2 times, most recently from a8eee16 to 73d4341 Compare May 19, 2024 02:46

jirijakes force-pushed the upgrade-bitcoin-031 branch 3 times, most recently from 042bc27 to 4aae621 Compare May 19, 2024 08:08

jirijakes changed the title ~~[Draft] Upgrade rust-bitcoin dependency to 0.31~~ Upgrade rust-bitcoin dependency to 0.31 May 19, 2024

jirijakes marked this pull request as ready for review May 19, 2024 09:18

This was referenced May 19, 2024

Upgrade esplora-client to 0.7 #3070

Closed

upgrade esplora-client to v0.7 #3071

Closed

tnull reviewed May 21, 2024

View reviewed changes

jirijakes force-pushed the upgrade-bitcoin-031 branch from 4aae621 to 32de8fe Compare May 23, 2024 12:52

dunxen reviewed May 28, 2024

View reviewed changes

thesimplekid mentioned this pull request May 29, 2024

Bump Deps cashubtc/cdk#147

Closed

jirijakes force-pushed the upgrade-bitcoin-031 branch from 32de8fe to 1daaeda Compare May 30, 2024 10:25

Upgrade rust-bitcoin to 0.31

a8bd4c0

jirijakes force-pushed the upgrade-bitcoin-031 branch from 1daaeda to a8bd4c0 Compare May 30, 2024 10:35

dunxen approved these changes May 30, 2024

View reviewed changes

tnull reviewed May 31, 2024

View reviewed changes

tnull approved these changes May 31, 2024

View reviewed changes

brunoerg mentioned this pull request May 31, 2024

Introduce mutation testing #2763

Merged

TheBlueMatt approved these changes May 31, 2024

View reviewed changes

TheBlueMatt merged commit bfda1b6 into lightningdevkit:main May 31, 2024
15 of 16 checks passed

jirijakes mentioned this pull request Jun 6, 2024

Rename Htlc::to_bitcoin_amount to satoshi_amount #3099

Draft

tnull mentioned this pull request Jun 6, 2024

Upgrade to rust-bitcoin 0.32 #3100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade rust-bitcoin dependency to 0.31 #3063

Upgrade rust-bitcoin dependency to 0.31 #3063

jirijakes commented May 12, 2024 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt May 13, 2024

jirijakes May 19, 2024

TheBlueMatt commented May 14, 2024

jirijakes commented May 14, 2024

codecov-commenter commented May 19, 2024 •

edited

Loading

jirijakes commented May 19, 2024 •

edited

Loading

tnull left a comment

tnull May 21, 2024

jirijakes May 23, 2024

dunxen May 28, 2024

jirijakes commented May 23, 2024

dunxen left a comment

dunxen May 28, 2024

dunxen May 28, 2024

jirijakes May 30, 2024

dunxen May 30, 2024

jirijakes commented May 30, 2024 •

edited

Loading

dunxen left a comment •

edited

Loading

tnull left a comment

tnull May 31, 2024

jirijakes May 31, 2024

TheBlueMatt left a comment

TheBlueMatt May 31, 2024

TheBlueMatt May 31, 2024

TheBlueMatt May 31, 2024

Upgrade rust-bitcoin dependency to 0.31 #3063

Upgrade rust-bitcoin dependency to 0.31 #3063

Conversation

jirijakes commented May 12, 2024 • edited Loading

Notes

Bech32

Amount type

Transaction weight

PSBT

Broken tests

transaction_utils / test_tx_change_edge

lightning-block-sync

Fuzz

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 14, 2024

jirijakes commented May 14, 2024

codecov-commenter commented May 19, 2024 • edited Loading

Codecov Report

jirijakes commented May 19, 2024 • edited Loading

tnull 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

jirijakes commented May 23, 2024

dunxen 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

jirijakes commented May 30, 2024 • edited Loading

dunxen left a comment • edited Loading

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt 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

jirijakes commented May 12, 2024 •

edited

Loading

codecov-commenter commented May 19, 2024 •

edited

Loading

jirijakes commented May 19, 2024 •

edited

Loading

jirijakes commented May 30, 2024 •

edited

Loading

dunxen left a comment •

edited

Loading