Update to rust-bitcoin v0.30.2 by wpaulino · Pull Request #2740 · lightningdevkit/rust-lightning

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

Fixes#2124.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

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

Additional details and impacted files

@@ Coverage Diff @@## main #2740 +/- ##
==========================================
- Coverage 88.55% 88.50% -0.06% 
==========================================
Files 113 113 Lines 89330 89323 -7 Branches 89330 89323 -7 ==========================================
- Hits 79110 79052 -58 - Misses 7849 7896 +47 - Partials 2371 2375 +4

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

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

 let BlockHeaderData { chainwork, height, header } = data;
 serde_json::json!({
- "chainwork": chainwork.to_string()["0x".len()..],
+ "chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

 "nonce": header.nonce,
- "bits": header.bits.to_hex(),
- "previousblockhash": header.prev_blockhash.to_hex(),
+ "bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

+ match WitnessProgram::new(*version, program.clone()) {
+ Ok(witness_program) => Payload::WitnessProgram(witness_program),
+ Err(_) => return None,
+ }


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

 }
 }
- assert_eq!(base_weight + inputs_total_weight as usize, claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+ assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.
Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

 	fn test_channel_id_v1_from_funding_txid() {
 let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
- assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+ assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

- let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
- let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+ let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+ let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated
Comment threadlightning-transaction-sync/src/esplora.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning-block-sync/src/lib.rs Outdated

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975Compare November 21, 2023 23:28

This was referenced Nov 22, 2023
Bump MSRV to rustc 1.63.0 #2681
Merged
Add electrum support to lightning-transaction-sync#2685
Merged

tnull reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/test_utils.rs Outdated

valentinewallace reviewed Nov 22, 2023
View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023
View reviewed changes

arik-so reviewed Nov 22, 2023
View reviewed changes

Comment threadlightning-block-sync/src/poll.rs
Comment threadlightning/src/chain/channelmonitor.rs Outdated

wpaulino dismissed valentinewallace’s stale review via 6181985November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2
ec928d5

Remove nightly warnings
ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023
View reviewed changes

tnull approved these changes Nov 23, 2023
View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:mainNov 23, 2023

tnull mentioned this pull request Nov 23, 2023
Bump rust-bitcoin to version 0.31 #2745
Closed

Kixunil reviewed Nov 23, 2023
View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

Uh oh!

Conversation

wpaulino commented Nov 21, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 21, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Nov 22, 2023

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

wpaulino commented Nov 21, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 21, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wpaulino commented Nov 21, 2023•
edited
Loading

codecov-commenter commented Nov 21, 2023•
edited
Loading

wpaulino commented Nov 21, 2023•
edited
Loading

codecov-commenter commented Nov 21, 2023•
edited
Loading