Update bitcoin crate to 0.28.1 by devrandom · Pull Request #1389 · lightningdevkit/rust-lightning

devrandom · 2022-03-27T10:33:57Z

keys
witness
deprecations

devrandom · 2022-03-27T12:28:28Z

is this a good time to do this? is the rust-bitcoin release imminent?

TheBlueMatt · 2022-03-27T15:45:07Z

Yea, I hear its coming so update is always good. One question before we pull the trigger on anything though is the WASM issues in rust-secp - rust-bitcoin/rust-secp256k1#427 and rust-bitcoin/rust-secp256k1#419

devrandom · 2022-03-28T09:50:27Z

Looks like the 427 is really just the dev missing feature flag. And 419 seems to be progressing.

codecov-commenter · 2022-05-03T12:17:47Z

Codecov Report

Merging #1389 (a650159) into main (e3a305c) will increase coverage by 0.50%.
The diff coverage is 98.36%.

@@            Coverage Diff             @@
##             main    #1389      +/-   ##
==========================================
+ Coverage   90.89%   91.40%   +0.50%     
==========================================
  Files          75       75              
  Lines       41624    45218    +3594     
  Branches    41624    45218    +3594     
==========================================
+ Hits        37834    41330    +3496     
- Misses       3790     3888      +98

Impacted Files	Coverage Δ
lightning-invoice/src/de.rs	`82.38% <ø> (ø)`
lightning-invoice/src/utils.rs	`96.85% <ø> (ø)`
lightning-net-tokio/src/lib.rs	`82.32% <ø> (+6.43%)`	⬆️
lightning/src/ln/peer_handler.rs	`59.45% <ø> (+6.83%)`	⬆️
lightning/src/util/events.rs	`33.21% <ø> (-0.35%)`	⬇️
lightning/src/util/macro_logger.rs	`85.48% <ø> (ø)`
lightning/src/util/ser.rs	`91.28% <ø> (-0.19%)`	⬇️
lightning/src/util/test_utils.rs	`87.94% <ø> (+5.51%)`	⬆️
lightning-block-sync/src/test_utils.rs	`93.46% <85.71%> (-0.42%)`	⬇️
lightning/src/routing/network_graph.rs	`89.88% <90.00%> (ø)`
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3a305c...a650159. Read the comment docs.

devrandom · 2022-05-03T14:10:12Z

Note that MSRV for no-std changed to 1.53.0 because of https://github.com/lightningdevkit/rust-lightning/runs/6272782644 - DebugStruct::finish_non_exhaustive

dunxen · 2022-05-03T14:25:58Z

Light review ACK

LGTM. Will take a closer look at the larger diffs in little while!

devrandom · 2022-05-03T14:29:41Z

Note that MSRV for no-std changed to 1.53.0 because of https://github.com/lightningdevkit/rust-lightning/runs/6272782644 - DebugStruct::finish_non_exhaustive

Looks like that was an accident. Hopefully rust-bitcoin/rust-bitcoin#690 will be included in a point release - I'll revert the MSRV changes once that's done.

TheBlueMatt · 2022-05-03T15:52:44Z

If upstream is gonna do a point release soon lets just wait for that here, IMO.

TheBlueMatt · 2022-05-04T18:02:39Z

Upstream cut a point release so this should be good now, I think, need to re-add the MSRV changes in CI.

devrandom · 2022-05-04T18:48:18Z

Upstream cut a point release so this should be good now, I think, need to re-add the MSRV changes in CI.

Done.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt

LGTM! Thanks for tackling this one!

TheBlueMatt · 2022-05-05T03:32:06Z

lightning/src/chain/channelmonitor.rs

-				$sum_actual_sigs += $sighash_parts.access_witness($idx)[0].len();
+				let sighash = hash_to_message!(&$sighash_parts.segwit_signature_hash($idx, &redeem_script, $amount, EcdsaSighashType::All).unwrap()[..]);
+				let sig = secp_ctx.sign_ecdsa(&sighash, &privkey);
+				let mut ser_sig = sig.serialize_der().to_vec();


