Merge rust-bitcoin/rust-bitcoin#6874: p2p: fix ServiceFlags BitXor semantics
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6874: p2p: fix ServiceFlags BitXor semantics
7d883da4013e5f787b30ce6844ecb20f562fa52e p2p: fix ServiceFlags BitXor semantics (wangjingshuiku)
Pull request description:
Fix `ServiceFlags` bitwise XOR implementations to perform actual XOR instead of delegating to `remove`.
`remove` uses `&= !other`, so reusing it for `BitXor` made toggling an unset flag a no-op. For example, `ServiceFlags::NONE ^ ServiceFlags::WITNESS` incorrectly returned `ServiceFlags::NONE`.
Implement `BitXor` and `BitXorAssign` directly using `^`, and add regression coverage for toggling both unset and set service flags.
ACKs for top commit:
apoelstra:
ACK 7d883da4013e5f787b30ce6844ecb20f562fa52e; successfully ran local tests
Tree-SHA512: f90258fc76651b6474609d3c2478b412c460754fbd02c3bee58cc3c09e46065c4be52ac5bd66c593a71f02fead779f75f6e2bb5ca378b0768b0340edb9ab9dbc
What changed, and why it matters
This commit fixes a bug in how the rust-bitcoin library handled the XOR (exclusive-or) operator for Bitcoin network service flags. XOR is supposed to toggle bits on if they are off and off if they are on. The old code incorrectly used a 'remove' operation instead, which only turned bits off. That meant expressions like 'no flags XOR witness' wrongly stayed at 'no flags' instead of becoming 'witness'. The fix implements XOR directly with the proper bitwise operator and adds tests to prevent the bug from returning.
Review any downstream code that uses ServiceFlags with the ^ or ^= operators to confirm it no longer depends on the previous buggy no-op-on-unset behavior. The fix is straightforward and includes tests; ensure it is included in the next release.
Security signals we found
Logic bug in bitwise operator semantics
Incorrect use of remove/clear where XOR/toggle was intended
Potential for unexpected protocol behavior if code relies on BitXor to toggle service flags
Regression tests added for fixed behavior
Evidence from the diff
In p2p/src/lib.rs, the ServiceFlags wrapper around a u64 implemented std::ops::BitXor and BitXorAssign by delegating to ServiceFlags::remove, which performs self.0 &= !other.0. That operation is a bit-clear, not an XOR, so toggling a bit that was unset had no effect. The patch replaces the implementations with direct XOR on the underlying u64 (self.0 ^ rhs.0 and self.0 ^= rhs.0) and adds regression tests covering both unset-to-set and set-to-set toggles.
Changed components
rust-bitcoin p2p/src/lib.rsServiceFlags BitXor implementationServiceFlags BitXorAssign implementationInspect captured patch +7 / −2
### p2p/src/lib.rs
@@ -292,11 +292,11 @@ impl ops::BitOrAssign for ServiceFlags {
impl ops::BitXor for ServiceFlags {
type Output = Self;
- fn bitxor(mut self, rhs: Self) -> Self { self.remove(rhs) }
+ fn bitxor(self, rhs: Self) -> Self { Self(self.0 ^ rhs.0) }
}
impl ops::BitXorAssign for ServiceFlags {
- fn bitxor_assign(&mut self, rhs: Self) { let _ = self.remove(rhs); }
+ fn bitxor_assign(&mut self, rhs: Self) { self.0 ^= rhs.0; }
}
encoding::encoder_newtype_exact! {
@@ -574,6 +574,11 @@ mod tests {
flags |= ServiceFlags::WITNESS;
assert_eq!(flags, ServiceFlags::WITNESS);
+ assert_eq!(ServiceFlags::NONE ^ ServiceFlags::WITNESS, ServiceFlags::WITNESS);
+
+ let mut xor_flags = ServiceFlags::NETWORK;
+ xor_flags ^= ServiceFlags::WITNESS;
+ assert_eq!(xor_flags, ServiceFlags::NETWORK | ServiceFlags::WITNESS);
let mut flags2 = flags | ServiceFlags::GETUTXO;
for f in &all {Why this scored 49/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.