Skip to content

deps: make syn and serde_json be workspac dependencies, manualy upgrade bitflags from 1.3 to 2.9 #4850

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

eval-exec
Copy link
Collaborator

What problem does this PR solve?

Issue Number: close #4844

Related changes

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

None: Exclude this PR from the release note.

@eval-exec eval-exec requested a review from a team as a code owner March 12, 2025 14:10
@eval-exec eval-exec requested review from zhangsoledad and removed request for a team March 12, 2025 14:10
@driftluo
Copy link
Collaborator

driftluo commented Mar 12, 2025

bitflag's serialization scheme has a breaking change(In my impression, the endianness sorting has changed) from 1.0 to 2.0, which will cause errors in identifying and reading data stored in peer stores. You cannot simply upgrade it. You need to change the default serialization scheme to 1.0 mode, otherwise, it will cause problems with ckb connection and discovery.

All other places use internal data. Only the network needs to interact and store data, and consistency must be guaranteed.

@eval-exec
Copy link
Collaborator Author

eval-exec commented Mar 12, 2025

Serialize, Deserialize has a breakchange: https://github.com/bitflags/bitflags/releases/tag/2.0.0

But the AddrInfo#flags is a u64, not bitflag::Flag

Need to review all actions related to the serialization of CKB and bitflags again.
Or need to reconsider whether upgrading bitflags from 1.0 to 2.0 is truly necessary?

@driftluo
Copy link
Collaborator

Just verify whether the conversion between 1.0 and 2.0 is the same in our usage. I remember it is bitflag to u64, then u64 to bitflag.

@eval-exec
Copy link
Collaborator Author

eval-exec commented Mar 28, 2025

bitflags's Flag value are encoded to ckb-network in bellow three message:
1:

table Identify {
flag: Uint64, // Flag
name: Bytes, // Network Name
client_version: Bytes,
}

2:
table GetNodes2 {
version: Uint32,
count: Uint32,
listen_port: PortOpt,
required_flags: Uint64,
}

3:
table Node2 {
addresses: BytesVec,
flags: Uint64,
}

These flags are Uint64, which is created by flags.bits().pack().
Both bitflags@1 and bitflags@2 using bits method:

    #[doc = r" Returns the raw value of the flags currently stored."]
    #[inline]
    pub const fn bits(&self) -> u64 {
        self.bits
    }

This method return same u64 value for both version, so I beleave upgrade bitflag@1 to bitflag@2 is safe.

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.

3 participants