lightningdevkit
[Draft] Upgrade rust-bitcoin dependency to 0.31
Commits do not compile on their own. I guess at the end they will be squashed into one, but for ease of reviewing, I broke them up.
Please let me know what else I forgot and what could be done better and easier for reviewing.
- [X] bump bitcoin dependency to v0.31.2 in all subcrates
- [X] fix compilation errors
- [X] fix new deprecations
- [ ] fix broken tests
- [ ] fix std/no-std
- [ ] musig2: upgrade to v0.31 (https://github.com/arik-so/rust-musig2/pull/11)
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.
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
This has something to do with corrected calculation of transaction weight. Have not investigated yet in depth.
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.
Please let us know when you want some rounds of review by leaving a comment!
Please let us know when you want some rounds of review by leaving a comment!
Yes, I will. Thanks!
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).
:exclamation: 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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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!
Addressed @dunxen's comments (thanks!), fixed commit compile check, rebased and finally squashed (otherwise commits would not compile on their own).