Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to bitcoin:0.32 #234

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Conversation

bennyhodl
Copy link
Contributor

All peer dependencies are now able to update to bitcoin 32.

Most changes are renaming of methods and different export paths.

Notable changes:

  • lightning no longer exports std::io::Cursor and uses an emulation through lightning::io::Cursor. PR lightning/src/io/mod.rs
  • New methods to the lightning peer manager peer_connected and peer_disconnected. I believe they don't do anything since there is no specific node feature flagging for DLC's. Which could be an interesting feature to add to assert peers are able to parse DLC mesages.
  • When parsing network related information for writing addresses in the messenger, we can no longer parse the network since it is not public on the address. Since non-mainnet addresses are parsed the same we can only check if the network is mainnet or a testnet flavor. Not sure if we can pass a network here.
  • bitcoin 32 introduces using Amount over u64 in all transaction related structs. I opted to keep this crate as u64 but I think should introduce using Amount over u64 in a future PR.

@bennyhodl
Copy link
Contributor Author

create_cet_adaptor_sig_is_valid is failing. Couldn't track down why.

@bennyhodl bennyhodl changed the title Update to Bitcoin 32 Update to [email protected] Sep 9, 2024
@bennyhodl bennyhodl changed the title Update to [email protected] Update to bitcoin:0.32 Sep 9, 2024
@Tibo-lg
Copy link
Contributor

Tibo-lg commented Sep 10, 2024

That's nice thanks! I couldn't see right away what could be the issue with the unit test that is failing, but it might be good to try to get the integration tests running to see if it's really just that unit test or if there is an actual issue (not sure what is the issue with them, seems something to do with coin selection).

keychain: KeychainKind::External,
is_spent: false,
// @Tibo how do we want to handle this? Store the localoutput in storage?
confirmation_time: ConfirmationTime::unconfirmed(1),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tibo-lg coin selection seems to be the issue for integration tests. Doesn't seem to lead to the unit test though.

Integration tests are very flakey so haven't gotten them to fully pass.

How should we handle the derivation index and conf time? Not sure if it should be included with the Utxo type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters because these fields are probably used by BDK but not in these tests. I tried running the integration tests by replacing BnB with OlderFirst, but it then runs into a signature verification error which might be the same issue highlighted by the failing unit test. I'll try to look a bit more what could be the issue but it's not obvious for now.

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Sep 12, 2024

I think I've fixed all issues. I wanted to push to your branch but was somehow not able not sure why. Can you cherry pick this commit? be327ce
Then have a look to make sure the changes make sense to you. I'll have a last look tomorrow and if everything is good will merge.

@bennyhodl
Copy link
Contributor Author

tACK

rebased and pushed with the changes.

Looks like the issue was the hash type macro?

@bennyhodl
Copy link
Contributor Author

Is this good to merge and release?

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Sep 20, 2024

Is this good to merge and release?

No CI is not passing, it seems there is some PSBT issue. I don't have time to look at it right now, I'll try to next week, but if you solve it before I'm happy to make a release
https://github.com/p2pderivatives/rust-dlc/actions/runs/10863236537/job/30147194064?pr=234#step:7:13

Comment on lines 659 to 664
let transaction = fund_psbt.extract_tx().map_err(|e| {
Error::InvalidState(format!(
"Could not extract transaction from funding psbt. {}",
e
))
})?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix the error.

Suggested change
let transaction = fund_psbt.extract_tx().map_err(|e| {
Error::InvalidState(format!(
"Could not extract transaction from funding psbt. {}",
e
))
})?;
let transaction = fund_psbt.extract_tx_unchecked_fee_rate();

@bennyhodl
Copy link
Contributor Author

Thank you. Pushed the change and all pass.

@Tibo-lg Tibo-lg merged commit 0d16b4b into p2pderivatives:master Oct 7, 2024
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants