Skip to content

revert: "fix(rpc-json): make DMNState.service and MasternodeStatus.service optional for Core v24" #586

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

Merged
QuantumExplorer merged 1 commit into from
Mar 26, 2026
Merged

revert: "fix(rpc-json): make DMNState.service and MasternodeStatus.service optional for Core v24" #586

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 8 additions & 145 deletions rpc-json/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2056,9 +2056,8 @@ pub struct GetMasternodePaymentsResult {
#[derive(Clone, PartialEq, Eq, Debug, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct DMNState {
#[serde(default)]
#[serde_as(as = "Option<DisplayFromStr>")]
pub service: Option<SocketAddr>,
#[serde_as(as = "DisplayFromStr")]
pub service: SocketAddr,
Comment on lines +2059 to +2060
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for any documentation or comments about Core v24 compatibility
rg -n -i "core.*v?24|v24|service.*optional" --type=md --type=rs

Repository: dashpay/rust-dashcore

Length of output: 91

🏁 Script executed:

#!/bin/bash
# Search for Core v24 references with correct ripgrep syntax
rg -n -i "core.*v?24|v24|service.*optional" --type=markdown --type=rust

Repository: dashpay/rust-dashcore

Length of output: 389

🏁 Script executed:

#!/bin/bash
# Also search without type restrictions to catch comments
rg -n -i "core.*v?24|v24"

Repository: dashpay/rust-dashcore

Length of output: 389

🏁 Script executed:

#!/bin/bash
# Look at the git history to find PR `#523` or related commits
git log --oneline --all | grep -i "optional\|service\|v24\|fix.*rpc" | head -20

Repository: dashpay/rust-dashcore

Length of output: 1388

🏁 Script executed:

#!/bin/bash
# Check the actual file to understand the changes around lines 2059-2060
cd rpc-json/src && head -n 2070 lib.rs | tail -n 30

Repository: dashpay/rust-dashcore

Length of output: 1113

🏁 Script executed:

#!/bin/bash
# Get the full commit message for the revert
git log -1 --format="%B" 6c5d0f22

Repository: dashpay/rust-dashcore

Length of output: 208

🏁 Script executed:

#!/bin/bash
# Check if there's a PR or issue that explains the revert
git log -1 --format="%b" 6c5d0f22 | grep -i "close\|fix\|ref\|#"

Repository: dashpay/rust-dashcore

Length of output: 47

🏁 Script executed:

#!/bin/bash
# Check what the actual PR `#523` commit message said
git log -1 --format="%B" b05bf704

Repository: dashpay/rust-dashcore

Length of output: 1298

🏁 Script executed:

#!/bin/bash
# Check if this is part of a PR that documents the rationale
# Look for any PR or issue references in recent commits
git log --oneline -20 | head -5

Repository: dashpay/rust-dashcore

Length of output: 153

🏁 Script executed:

#!/bin/bash
# Check the broader PR context - look at what files changed in this revert
git show --stat 6c5d0f22 | head -40

Repository: dashpay/rust-dashcore

Length of output: 2335

This revert removes Core v24 compatibility without justification in the commit message.

PR #523 explicitly made service optional to support Dash Core v24, which gates the field behind -deprecatedrpc=service. The commit message documented that Core PR dash#7181 changed the default behavior, requiring this library to handle the field being absent.

This revert makes service required again, breaking support for Core v24 nodes. The revert commit message provides no explanation for this change.

Before merging, provide:

  1. Confirmation that Core v24 compatibility is no longer required
  2. Documentation of why this breaking change is necessary
  3. A proper commit message explaining the rationale (currently missing)

This is a breaking public API change affecting DMNState, DMNStateDiff, and MasternodeStatus.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@rpc-json/src/lib.rs` around lines 2059 - 2060, Revert the breaking change by
restoring service to an optional field: change the type in the affected structs
(DMNState, DMNStateDiff, MasternodeStatus) from SocketAddr to Option<SocketAddr>
and restore serde handling with serde_as to handle Option (e.g., use serde_as(as
= "Option<DisplayFromStr>")); update any code paths that access service to
handle None safely; include in the commit message a clear rationale referencing
Dash Core v24 compatibility and the original PR (`#523` / dash#7181) so the public
API break is documented or explicitly justified if you intend to keep the
change.

pub registered_height: u32,
#[serde(default, rename = "PoSeRevivedHeight", deserialize_with = "deserialize_u32_opt")]
pub pose_revived_height: Option<u32>,
Expand Down Expand Up @@ -2090,7 +2089,7 @@ pub struct DMNState {
#[derive(Clone, PartialEq, Eq, Debug, Deserialize)]
#[serde(try_from = "DMNStateDiffIntermediate")]
pub struct DMNStateDiff {
pub service: Option<Option<SocketAddr>>,
pub service: Option<SocketAddr>,
pub registered_height: Option<u32>,
pub last_paid_height: Option<u32>,
pub consecutive_payments: Option<i32>,
Expand Down Expand Up @@ -2175,7 +2174,7 @@ impl TryFrom<DMNStateDiffIntermediate> for DMNStateDiff {
.transpose()?;

Ok(DMNStateDiff {
service: service.map(Some),
service,
registered_height,
last_paid_height,
consecutive_payments,
Expand Down Expand Up @@ -2317,7 +2316,7 @@ impl DMNState {
self.pub_key_operator = pub_key_operator;
}
if let Some(service) = service {
self.service = service;
self.service = service
}
if let Some(revocation_reason) = revocation_reason {
self.revocation_reason = revocation_reason;
Expand Down Expand Up @@ -2367,9 +2366,8 @@ pub enum MasternodeState {
pub struct MasternodeStatus {
#[serde(default, deserialize_with = "deserialize_outpoint")]
pub outpoint: dashcore::OutPoint,
#[serde(default)]
#[serde_as(as = "Option<DisplayFromStr>")]
pub service: Option<SocketAddr>,
#[serde_as(as = "DisplayFromStr")]
pub service: SocketAddr,
Comment on lines +2369 to +2370
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Same breaking API change as DMNState.service.

This field also becomes required, with the same Core v24 compatibility implications noted above.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@rpc-json/src/lib.rs` around lines 2369 - 2370, The service field was made
required by changing its type/serde handling; revert it to optional to avoid the
breaking API change: change the field declaration to use Option<SocketAddr> and
update the serde_as attribute to #[serde_as(as = "Option<DisplayFromStr>")] (or
add #[serde(default)] if preferred), i.e., update the symbol service to
Option<SocketAddr> and adjust any code that constructs or reads this struct
(parsing/unwrap sites) to handle None safely.

#[serde(rename = "proTxHash")]
pub pro_tx_hash: ProTxHash,
#[serde(rename = "type")]
Expand Down Expand Up @@ -3260,8 +3258,7 @@ mod tests {
use serde_json::json;

use crate::{
DMNState, ExtendedQuorumListResult, MasternodeListDiff, MasternodeStatus, MnSyncStatus,
QuorumType, deserialize_u32_opt,
ExtendedQuorumListResult, MasternodeListDiff, MnSyncStatus, QuorumType, deserialize_u32_opt,
};

#[test]
Expand Down Expand Up @@ -3454,138 +3451,4 @@ mod tests {

println!("{:#?}", result);
}

/// Core v24 regression: `service` is omitted when `-deprecatedrpc=service` is not set.
#[test]
fn deserialize_dmn_state_without_service() {
let json = json!({
"registeredHeight": 850310,
"lastPaidHeight": 0,
"consecutivePayments": 0,
"PoSePenalty": 0,
"PoSeRevivedHeight": -1,
"PoSeBanHeight": -1,
"revocationReason": 0,
"ownerAddress": "yPBWCdMRY5PsS3hJzs7csbdWQVRR85yxUz",
"votingAddress": "ySM11LUD65Bi4p1gm68XLkdWc65TBKRzvQ",
"payoutAddress": "yX4Ve7Q8Y4jscV4LZJD8HVCHKyePzR3MhA",
"pubKeyOperator": "8ed3f0c208efbcfc815cbfb94490dc68cf2e29d44dd9f8a91e20e06057aa110d7062c8ab7ccc85a9ff0c88760157f563"
});

let result: DMNState =
serde_json::from_value(json).expect("should deserialize DMNState without service");
assert_eq!(result.service, None);
assert_eq!(result.registered_height, 850310);
assert_eq!(result.platform_p2p_port, None);
assert_eq!(result.platform_http_port, None);
}

/// Core v24 regression: `service` present should still deserialize correctly.
#[test]
fn deserialize_dmn_state_with_service() {
let json = json!({
"service": "194.135.88.228:6667",
"registeredHeight": 850310,
"lastPaidHeight": 0,
"consecutivePayments": 0,
"PoSePenalty": 0,
"PoSeRevivedHeight": -1,
"PoSeBanHeight": -1,
"revocationReason": 0,
"ownerAddress": "yPBWCdMRY5PsS3hJzs7csbdWQVRR85yxUz",
"votingAddress": "ySM11LUD65Bi4p1gm68XLkdWc65TBKRzvQ",
"payoutAddress": "yX4Ve7Q8Y4jscV4LZJD8HVCHKyePzR3MhA",
"pubKeyOperator": "8ed3f0c208efbcfc815cbfb94490dc68cf2e29d44dd9f8a91e20e06057aa110d7062c8ab7ccc85a9ff0c88760157f563"
});

let result: DMNState =
serde_json::from_value(json).expect("should deserialize DMNState with service");
assert_eq!(result.service, Some("194.135.88.228:6667".parse().unwrap()));
}

/// Core v24 regression: `MasternodeStatus` without `service`.
#[test]
fn deserialize_masternode_status_without_service() {
let json = json!({
"outpoint": "0000000000000000000000000000000000000000000000000000000000000000-0",
"proTxHash": "c560a9be2be9db79e1aaa16e4dd3cd22bddcb0155f88aba68aa4797d375ef370",
"type": "Regular",
"collateralHash": "ff6226e6c97bfcf40b6d04e12e3f75678024988823bfba28cde2a9ac11b1a765",
"collateralIndex": 1,
"dmnState": {
"registeredHeight": 850310,
"lastPaidHeight": 0,
"consecutivePayments": 0,
"PoSePenalty": 0,
"PoSeRevivedHeight": -1,
"PoSeBanHeight": -1,
"revocationReason": 0,
"ownerAddress": "yPBWCdMRY5PsS3hJzs7csbdWQVRR85yxUz",
"votingAddress": "ySM11LUD65Bi4p1gm68XLkdWc65TBKRzvQ",
"payoutAddress": "yX4Ve7Q8Y4jscV4LZJD8HVCHKyePzR3MhA",
"pubKeyOperator": "8ed3f0c208efbcfc815cbfb94490dc68cf2e29d44dd9f8a91e20e06057aa110d7062c8ab7ccc85a9ff0c88760157f563"
},
"state": "READY",
"status": "Ready"
});

let result: MasternodeStatus = serde_json::from_value(json)
.expect("should deserialize MasternodeStatus without service");
assert_eq!(result.service, None);
assert_eq!(result.dmn_state.service, None);
}

/// DMNState diff correctly captures clearing of service field.
#[test]
fn dmn_state_diff_clears_service() {
let with_service = DMNState {
service: Some("194.135.88.228:6667".parse().unwrap()),
registered_height: 850310,
pose_revived_height: None,
pose_ban_height: None,
revocation_reason: 0,
owner_address: [0u8; 20],
voting_address: [0u8; 20],
payout_address: [0u8; 20],
pub_key_operator: vec![0u8; 48],
operator_payout_address: None,
platform_node_id: None,
platform_p2p_port: None,
platform_http_port: None,
};
let without_service = DMNState {
service: None,
..with_service.clone()
};

// Diff from Some → None should capture the change
let diff = with_service
.compare_to_newer_dmn_state(&without_service)
.expect("should detect service removal");
assert_eq!(diff.service, Some(None), "diff should record service cleared");

// Applying the diff should clear the service
let mut state = with_service.clone();
state.apply_diff(diff);
assert_eq!(state.service, None, "service should be None after applying diff");

// Diff from None → Some should also work
let diff = without_service
.compare_to_newer_dmn_state(&with_service)
.expect("should detect service addition");
assert_eq!(
diff.service,
Some(Some("194.135.88.228:6667".parse().unwrap())),
"diff should record service set"
);

// Applying the diff should restore the service
let mut state = without_service;
state.apply_diff(diff);
assert_eq!(
state.service,
Some("194.135.88.228:6667".parse().unwrap()),
"service should be restored after applying diff"
);
}
}
Loading