Update bitcoin crate to 0.28.1 #1389

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update bitcoin crate to 0.28.1 #1389

Update bitcoin crate to 0.28.1 #1389

devrandom commented Mar 27, 2022 •

edited

Loading

devrandom commented Mar 27, 2022

TheBlueMatt commented Mar 27, 2022

devrandom commented Mar 28, 2022

codecov-commenter commented May 3, 2022 •

edited

Loading

devrandom commented May 3, 2022

dunxen commented May 3, 2022

devrandom commented May 3, 2022

TheBlueMatt commented May 3, 2022

TheBlueMatt commented May 4, 2022 •

edited

Loading

devrandom commented May 4, 2022

TheBlueMatt left a comment

TheBlueMatt May 5, 2022

tnull left a comment •

edited

Loading

dunxen left a comment

Update bitcoin crate to 0.28.1 #1389

Update bitcoin crate to 0.28.1 #1389

Conversation

devrandom commented Mar 27, 2022 • edited Loading

devrandom commented Mar 27, 2022

TheBlueMatt commented Mar 27, 2022

devrandom commented Mar 28, 2022

codecov-commenter commented May 3, 2022 • edited Loading

Codecov Report

devrandom commented May 3, 2022

dunxen commented May 3, 2022

devrandom commented May 3, 2022

TheBlueMatt commented May 3, 2022

TheBlueMatt commented May 4, 2022 • edited Loading

devrandom commented May 4, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt May 5, 2022

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

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