Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix Taptree Strict Encode/Decode #95

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

crisdut
Copy link
Member

@crisdut crisdut commented Oct 31, 2022

Description

I tried to use strict_decode in the PSBT file with output taptree and the method returns Error::DataIntegrityError.

This error occurs because the Taptree decode expected a different format. To simplify the process, I changed to use serialize and deserialize taptree provided by rust-bitcoin lib.

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Nov 15, 2022
@dr-orlovsky dr-orlovsky added this to the v0.9.0 milestone Nov 15, 2022
@dr-orlovsky
Copy link
Member

Can you please provide the context in which you tried to serialize PSBT file? I mean was it a part of a wallet functionality, or something RGB-specific?

I think it was my mistake to start using strict_encoding all around, which results in things like this. My plan is to restrict it just to consensus-level stuff and to remove PSBTs and other structures which already have well-defined encodings. Please see #97 for the details.

I am also happy to merge this PR, but I assume that is a breaking change, requiring major version bump. So please provide where exactly you are using this and depending on that we will decide on the best strategy to fix the issue.

@crisdut
Copy link
Member Author

crisdut commented Nov 15, 2022

Can you please provide the context in which you tried to serialize PSBT file? I mean was it a part of a wallet functionality, or something RGB-specific?

I found this bug when I made regression tests in tapret feature. When I tried to send a PSBT file with taptree in rgb-cli finalize operation, the node returned an error because strict_encode generated a different format than the strict_decode expected.

Without this fix, any PSBT file with taptree fails.

I think it was my mistake to start using strict_encoding all around, which results in things like this. My plan is to restrict it just to consensus-level stuff and to remove PSBTs and other structures which already have well-defined encodings. Please see #97 for the details.

WOW, I will check the proposal.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 3fc665b

@dr-orlovsky
Copy link
Member

Thank you for the explanation, now I got it.

I think we can release that before 0.9 as a bugfix, since the previous version of the code didn't work anyway.

@dr-orlovsky dr-orlovsky merged commit 53d02df into LNP-BP:master Nov 15, 2022
@dr-orlovsky
Copy link
Member

Can you pls also do a PR against 0.8 branch such that we can do a bugfix release?

@crisdut
Copy link
Member Author

crisdut commented Nov 15, 2022

Can you pls also do a PR against 0.8 branch such that we can do a bugfix release?

Sorry, do you refer the rgb-node repo?

@dr-orlovsky
Copy link
Member

No, I refer to this repo, it has v0.8 branch which is a maintenance branch for 0.8.x fixes. Master already contains braking changes since 0.8 release and I can't publish 0.8.x versions from here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants