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

Fix for merging LNPBP4 merkle blocks #115

Merged
merged 6 commits into from Dec 25, 2022
Merged

Fix for merging LNPBP4 merkle blocks #115

merged 6 commits into from Dec 25, 2022

Conversation

dr-orlovsky
Copy link
Member

The problem was caused by a very simple oversight: 387af22

It took me drawing a lot of charts to finally see the obvious mistake.

As far as I can see, this fix is not consensus-breaking, so after getting ACKs on this PR to master, I will backport it into v0.8 and v0.9 as well and will release v0.8.3 version.

Additionally to the fix I made a test in 75fdf50 which matches the merkle blocks which fails to merge from the original bug report RGB-WG/rgb-node#208 (comment) This test fails before the fix is applied - and after the fix it passes successfully.

Next, I improved code style of merkle block merge in 1e11bd0 to ensure that the code is correct and problems like this can't be easily introduced in a future.

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Dec 24, 2022
@dr-orlovsky dr-orlovsky added this to the v0.8.x milestone Dec 24, 2022
@dr-orlovsky
Copy link
Member Author

... and it was not the end of the story!

I decided to test against an expected result of the merge procedure - and the test has failed! See 1f27740

It was caused by an incomplete fix which I have completed in 699a801

@zoedberg
Copy link
Member

@dr-orlovsky thanks for finding the bug! :) Could you please open a PR to apply this fix also to the v0.8 branch? master has some breaking changes which prevents me from testing this

@dr-orlovsky dr-orlovsky modified the milestones: v0.8.x, v0.9.0 Dec 25, 2022
dr-orlovsky added a commit that referenced this pull request Dec 25, 2022
Backport of #115 Fix for merging LNPBP4 merkle blocks
@dr-orlovsky dr-orlovsky changed the base branch from master to v0.9 December 25, 2022 17:37
@dr-orlovsky
Copy link
Member Author

@zoedberg I changed the base for this PR from master to v0.9 and going to merge it there, since we already have it working in v0.8. It would get into master together with the rest of stuff once #114 will get merged.

@dr-orlovsky dr-orlovsky merged commit 480f754 into v0.9 Dec 25, 2022
@dr-orlovsky dr-orlovsky deleted the lnpbp4/merge-fix branch February 3, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants