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 deterministic.rs 'seal changed -was: 0xa7a4 now 0x544d' #438

Closed
wants to merge 1 commit into from

Conversation

hyperji
Copy link

@hyperji hyperji commented Mar 23, 2024

fix issue: #437

reason:
seal changed -was: 0xa7a4 now 0x544d

@D-Stacks D-Stacks requested a review from aspect March 23, 2024 19:23
Copy link
Collaborator

@D-Stacks D-Stacks left a comment

Choose a reason for hiding this comment

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

After a quick talk with @aspect I can confirm that changing the seal hash here is okay, as long as the fields did not change (which is the case).

Copy link
Collaborator

@coderofstuff coderofstuff left a comment

Choose a reason for hiding this comment

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

This allows compiling the entire repo (specifically the wallet part) with the latest rust version (1.77.0 as of this writing) but breaks older version.

kaspad and the rest of the components that do not depend on wallet are semantically the same and are not affected by this PR.

LGTM.

@gvbgduh
Copy link
Contributor

gvbgduh commented Mar 24, 2024

But what is this seal and its purpose and why does it suddenly got broken? Can we really just patch it with a new value?
It sure indicates that some hash of a data structure (?) got changed and there should be a reason it was sealed.

@biryukovmaxim
Copy link
Collaborator

biryukovmaxim commented Mar 24, 2024

This pr is included by 1.77 rustc version
After that seal proc macro gets different tokens. Without unnecessary whitespaces. That's why hash changed.

I suggest to @aspect to make it derive macro only depending on fields and types, he replied that this is general purpose macro and it can be applied to any code. In summary it will work fine until compiler change the logic again.

Imho the proper solution is changing the hash and pinning rust toolchain in the repo. And maybe adding msrv

@gvbgduh
Copy link
Contributor

gvbgduh commented Mar 24, 2024

I see, thanks for clarification

@coderofstuff
Copy link
Collaborator

Thank you for your contribution. As this was integrated into a more fundamental fix in #442 this current PR has been superseded.

@aspect
Copy link
Collaborator

aspect commented Mar 26, 2024

Fixed in #442

I couldn't merge this PR due to other ongoing issues with lints (caused by the new rustc version as well), so the change was applied in the above-mentioned PR that was just merged.

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.

6 participants