I was gonna nag about the extra allocation here, but instead I just did the util method upstream so we can fix it later - rust-bitcoin/rust-bitcoin#989

tnull

AFAICT looks good to me. Only added two minor remarks and a question. Feel free to ignore!

lightning-block-sync/src/test_utils.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/keysinterface.rs

dunxen

LGTM now!

lightning-block-sync/src/test_utils.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/onion_route_tests.rs

lightning/src/ln/script.rs

lightning/src/ln/chan_utils.rs

devrandom force-pushed the 2022-03-bitcoin branch from 75eef02 to af9ba7e Compare May 3, 2022 12:00

devrandom marked this pull request as ready for review May 3, 2022 12:01

devrandom force-pushed the 2022-03-bitcoin branch from af9ba7e to c14b28d Compare May 3, 2022 12:06

devrandom force-pushed the 2022-03-bitcoin branch from c14b28d to 2e3b81d Compare May 3, 2022 14:08

devrandom force-pushed the 2022-03-bitcoin branch from 6dc2510 to 7b65d22 Compare May 4, 2022 18:26

TheBlueMatt added the Seeking Code Review label May 4, 2022

devrandom changed the title ~~Update bitcoin to 0.28.0~~ Update bitcoin to 0.28.1 May 4, 2022

devrandom changed the title ~~Update bitcoin to 0.28.1~~ Update bitcoin crate to 0.28.1 May 4, 2022

TheBlueMatt reviewed May 4, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

devrandom force-pushed the 2022-03-bitcoin branch from 7b65d22 to 2bffd46 Compare May 4, 2022 21:45

TheBlueMatt previously approved these changes May 5, 2022

View reviewed changes

tnull previously approved these changes May 5, 2022

View reviewed changes

lightning-block-sync/src/test_utils.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/keysinterface.rs Show resolved Hide resolved

devrandom dismissed stale reviews from tnull and TheBlueMatt via e23de53 May 5, 2022 10:08

devrandom force-pushed the 2022-03-bitcoin branch from 2bffd46 to e23de53 Compare May 5, 2022 10:08

dunxen previously approved these changes May 5, 2022

View reviewed changes

devrandom mentioned this pull request May 5, 2022

[backport] Fix no_std MSRV rust-bitcoin/rust-bitcoin#986

Merged

tnull previously approved these changes May 5, 2022

View reviewed changes

jkczyz reviewed May 5, 2022

View reviewed changes

bitcoin crate 0.28.1

28d33ff

Improve ShutdownScript::new_witness_program

a650159

devrandom dismissed stale reviews from tnull and dunxen via a650159 May 5, 2022 16:05

devrandom force-pushed the 2022-03-bitcoin branch from e23de53 to a650159 Compare May 5, 2022 16:05

tnull approved these changes May 5, 2022

View reviewed changes

TheBlueMatt approved these changes May 5, 2022

View reviewed changes

jkczyz approved these changes May 5, 2022

View reviewed changes

jkczyz merged commit 6592081 into lightningdevkit:main May 5, 2022

devrandom removed the Seeking Code Review label May 9, 2022

Conversation

devrandom commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Mar 27, 2022

Uh oh!

TheBlueMatt commented Mar 27, 2022

Uh oh!

devrandom commented Mar 28, 2022

Uh oh!

codecov-commenter commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devrandom commented May 3, 2022

Uh oh!

dunxen commented May 3, 2022

Uh oh!

devrandom commented May 3, 2022

Uh oh!

TheBlueMatt commented May 3, 2022

Uh oh!

TheBlueMatt commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented May 4, 2022

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 5, 2022

Choose a reason for hiding this comment

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dunxen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

devrandom commented Mar 27, 2022 •

edited

Loading

codecov-commenter commented May 3, 2022 •

edited

Loading

TheBlueMatt commented May 4, 2022 •

edited

Loading

tnull left a comment •

edited

Loading