Bitcoin 0.32.2 upgrade #3239

arik-so · 2024-08-13T21:43:04Z

As the title suggests, but still unfortunately a work in progress.

codecov · 2024-08-13T23:29:35Z

Codecov Report

Attention: Patch coverage is 82.44804% with 76 lines in your changes missing coverage. Please review.

Project coverage is 90.39%. Comparing base (5ab40b2) to head (36b484a).
Report is 48 commits behind head on main.

Files	Patch %	Lines
lightning/src/offers/invoice.rs	57.14%	0 Missing and 9 partials ⚠️
lightning/src/io/mod.rs	77.77%	6 Missing and 2 partials ⚠️
lightning/src/routing/router.rs	0.00%	7 Missing ⚠️
lightning/src/util/sweep.rs	12.50%	6 Missing and 1 partial ⚠️
lightning-rapid-gossip-sync/src/processing.rs	25.00%	0 Missing and 6 partials ⚠️
lightning/src/util/ser.rs	87.23%	2 Missing and 4 partials ⚠️
lightning/src/chain/onchaintx.rs	50.00%	5 Missing ⚠️
lightning/src/events/bump_transaction.rs	33.33%	3 Missing and 1 partial ⚠️
lightning-invoice/src/lib.rs	84.21%	3 Missing ⚠️
lightning-persister/src/fs_store.rs	62.50%	3 Missing ⚠️
... and 12 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3239      +/-   ##
==========================================
+ Coverage   89.75%   90.39%   +0.63%     
==========================================
  Files         123      124       +1     
  Lines      102158   107249    +5091     
  Branches   102158   107249    +5091     
==========================================
+ Hits        91695    96947    +5252     
+ Misses       7773     7717      -56     
+ Partials     2690     2585     -105

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

lightning-rapid-gossip-sync/src/lib.rs

lightning/src/lib.rs

TheBlueMatt · 2024-08-14T20:00:39Z

lightning/src/lib.rs

@@ -144,7 +299,7 @@ pub mod io_extras {
 		Ok(count)
 	}

