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.

Sign up for GitHub

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

Jump to bottom

chore!: upgrade bitflags crate #5831

Merged
merged 9 commits into from Oct 11, 2023
Merged

chore!: upgrade bitflags crate #5831

merged 9 commits into from Oct 11, 2023

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Oct 4, 2023

Description

Upgrade the bitflags crate to 2.4.0. This made a few notable changes:

  • serde serialization changed from a map {"bits": 0} to just outputting the compact bit 0. It is possible to maintain the legacy format but we don't need to keep that.
  • bitflags maintain an internal representation which Borsh can't serialize. So in the instance of KernerlFeatures the interface was migrated from the use of the standard bitflags macro to defining the struct ourself, and implementing a bitflags trait. This allows borsh to operate as normal on the unit struct wrapper of the u8 rep.
  • The kernel feature string representation in the faucet file required updating, but the value is still the same once deserialized so it shouldn't require a change in the genesis block, or tx's.
    • It will still be a breaking change as old nodes and new nodes would have a different serialized representation over rpc, and grpc.
  • the bitflags crate no longer derives on the structs it creates. I've manually added only the needed derivations to build, and pass tests.

Motivation and Context

We wanted to upgrade before release.
Closes: #5669

See v2 release changes for bitflags

How Has This Been Tested?

CI

What process can a PR reviewer use to test or verify this change?

Review the changes

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - May not be able to communicate with older nodes.

BREAKING CHANGE: May not be able to communicate with older nodes. Will require users to upgrade their nodes

@brianp
Bump bitflags to 2.4.0
0b2d3f6
@brianp
Add explicit derivations as bitflags no longer does
88e754f
@brianp
Update interface usage
e861e1c
@brianp
Use legacy serde formatters
1a44de6
@brianp
Support borsh serialization
d7e1dfd 
bitflags uses an internal structure that can't be serialized by borsh.
This format of creating the struct and using the bitflags trait is the
recommended format for supporting not supported serializers.
@brianp
Revert "Use legacy serde formatters"
cb42182 
Upgrade to the new format.

This reverts commit 1a44de6.
With a bit extra ammended into it...
@brianp
Update kenrnel format in faucets
fee1c8d
@brianp
use the correct network
c233989
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited

Test Results (CI)

1 219 tests   1 219 ✔️  9m 41s ⏱️
     39 suites         0 💤
       1 files           0 ❌

Results for commit 166b460.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 4, 2023
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited

Test Results (Integration tests)

33 tests  +33   33 ✔️ +33   13m 24s ⏱️ + 13m 24s
11 suites +11     0 💤 ±  0 
  2 files   +  2     0 ❌ ±  0 

Results for commit 166b460. ± Comparison against base commit dd2eddb.

♻️ This comment has been updated with latest results.

@SWvheerden SWvheerden changed the title chore: upgrade bitflags crate chore!: upgrade bitflags crate Oct 5, 2023
SWvheerden
SWvheerden reviewed Oct 6, 2023
View reviewed changes
applications/minotari_console_wallet/Cargo.toml Outdated Show resolved Hide resolved
base_layer/core/Cargo.toml Outdated Show resolved Hide resolved
comms/core/Cargo.toml Outdated Show resolved Hide resolved
comms/dht/Cargo.toml Outdated Show resolved Hide resolved
@brianp @SWvheerden
Apply suggestions from code review
166b460 
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
hansieodendaal
hansieodendaal approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

utACK

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 9, 2023
@SWvheerden SWvheerden merged commit dae7dd9 into tari-project:development Oct 11, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWvheerden SWvheerden SWvheerden approved these changes

@hansieodendaal hansieodendaal hansieodendaal approved these changes

Assignees
No one assigned
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update bitflags to v2
4 participants
@brianp @SWvheerden @hansieodendaal @ghpbot-tari-project