Skip to content

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