-	pub fn read_to_end<D: io::Read>(mut d: D) -> Result<alloc::vec::Vec<u8>, io::Error> {
+	pub fn read_to_end<D: Read>(mut d: &mut D) -> Result<alloc::vec::Vec<u8>, io::Error> {


Can this be dropped in favor of read_to_limit?

It can be, but would probably be better in a followup because it does produce some errors.

lightning/src/util/ser.rs

lightning/src/blinded_path/mod.rs

TheBlueMatt · 2024-08-14T20:10:04Z

We'll probably end up with a handful of followups to upstream stuff to bitcoin-io/drop our usage of Cursor/remove no-std features, but they can be in a followup.

lightning-block-sync/src/utils.rs

fuzz/src/full_stack.rs

lightning-rapid-gossip-sync/src/processing.rs

lightning/src/lib.rs

lightning/src/offers/invoice.rs

lightning/src/util/macro_logger.rs

lightning-transaction-sync/Cargo.toml

lightning-transaction-sync/src/electrum.rs

lightning/src/io/mod.rs

tnull

Did a first pass. Already looks pretty good.

AFAICT, you can (and should) drop most to almost all (?) changes related to lightning-transaction-sync if you'd simply bump rust-esplora-client and rust-electrum-client to their latest versions which have already been upgraded to rust-bitcoin 0.32.

lightning/src/offers/parse.rs

lightning/src/util/ser_macros.rs

lightning-transaction-sync/src/util.rs

lightning-invoice/Cargo.toml

lightning-rapid-gossip-sync/src/lib.rs

lightning-rapid-gossip-sync/src/processing.rs

lightning/src/util/ser.rs

In anticipation of the rust-bitcoin upgrade, which incorporates its own `io::Read` implementation, we need to make our usage compatible with dropping `std::io` and `core2::io`. Notably, in version 0.32.2, `bitcoin::io`'s `Read` is no longer implemented for `&mut R where R: Read + ?Sized`, which results in errors anytime `&mut &mut Readable` is passed instead of `&mut Readable`. This commit fixes those instances.

In lieu of using `Seek` and `SeekFrom`, we will instead rely exclusively on the `io::Cursor` type.

The rust-bitcoin upgrade will introduce `bitcoin::io` module, which will be missing a necessary subset of traits. To accommodate those traits' future implementations, we move the `lightning::io` module to its own file, where we will be able to implement the missing trait subset in the next commit.

The `bitcoin::io` Cursor will miss a bunch of functionality that we were using that we will reimplement here, most critically `set_position`.

TheBlueMatt

Basically LGTM. Feel free to squash and I think we should just land this and then do fixups later.

TheBlueMatt · 2024-08-16T17:30:13Z

lightning/src/util/ser.rs

+impl<'a, R: Read> Read for BufReader<'a, R> {
+	#[inline]
+	fn read(&mut self, output: &mut [u8]) -> io::Result<usize> {
+		let input = self.fill_buf()?;


Technically if we want to get fancy we can ignore the whole buffer nonsense here and actually just do a straight read, but I'm also hoping this code never makes it into a release at all (ie rust-bitcoin updates fast enough) so its not worth worrying about too much.

Taproot is not yet being actively used, and this commit suppresses the noisy warning generation.

Version 0.32.2 of `rust-bitcoin` deprecates a number of methods that are commonly used in this project, most visibly `txid()`, which is now called `compute_txid()`. This resulted in a lot of warnings, and this commit is part of a series that seeks to address that.

TheBlueMatt

LGTM. There's probably a few things to clean up here and there but we should land this to get it out of the way and can clean up in a followup.

tnull

Post-merge ACK

arik-so force-pushed the bitcoin-0.32.2-upgrade branch from 2bf94c4 to db61c59 Compare August 13, 2024 23:12

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 12 times, most recently from 458d5cf to 9d5a386 Compare August 14, 2024 16:33

arik-so marked this pull request as ready for review August 14, 2024 18:03

arik-so force-pushed the bitcoin-0.32.2-upgrade branch from 4c829f4 to 203803c Compare August 14, 2024 19:02

TheBlueMatt reviewed Aug 14, 2024

View reviewed changes

lightning-block-sync/src/utils.rs Show resolved Hide resolved

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 7 times, most recently from 48bb54e to 07e0600 Compare August 15, 2024 01:13

TheBlueMatt reviewed Aug 15, 2024

View reviewed changes

lightning-transaction-sync/src/electrum.rs Outdated Show resolved Hide resolved

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 2 times, most recently from 215b556 to df9aa8f Compare August 15, 2024 06:57

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 3 times, most recently from c6a34da to b72b5e9 Compare August 15, 2024 16:57

tnull self-requested a review August 15, 2024 17:35

TheBlueMatt reviewed Aug 15, 2024

View reviewed changes

lightning/src/io/mod.rs Outdated Show resolved Hide resolved

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 2 times, most recently from 4b824f5 to 768c040 Compare August 15, 2024 20:22

arik-so requested a review from TheBlueMatt August 15, 2024 21:41

arik-so force-pushed the bitcoin-0.32.2-upgrade branch 2 times, most recently from ebb80e6 to 7462995 Compare August 16, 2024 06:19

tnull reviewed Aug 16, 2024

View reviewed changes

arik-so added 4 commits August 16, 2024 09:23

Drop Seek and SeekFrom usage.

fb818e0

In lieu of using `Seek` and `SeekFrom`, we will instead rely exclusively on the `io::Cursor` type.

Reimplement io::Cursor.

40b0400

The `bitcoin::io` Cursor will miss a bunch of functionality that we were using that we will reimplement here, most critically `set_position`.

arik-so force-pushed the bitcoin-0.32.2-upgrade branch from 7462995 to e07cccb Compare August 16, 2024 16:43

TheBlueMatt reviewed Aug 16, 2024

View reviewed changes

arik-so added 8 commits August 16, 2024 10:31

Upgrade rust-bitcoin to 0.32.2.

176d2ad

Remove secp256k1 dependency.

0cc9378

Suppress Taproot warnings.

60ca255

Taproot is not yet being actively used, and this commit suppresses the noisy warning generation.

Fix fuzz warnings.

36b484a

Version 0.32.2 of `rust-bitcoin` deprecates a number of methods that are commonly used in this project, most visibly `txid()`, which is now called `compute_txid()`. This resulted in a lot of warnings, and this commit is part of a series that seeks to address that.

arik-so force-pushed the bitcoin-0.32.2-upgrade branch from e07cccb to 36b484a Compare August 16, 2024 17:32

TheBlueMatt approved these changes Aug 16, 2024

View reviewed changes

TheBlueMatt merged commit 43dcf2f into lightningdevkit:main Aug 16, 2024
18 of 21 checks passed

tnull reviewed Aug 17, 2024

View reviewed changes

TheBlueMatt mentioned this pull request Aug 19, 2024

Upgrade to rust-bitcoin 0.32 #3100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitcoin 0.32.2 upgrade #3239

Bitcoin 0.32.2 upgrade #3239

arik-so commented Aug 13, 2024

codecov bot commented Aug 13, 2024 •

edited

Loading

TheBlueMatt Aug 14, 2024

arik-so Aug 14, 2024

TheBlueMatt commented Aug 14, 2024

tnull left a comment

TheBlueMatt left a comment

TheBlueMatt Aug 16, 2024

TheBlueMatt left a comment

tnull left a comment

Bitcoin 0.32.2 upgrade #3239

Bitcoin 0.32.2 upgrade #3239

Conversation

arik-so commented Aug 13, 2024

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

TheBlueMatt Aug 14, 2024

Choose a reason for hiding this comment

arik-so Aug 14, 2024

Choose a reason for hiding this comment

TheBlueMatt commented Aug 14, 2024

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Aug 16, 2024

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2024 •

edited

Loading