Repository navigation
feat(drive-abci)!: resolve Core v24 platform ports at protocol version 14 - #5228
Merged
Merged
Conversation
Core v24 leaves `service`, `platformP2PPort` and `platformHTTPPort` out of its masternode list RPC output unless started with `-deprecatedrpc=service`, and Drive and DAPI read those fields. Render `deprecatedrpc=service` unconditionally in the default section of dash.conf so it applies on every network. Core v23 accepts the option and ignores it. Rollout follows earlier template-only changes to dash.conf (adding and removing `deprecatedrpc=hpmn`): no migration. Every command re-renders the service configs when loading migrates the config format version, so an upgrade from 4.1.x or earlier picks the line up on the first dashmate command. Every 4.2.0 prerelease stamps the format version as 4.2.0 (the newest migration key), so an upgrade from a 4.2.0 prerelease to a later prerelease or to 4.2.0 itself runs no migration and does not re-render. Those nodes need `dashmate config render --config=<name>` and a Core restart before they run Core v24. The new test renders dash.conf for mainnet, testnet, devnet and local and checks the option sits before the first network section. Red to green: 4 failing before the template change, 4 passing after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin rust-dashcore to 538522bc (dashpay/rust-dashcore#1093), whose rpc-json parses the masternode lists Dash Core v24 prints, and adapt drive-abci to it. Only rpc-json differs from the previous pin e4208c90. The re-exported `dpp::dashcore_rpc` (and `dash_sdk::dashcore_rpc`) masternode types change in a breaking way for Rust consumers; nothing nodes agree on changes. DMNState and DMNStateDiff rename the flat platform ports to the deprecated `legacy_platform_p2p_port` / `legacy_platform_http_port`, add the nested `addresses` and the `payouts` list, and make `owner_address`, `payout_address` and `collateral_address` optional, since a shared masternode has none of them. A Tor or I2P `service` reads as `[::]:0` and a `-1` platform port as absent, instead of failing the whole list. Adapt every reader and struct literal in drive-abci, behaviour-identical for every masternode list Core printed before v24 (all three addresses always present): - platform ports are read from the legacy fields exactly as before, with `#[allow(deprecated)]` on the single statement, pattern or field that touches one; `addresses` is not read; - create_owner_identity v0/v1 and transition_to_version_4 need a value from the now optional owner/payout address and fail with DashCoreBadResponseError when it is absent, instead of inventing one; what an added masternode without them gets is decided separately; - the stored MasternodeV0/MasternodeStateV0 format is unchanged: the addresses are stored as before (an absent one as zero bytes, which only a list the node could not process until now can produce) and read back as present; `payouts` and `addresses` are not stored and read back as `None`; - test fixtures wrap the same values in `Some`, drawing the same random numbers in the same order, so seeded strategy tests are unchanged. No behaviour change for pre-v24 lists, so no new test; the drive-abci tests, including the mainnet listdiff fixture test, pass unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dash Core v24 lists masternodes every shipped Drive fails to parse, which
halts the chain: a shared masternode has no owner, payout or collateral
address, and an extended-address masternode has a `payouts` list instead
of `payoutAddress`. Now that rust-dashcore parses them, decide what an
added one gets. Its single payout address is the payout address, else
the only entry of a one-entry payout list (Core turns a payout address
into such a list when a masternode moves to extended addresses), else
none.
- No owner address (shared): no owner identity. Voter and operator
identities as for any masternode.
- Owner address and a single payout address: unchanged, TRANSFER key 0
and OWNER key 1, byte-identical to a legacy payout address.
- Owner address and several payouts: an owner identity with only the
OWNER key 1; key id 0 stays free.
- create_owner_identity v0 (protocol versions 1-3), whose identity holds
only the TRANSFER key: no owner identity without a single payout
address.
- transition_to_version_4 adds no OWNER key for a masternode without an
owner address, after the existing skip of masternodes without owner
identity keys.
- Diffs are unchanged: only `payout_address` rotates the TRANSFER key;
a `payouts` change is parsed but not acted on.
- Stored masternodes keep their format: an absent owner, payout or
collateral address is stored as zero bytes and now reads back as
absent; `payouts` is not stored.
- Drive logs at debug level, with the ProTx hash and the reason, each
added masternode that gets no owner identity or one with only the
OWNER key, and each masternode transition_to_version_4 gives no OWNER
key.
In-place change to shipped generations: create_owner_identity v0
(protocol versions 1-3) and v1 (4-14), update_masternode_identities v0
and perform_events_on_first_block_of_protocol_change v0
(transition_to_version_4, every version). Every input that takes a new
branch is an added masternode without an owner address or without a
single payout address, which only a Core v24 masternode list contains
and every shipped binary rejects at deserialization, so no committed
block was produced from one. Pre-v24 inputs are unchanged: Core prints
an owner and payout address for every earlier masternode and refuses a
null owner key, and a single payout gives the legacy identity byte for
byte. The stored-state conversion is not consensus state; nothing that
decides a block reads a stored payout or collateral address or the
payout list.
Tests, red to green:
- should_update_masternode_identities_from_a_core_v24_masternode_list
runs Core v24 `protx listdiff` JSON (shared, one payout, Tor primary
address with two payouts, Evo with `-1` ports, IPv6 Evo with two
payouts, then a legacy-to-extended-address diff and payout-list-only
diffs) through update_masternode_list with the mock Core RPC. Against
rust-dashcore e4208c90 it fails parsing:
CoreRpc(Json(Error("missing field `payoutAddress`", line: 74, column: 7)))
on the parent commit it fails with
DashCoreBadResponseError("masternode 27978dd8...0bef has no payout address")
and it passes here.
- create_owner_identity: shared (protocol versions 1 and latest), single
payout equals payout address byte for byte (1 and latest), several
payouts give OWNER key 1 only, several payouts on version 1 give no
owner identity. All four fail on the parent commit with
DashCoreBadResponseError and pass here.
- should_reload_absent_addresses_as_absent fails on the parent commit
(Some([0; 20]) read back) and passes here.
- should_add_no_owner_key_for_a_masternode_without_owner_address_on_transition_to_version_4
fails on the parent commit with DashCoreBadResponseError and passes
here.
- should_not_change_owner_identity_keys_on_a_payout_list_change and
should_reload_a_masternode_with_every_address_unchanged pin unchanged
behaviour and pass on both.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Core v24 keeps an ExtAddr evonode's platform ports in its network info.
Its legacy scalar ports stay zero, and a diff lists the legacy
`platformP2PPort` / `platformHTTPPort` fields only when those scalars
change. A ProUpServTx that only moves the ports therefore reaches Drive
as a diff carrying the change only in `addresses`, which Drive ignored:
the validator kept its old port and Tenderdash kept dialling it. A
validator built later from the state got the same stale port, port 0
for an evonode revoked after V24 and revived since, or no validator for
an evonode first listed with `-1` ports.
A port is now the nested entry, else the legacy field as it is, zero
included (`PlatformPorts::of`: rust-dashcore's
`platform_{p2p,http}_address()`, which fall back that way). Keeping a
zero legacy port keeps validator-set membership what earlier binaries
build: they make every evonode with both legacy ports a member, whatever
their value, and Core 23 and 24 list a revoked legacy evonode with
`addresses: {}` beside its flat ports.
Validator-set membership is consensus-relevant: validator_set_update
rotates the quorum when the proposer is the set's last member, banned
members included, and the next quorum's hash is NextValidatorsHash.
`MasternodeStateV0` stores neither `addresses` nor the payout list, so
two invariants keep membership the same on every node:
- After every masternode list update, the masternode lists in memory
equal what a node that restarts loads from storage.
- Validator membership is a function of the committed Core diff
windows: every node asks Core for the same windows, fixed by the
committed core heights, and applies them in the same order.
One consequence: an ExtAddr evonode registered and revoked within one
window arrives as an added item with `-1` ports and is not a member,
while one revoked in a later window stays a banned member with its
stored ports. Both follow from the committed windows, so every node
agrees, and shipped binaries halt on the `-1` added item, so no earlier
behaviour constrains it.
- `insert_masternode` and `apply_masternode_state_diff` keep each state
in stored form (`in_stored_form`: through `MasternodeStateV0`): the
resolved ports move into the legacy port fields, `addresses` and
`payouts` become `None`, and an owner or payout address of zero bytes
reads as absent, exactly as after a reload. Identity updates are
unaffected: they read added masternodes and diffs as Core reported
them. `any_stored_masternode_changes` compares in the same form.
- The stored state keeps the resolved ports, else the legacy value
unchanged, even one outside the port range, as earlier binaries store
it; a state loaded back is then already in stored form.
- A validator built from the masternode state takes the resolved ports.
- The validator refresh on a diff compares the ports resolved before and
after it, reading the updated state rather than the diff, since an
ExtAddr evonode's port move is only in `addresses`. It triggers as
before (ban, service, or a P2P port change) and writes only the ports
that changed, which for lists without nested ports, or with nested
ports mirroring the legacy ones, is exactly what the legacy fields in
the diff set before.
- Drive logs at debug level, with the ProTx hash, an evonode left out
of a validator set because its platform ports do not resolve.
Only the port is taken. The validator host stays the `service` IP: a
legacy evonode's port-only diff pairs its port with the placeholder host
255.255.255.255.
In-place change to shipped generations: update_state_masternode_list v0
and the validator built from masternode state (every protocol version).
Node addresses are not consensus-hashed: Tenderdash's ValidatorSet.Hash
is merkle(threshold public key, quorum hash). Validator-set membership
is unchanged for every input old binaries accept: an evonode is a
member when it has both ports and a platform node id; a resolved port is
present whenever the legacy field is, zero included; and every evonode
old binaries can parse lists both legacy fields (Core before v24 prints
them for every evonode; Core v24 prints them together with `service`,
which old binaries require, and as `-1` for an evonode without network
info, which old binaries reject). The update loop carries this
justification as a comment.
The test that a payment-only diff leaves the state clean now builds its
state with insert_masternode, as Drive holds it: its zero-byte fixture
addresses are not in stored form.
Test would have caught this in CI. With this commit's port reading
reverted to the parent's (legacy fields only, from the state and from
the diff, and no stored-form normalization), these fail and pass here:
- an ExtAddr evonode's port move as an addresses-only diff, with
`service` too, and an HTTPS-only move: the validator keeps 36656 or
443 instead of 36657 or 8443;
- should_move_the_validator_of_an_ext_addr_evonode_on_a_core_v24_port_only_diff
runs Core v24 `protx listdiff` JSON (the new fixture
core_v24_protx_list_diffs/1260-1270.json: `service` and the whole
`addresses`, no legacy ports) through update_masternode_list. With
the parent's source files it fails: the existing member and a new
quorum's validator keep 36656/1443 instead of 36657/8443;
- a state with zero legacy ports gets port 0 instead of 36656, and
without legacy ports no validator;
- the stored state keeps the stale legacy port 36656 instead of 36657;
- should_hold_in_stored_form_exactly_what_the_store_reads_back: the
in-memory state differs from what the store reads back (it also fails
when only the stored-form normalization is removed);
- should_hold_after_every_update_the_masternode_lists_a_restarted_node_loads
stores and reloads the platform state after each of an ExtAddr
evonode's registration, addresses-only port move and revoke (legacy
ports 0, no `addresses`); with only the stored-form normalization
removed, the lists in memory differ from the reloaded ones after the
registration;
- should_not_advertise_an_evonode_whose_network_info_core_emptied pins
the banned member a new quorum gets, with the ports it last had; with
the parent's port reading it gets none;
- should_store_legacy_ports_unchanged_without_nested_ports fails for a
legacy value outside the port range when the stored form drops the
legacy fallback.
Guard tests, passing before and after:
- should_build_the_validators_earlier_binaries_built_from_mainnet_masternode_lists:
every evonode of the mainnet listdiff fixtures, applied the way Drive
applies them, as listed and with `addresses` synthesized as Core 23
prints them (GetNetInfoWithLegacyFields for added items; the core
entry, and changed ports on the service IP or the 255.255.255.255
placeholder, for diffs), gets the validator an earlier binary builds
from the legacy fields alone, for a new quorum and as a refreshed
member of an existing set. It passes with the parent's source files.
It fails when the legacy fallback is dropped (as listed), and when
the stored form skips the legacy fallback once `addresses` is listed
(with Core 23 addresses only).
- should_move_the_validator_of_a_legacy_evonode_on_a_core_v24_port_diff:
a legacy evonode's port diff before V24 (legacy port plus a
255.255.255.255 entry, fixture core_v24_protx_list_diffs/before_v24;
Core accepts a legacy evonode's ProUpServTx only before V24) moves
the validator's P2P port and keeps its IP.
- should_make_an_evonode_with_a_tor_primary_address_a_validator_on_the_unspecified_ip:
an ExtAddr evonode whose primary address is Tor is a validator on
`::` with the ports of its platform entries, and its list processes.
- The placeholder host never becomes the validator host; a
core-address-only diff keeps the platform ports, before and after a
restart; zero legacy ports without nested ones keep the evonode a
member with port 0; a revoked legacy evonode listed with
`addresses: {}` builds the member its legacy ports give; a legacy
evonode revoked after V24 gives, whether or not the node restarted,
exactly the member earlier binaries build (banned, on `::`, ports 0)
and the same stored state; every port test checks that a new quorum,
built from memory or after a reload, gets the same member as the
refreshed validator.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drive built each validator's Tenderdash node address as
`tcp://{node_id}@{node_ip}:{port}`, which leaves an IPv6 host
unbracketed (`tcp://…@2001:db8::1:26656`). A URL authority needs IPv6
hosts in brackets (RFC 3986). Core v24 lets an ExtAddr evonode's primary
address be IPv6.
Format the host and port through `SocketAddr`, so IPv6 is written as
`[2001:db8::1]:26656`; IPv4 addresses are formatted exactly as before.
All four places that build a node address (to_update, to_update_owned,
and both branches of update_difference) now share one helper.
Tenderdash v1.8.1 (the version dashmate runs) already reads both forms
to the same address, because Go's net/url enforces strict colons only
for http(s) URLs, and it writes the bracketed form itself
(net.JoinHostPort). So this changes no Tenderdash behaviour today; it
stops depending on that leniency. Validator node addresses are not part
of Tenderdash's validator-set hash, so the change is not consensus-
affecting.
Test would have caught this in CI: the IPv6 node-address test fails
before the fix (`…@2001:db8::1:30`), passes after; the IPv4 test pins
the unchanged format for every builder. validator_set tests: 12 passed;
clippy -D warnings clean. End to end, from Core v24 `protx listdiff`
JSON through update_masternode_list to the Tenderdash validator update,
the Core v24 fixture test's IPv6 evonode gets
`tcp://…@[2001:db8::4]:26656` and the Tor-primary evonode
`tcp://…@[::]:26656`; before the fix they fail with `…@2001:db8::4:26656`
and `…@:::26656`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds a version 1 masternode-list update path that resolves platform ports from nested addresses. It updates masternode and validator state and related identity state. It adds compatibility and restart tests, updates a Swift identity-read test and vector, and changes Swift migration-test fixture handling. ChangesMasternode List Updates
Identity Read Test Vector
Swift Migration Test Fixture
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant MasternodeUpdate as update_masternode_list_v1
participant StateUpdater as update_state_masternode_list_v1
participant Core
participant PlatformState
participant IdentityState
MasternodeUpdate->>StateUpdater: apply update at Core height
StateUpdater->>Core: fetch masternode diff
Core-->>StateUpdater: return masternode diff
StateUpdater->>PlatformState: update masternodes and validators
StateUpdater-->>MasternodeUpdate: return diff and removed masternodes
MasternodeUpdate->>IdentityState: update identities and remove proposed versions
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR adds versioned nested-address port resolution and updates the Swift migration fixture capture. The reviewed evidence establishes no actionable merge-blocking risk. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects consensus-related validator connectivity, but separates the new behavior by protocol version and preserves the existing storage representation. Restart and revocation checks provide useful safeguards. No introduced security defect was established; production activation order and failure recovery remain important verification areas. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 30, 2026
Collaborator
|
✅ Final review complete — no blockers (commit 14b0eab) · triage: critical |
thepastaclaw
reviewed
Oct 1, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
⚠️ DEGRADED — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6.1-solunavailable: 503 auth_unavailable: no auth available (providers=codex, model=gpt-6.1-sol; last upstream error: usage_limit_reached: T), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor,gpt-6.1-sol→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Port resolution, stored-form normalization, and IPv6 bracketing are correct for all Core-reachable inputs with strong parity coverage. One in-scope nitpick remains: the in-place-edit proof comment overstates port resolution by omitting the u16-range bound. The stored-form hardening ideas are future drift risk with no current production violation.
💬 1 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: platform-versioning); reviewer 4: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: general); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: architecture-layering); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: platform-versioning); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: sol-verifier, role: final-verifier)
- Degraded mode:
gpt-6.1-solunavailable: 503 auth_unavailable: no auth available (providers=codex, model=gpt-6.1-sol; last upstream error: usage_limit_reached: T (detected by probe, since 2026-09-30T21:27:42Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor,gpt-6.1-sol→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6.1-sol) (effort low) — Large masternode port and validator-address change, but node addresses are excluded from consensus hash and membership is preserved, so it does not change a critical surface. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer,glm-5.3-flash— platform-versioning (completed, effort high); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 78% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — final-verifier; agentsol-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — platform-versioning (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [NITPICK] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:179-186: Parity comment overstates port resolution: omits u16-range bound
The in-place-edit proof states 'a port resolves whenever its legacy field is present'. The code does not do exactly that: PlatformPorts::of resolves via rust-dashcore legacy_platform_address(), which drops ports outside u16 range via u16::try_from(port).ok(), while pre-change binaries used `as u16` truncation and kept such an evonode a member. The divergence is unreachable in practice because Core stores ProTx platform ports as uint16_t and its -1 sentinel deserializes to None, so Core can never print >65535, and membership parity holds for every Core-emitted list. The code is therefore correct and no behavior change is needed, but because this comment is the checkable invariant future editors will rely on, it should state the range condition. Same precision applies to the PlatformPorts::of doc at platform_types/validator/v0/mod.rs:70-76.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Stored-form invariant enforced on two accessors, documented on four — Speculative hardening, not a live defect: all current production writes go through the normalizing insert_masternode/apply_masternode_state_diff pair; set_full/hpmn_masternode_list have no production callers outside tests and full/hpmn_masternode_list_mut production uses are test helpers or insert Regular items already in stored form. Normalizing setters is future-proofing, not required for this PR's goal.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Moves the rust-dashcore pin to 40268cc0 (dashpay/rust-dashcore#1093), whose platform address accessors bracket an IPv6 node IP in the legacy port fallback, as nested entries already are. Drive takes only the port from these accessors, so nothing it decides changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…as absent The stored masternode list writes an absent owner, payout or collateral address as zero bytes and reads zero bytes back as absent, so a present address of zero bytes does not round-trip. Core never prints one (it refuses a null owner key for every masternode but a shared one, and nothing reads a stored payout or collateral address), so this is the intended collision; pin it with a test instead of leaving it to prose, and say on the collateral_address accessor that zero bytes mean absent. No behaviour change; the test passes before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Core v24 keeps an ExtAddr evonode's platform ports in its network info.
Its legacy scalar ports stay zero, and a diff lists the legacy
`platformP2PPort` / `platformHTTPPort` fields only when those scalars
change. A ProUpServTx that only moves the ports therefore reaches Drive
as a diff carrying the change only in `addresses`, which Drive ignored:
the validator kept its old port and Tenderdash kept dialling it. A
validator built later from the state got the same stale port, port 0
for an evonode revoked after V24 and revived since, or no validator for
an evonode first listed with `-1` ports.
A port is now the nested entry, else the legacy field as it is, zero
included (`PlatformPorts::of`: rust-dashcore's
`platform_{p2p,http}_address()`, which fall back that way). Keeping a
zero legacy port keeps validator-set membership what earlier binaries
build: they make every evonode with both legacy ports a member, whatever
their value, and Core 23 and 24 list a revoked legacy evonode with
`addresses: {}` beside its flat ports.
Validator-set membership is consensus-relevant: validator_set_update
rotates the quorum when the proposer is the set's last member, banned
members included, and the next quorum's hash is NextValidatorsHash.
`MasternodeStateV0` stores neither `addresses` nor the payout list, so
two invariants keep membership the same on every node:
- After every masternode list update, the masternode lists in memory
equal what a node that restarts loads from storage.
- Validator membership is a function of the committed Core diff
windows: every node asks Core for the same windows, fixed by the
committed core heights, and applies them in the same order.
One consequence: an ExtAddr evonode registered and revoked within one
window arrives as an added item with `-1` ports and is not a member,
while one revoked in a later window stays a banned member with its
stored ports. Both follow from the committed windows, so every node
agrees, and shipped binaries halt on the `-1` added item, so no earlier
behaviour constrains it.
- `insert_masternode` and `apply_masternode_state_diff` keep each state
in stored form (`in_stored_form`: through `MasternodeStateV0`): the
resolved ports move into the legacy port fields, `addresses` and
`payouts` become `None`, and an owner or payout address of zero bytes
reads as absent, exactly as after a reload. Identity updates are
unaffected: they read added masternodes and diffs as Core reported
them. `any_stored_masternode_changes` compares in the same form.
- The stored state keeps the resolved ports, else the legacy value
unchanged, even one outside the port range, as earlier binaries store
it; a state loaded back is then already in stored form.
- A validator built from the masternode state takes the resolved ports.
- The validator refresh on a diff compares the ports resolved before and
after it, reading the updated state rather than the diff, since an
ExtAddr evonode's port move is only in `addresses`. It triggers as
before (ban, service, or a P2P port change) and writes only the ports
that changed, which for lists without nested ports, or with nested
ports mirroring the legacy ones, is exactly what the legacy fields in
the diff set before.
- Drive logs at debug level, with the ProTx hash, an evonode left out
of a validator set because its platform ports do not resolve.
Only the port is taken. The validator host stays the `service` IP: a
legacy evonode's port-only diff pairs its port with the placeholder host
255.255.255.255.
In-place change to shipped generations: update_state_masternode_list v0
and the validator built from masternode state (every protocol version).
Node addresses are not consensus-hashed: Tenderdash's ValidatorSet.Hash
is merkle(threshold public key, quorum hash). Validator-set membership
is unchanged for every input old binaries accept: an evonode is a
member when it has both ports and a platform node id; a resolved port is
present whenever the legacy field is, zero included; and every evonode
old binaries can parse lists both legacy fields (Core before v24 prints
them for every evonode; Core v24 prints them together with `service`,
which old binaries require, and as `-1` for an evonode without network
info, which old binaries reject). The update loop carries this
justification as a comment.
The test that a payment-only diff leaves the state clean now builds its
state with insert_masternode, as Drive holds it: its zero-byte fixture
addresses are not in stored form.
Test would have caught this in CI. With this commit's port reading
reverted to the parent's (legacy fields only, from the state and from
the diff, and no stored-form normalization), these fail and pass here:
- an ExtAddr evonode's port move as an addresses-only diff, with
`service` too, and an HTTPS-only move: the validator keeps 36656 or
443 instead of 36657 or 8443;
- should_move_the_validator_of_an_ext_addr_evonode_on_a_core_v24_port_only_diff
runs Core v24 `protx listdiff` JSON (the new fixture
core_v24_protx_list_diffs/1260-1270.json: `service` and the whole
`addresses`, no legacy ports) through update_masternode_list. With
the parent's source files it fails: the existing member and a new
quorum's validator keep 36656/1443 instead of 36657/8443;
- a state with zero legacy ports gets port 0 instead of 36656, and
without legacy ports no validator;
- the stored state keeps the stale legacy port 36656 instead of 36657;
- should_hold_in_stored_form_exactly_what_the_store_reads_back: the
in-memory state differs from what the store reads back (it also fails
when only the stored-form normalization is removed);
- should_hold_after_every_update_the_masternode_lists_a_restarted_node_loads
stores and reloads the platform state after each of an ExtAddr
evonode's registration, addresses-only port move and revoke (legacy
ports 0, no `addresses`); with only the stored-form normalization
removed, the lists in memory differ from the reloaded ones after the
registration;
- should_not_advertise_an_evonode_whose_network_info_core_emptied pins
the banned member a new quorum gets, with the ports it last had; with
the parent's port reading it gets none;
- should_store_legacy_ports_unchanged_without_nested_ports fails for a
legacy value outside the port range when the stored form drops the
legacy fallback.
Guard tests, passing before and after:
- should_build_the_validators_earlier_binaries_built_from_mainnet_masternode_lists:
every evonode of the mainnet listdiff fixtures, applied the way Drive
applies them, as listed and with `addresses` synthesized as Core 23
prints them (GetNetInfoWithLegacyFields for added items; the core
entry, and changed ports on the service IP or the 255.255.255.255
placeholder, for diffs), gets the validator an earlier binary builds
from the legacy fields alone, for a new quorum and as a refreshed
member of an existing set. It passes with the parent's source files.
It fails when the legacy fallback is dropped (as listed), and when
the stored form skips the legacy fallback once `addresses` is listed
(with Core 23 addresses only).
- should_move_the_validator_of_a_legacy_evonode_on_a_core_v24_port_diff:
a legacy evonode's port diff before V24 (legacy port plus a
255.255.255.255 entry, fixture core_v24_protx_list_diffs/before_v24;
Core accepts a legacy evonode's ProUpServTx only before V24) moves
the validator's P2P port and keeps its IP.
- should_make_an_evonode_with_a_tor_primary_address_a_validator_on_the_unspecified_ip:
an ExtAddr evonode whose primary address is Tor is a validator on
`::` with the ports of its platform entries, and its list processes.
- The placeholder host never becomes the validator host; a
core-address-only diff keeps the platform ports, before and after a
restart; zero legacy ports without nested ones keep the evonode a
member with port 0; a revoked legacy evonode listed with
`addresses: {}` builds the member its legacy ports give; a legacy
evonode revoked after V24 gives, whether or not the node restarted,
exactly the member earlier binaries build (banned, on `::`, ports 0)
and the same stored state; every port test checks that a new quorum,
built from memory or after a reload, gets the same member as the
refreshed validator.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drive built each validator's Tenderdash node address as
`tcp://{node_id}@{node_ip}:{port}`, which leaves an IPv6 host
unbracketed (`tcp://…@2001:db8::1:26656`). A URL authority needs IPv6
hosts in brackets (RFC 3986). Core v24 lets an ExtAddr evonode's primary
address be IPv6.
Format the host and port through `SocketAddr`, so IPv6 is written as
`[2001:db8::1]:26656`; IPv4 addresses are formatted exactly as before.
All four places that build a node address (to_update, to_update_owned,
and both branches of update_difference) now share one helper.
Tenderdash v1.8.1 (the version dashmate runs) already reads both forms
to the same address, because Go's net/url enforces strict colons only
for http(s) URLs, and it writes the bracketed form itself
(net.JoinHostPort). So this changes no Tenderdash behaviour today; it
stops depending on that leniency. Validator node addresses are not part
of Tenderdash's validator-set hash, so the change is not consensus-
affecting.
Test would have caught this in CI: the IPv6 node-address test fails
before the fix (`…@2001:db8::1:30`), passes after; the IPv4 test pins
the unchanged format for every builder. validator_set tests: 12 passed;
clippy -D warnings clean. End to end, from Core v24 `protx listdiff`
JSON through update_masternode_list to the Tenderdash validator update,
the Core v24 fixture test's IPv6 evonode gets
`tcp://…@[2001:db8::4]:26656` and the Tor-primary evonode
`tcp://…@[::]:26656`; before the fix they fail with `…@2001:db8::4:26656`
and `…@:::26656`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 tasks
The rendered-dash.conf test for `deprecatedrpc=service` is not needed; the template change stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…changed
Shipped generations may change in place only where the new code cannot
run under the protocol versions that select them. A node at protocol
version 13 running against Dash Core v24 does receive shared and
extended-address masternodes, so the branches the previous commit added
to them were reachable and would have created state where every
released binary stops.
Restore create_owner_identity v0 (protocol versions 1-3) and v1 (4-13),
update_masternode_identities v0 and transition_to_version_4 to the
mechanical adaptation of the rust-dashcore bump: a masternode without the
payout or owner address they read fails with DashCoreBadResponseError,
the same halt, without state, as a release that cannot parse the list.
The three create_owner_identity and update_masternode_identities files
are byte-identical to that commit again; transition_to_version_4 differs
only in its test. The stored masternode conversion (absent address <->
zero bytes) stays: it is aux storage, not consensus state.
Tests now pin the shipped behaviour:
- create_owner_identity at protocol versions 3 and 13: shared, single
payout list, several payouts (and, at 13, no owner address) fail with
DashCoreBadResponseError; a legacy masternode gets its identity.
- The Core v24 listdiff fixture at protocol version 13 fails with
DashCoreBadResponseError ("... has no payout address").
- transition_to_version_4 gives a masternode with an owner address its
OWNER key after its TRANSFER key, skips a masternode without an owner
identity, and fails on one with an owner identity but no owner
address.
- The payout-list-diff test moves to the update_masternode_identities
dispatcher and runs at protocol version 13 and the latest version.
The tests for the new owner identities move to the new generations in
the next commit.
Red to green: the three new error tests fail on the parent commit
("expected DashCoreBadResponseError, got Ok(None)" / "got Ok(())") and
pass here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lti-payout masternodes at protocol version 14
From v24 on, Dash Core lists shared masternodes, which have no owner,
payout or collateral address, and extended-address masternodes, which
have a `payouts` list instead of `payoutAddress`. create_owner_identity 1
fails on both with DashCoreBadResponseError, which fails the block.
New generations, selected only by protocol version 14
(DRIVE_ABCI_METHOD_VERSIONS_V10, amended in place; only PLATFORM_V14
references it):
- create_owner_identity 2, a copy of 1: no owner address gives no owner
identity; an owner address and a single payout address (payoutAddress,
else the only entry of a one-entry payout list) give the version 1
identity byte for byte (TRANSFER key 0, OWNER key 1); several payouts
give an owner identity with only the OWNER key 1, key id 0 free. The
effective-payout helper lives in v2, its only user.
- update_masternode_identities 1, a copy of 0 that adds no owner identity
when create_owner_identity gives none. Updates are processed as in 0:
only a payoutAddress change rotates the TRANSFER key, and a `payouts`
change is not acted on.
The create_owner_identity dispatcher now returns Option<Identity>.
Versions 0 and 1 are unchanged and wrapped in Some; the only change to
update_masternode_identities 0 unwraps it and treats None as
CorruptedCodeExecution, unreachable because every protocol version that
selects it selects create_owner_identity 0 or 1. transition_to_version_4
is unchanged; it only runs when crossing protocol version 4.
Item 66 of PLATFORM_V14's doc records the change: protocol version 14
has to be active on a network before its Dash Core activates V24, since
earlier versions keep failing on these masternodes.
Tests at the latest version, through the dispatchers:
- create_owner_identity v2: shared gives None; a single payout gives the
payout address's identity, which equals version 1's at protocol
version 13, byte for byte; several payouts give OWNER key 1 only.
- update_masternode_identities v1: a shared masternode gets voter and
operator identities and no owner identity; a change of its voting
address and operator key, and its removal, succeed without an owner
identity and leave its voter and operator keys disabled.
- update_masternode_list with the Core v24 listdiff fixture: processed,
with the identities above and no key change on payout-list diffs
(protocol version 13 keeps failing on it, pinned in the previous
commit).
- The shipped-generation test now also checks the key layout a legacy
masternode gets at protocol versions 3 (TRANSFER 0) and 13 (TRANSFER 0,
OWNER 1), and the payout-list-diff test describes both generations.
Red to green: with create_owner_identity 1 and
update_masternode_identities 0 left in DRIVE_ABCI_METHOD_VERSIONS_V10 the
six new-generation tests fail with DashCoreBadResponseError ("... has no
payout address"); with the table change they pass.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin the historical transition regression and its initial storage to PV4 so the successful OWNER-key insertion checks the protocol that first shipped it. The targeted test passes. Test-only change; no production behavior delta.
Keep the upstream PV14 feature notes and append the Core v24 owner-identity notes after them. Retain the narrow rust-dashcore pin and the PV14-only method selections.
…n 14 Select update_masternode_list v1 only through PV14's existing method table. Resolve nested-first platform ports in that generation before inserting masternodes; keep shipped state update, storage conversion, accessors and validator construction on their historical flat-field behavior. Project the previous masternode state through its persisted form before applying a diff, so transient PV13 addresses cannot distinguish a running node from one restarted before activation. Compare projected states for no-op detection and per-entry writes. Preserve legacy fallback, zero ports and existing-validator refresh triggers, including the HTTPS-only lag. Test would have caught this in CI: ✖ before fix, ✔ after. The dispatcher regression observed PV13 ports 36656/8443 instead of its historical 26656/443 before the generation split; it now verifies additions, nested-only updates and flat-only updates on both sides of the gate. Validation: 59 targeted tests passed, none ignored, including a public proposal/finalize/commit activation with pre/post-update restarts and matching app hashes. Scoped rustfmt and drive-abci clippy with all features/targets and -D warnings passed. Two independent code reviews found no blockers.
shumkov
changed the title
thepastaclaw
reviewed
Oct 4, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static review of the complete diff at cab6b20 found no blocking defects; nested-port resolution is confined to PV14, and the prior parity-documentation finding is fixed. One documentation nitpick remains in the newly added v1 function. git diff --check passed; no builds or tests were run, and the supplied CI snapshot still showed Rust workspace tests and JS builds queued.
💬 1 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate protocol-gated changes in update_state_masternode_list/v1/mod.rs and update_masternode_list/v1/mod.rs alter masternode state and validator-update decisions affecting quorum rotation, including persisted-state normalization needed for consensus agreement across live and restarted nodes. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v1/mod.rs`:
- [NITPICK] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v1/mod.rs:25-27: v1 docstring names parameters the function does not have
The new function's argument documentation lists `state`, but the signature instead has an optional `platform_state` and a mutable `block_platform_state`. Those have distinct roles: the former supplies committed state for identity updates, while the latter receives masternode and validator-set updates. Replace the copied argument list with the actual parameters so readers can distinguish these state inputs. This is documentation-only and does not affect behavior.
Documentation only; no behavior change. Regression tests omitted because only argument documentation changed. Verified cargo fmt and git diff --check.
thepastaclaw
approved these changes
Oct 4, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at fd9f486 found no actionable in-scope defects. Nested-port resolution is selected only by PV14, historical production behavior is preserved, and persisted-state projection addresses live/restarted-node consistency; both prior documentation findings are resolved. No local builds or tests were run; the supplied CI snapshot from 2026-10-04T15:38:38Z still showed Rust workspace tests, platform/browser suites, and several JavaScript checks pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The new update_state_masternode_list/v1 implementation and PV14 dispatch introduce intricate persisted-state normalization and port-update rules that affect validator membership and quorum rotation across activation and restarts. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Collaborator
Author
|
@thepastaclaw final update at @coderabbitai review |
Contributor
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
Contributor
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
thepastaclaw
approved these changes
Oct 5, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff at c233c6b has no actionable in-scope defects: nested-port resolution is confined to PV14, persisted-state normalization preserves restart agreement, and identity processing retains the original Core RPC diff. Both prior documentation findings are fixed, and the IPv6 formatter and test-fixture changes match the stated scope. Verification was static with no local builds or tests; git diff --check passed, and current CI reports Rust workspace tests, Swift build/tests and frozen-schema checks, JavaScript package tests, and Kotlin tests passing, while PR Hygiene remains pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate new update_state_masternode_list_v1 implementation changes protocol-gated persisted masternode state and validator-set updates, making restart determinism and consensus-state consistency part of the diff itself. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Collaborator
Author
|
/self-reviewed |
Contributor
|
Waiting for an active PR slot — merge, close, or convert another active PR by this author to draft so this one can enter human review. |
github-actions
Bot
added
too-many-open-prs
Preserve the PV14 address and BLS signature change notes, both offline Swift identity fetch paths, and the deterministic historical SQLite snapshot. Upstream BLS implementation and the online test rename are retained unchanged. Integration only: no new behavior or regression test introduced. Existing tests and assertions from both branches are retained; independent reviews and Swift parse checks passed.
Contributor
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
approved these changes
Oct 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 14b0eab found no actionable in-scope defects. PV14 alone selects nested-port resolution, historical production behavior is preserved, and persisted-state normalization supports consistent live/restarted behavior; both prior documentation findings are fixed. No local builds or tests were run: the supplied exact-head CI snapshot reports successful Rust, Swift, Kotlin, and JavaScript checks, with full-network suites skipped and PR Hygiene pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate PV14 update_state_masternode_list/v1 implementation changes persisted-state normalization and masternode update detection that determine validator membership and quorum rotation, directly affecting consensus rules. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Collaborator
Author
|
/self-reviewed |
Collaborator
Author
|
/skip-bots |
Contributor
|
Waiting for an active PR slot — merge, close, or convert another active PR by this author to draft so this one can enter human review. |
github-actions
Bot
added
too-many-open-prs
This was referenced Oct 6, 2026
3 of 23 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Oct 13, 2026
Port of #4932 (closed) from v5.0-dev to v5.1-dev, reorganized as a single commit on top of 6499c68. It replaces the original history: the #4932 commits, the port merge, two v5.1-dev merges and the interim Node 20 change, which #5310 later superseded. - Bump the rust-dashcore git dependencies (secp256k1 0.30 -> 0.33, rand 0.9, getrandom 0.4) and migrate call sites mechanically: no secp256k1 context arguments; key, signature and RNG API renames; hex rendering through the `hex` crate. Seeded test keys are unchanged. - Enable getrandom 0.3/0.4 `wasm_js` on wasm32-unknown-unknown in rs-dpp. - Keep the Core v24 masternode handling and persistence from #5227/#5228, replacing the migration's superseded strict legacy conversions. - Preserve the existing cryptography implementations, legacy RPC projection and FFI ABI. SQLite replay, accounting repairs and SwiftData accounting changes remain in the dependent PR #5150. Co-Authored-By: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Basic explanation
What this does: From protocol version 14, Drive resolves evonode platform ports from Core's nested
addresses, with the existing legacy fallback. Earlier protocol versions keep using the flat fields. IPv6 validator dialing addresses are formatted with brackets.Value: After Core V24 activation, an extended-address evonode's port-only service update reaches Tenderdash instead of leaving it at the old port.
Risks: Validator membership affects quorum rotation, so the new port rules are protocol-versioned. Running and restarted nodes must start from identical persisted ports when PV14 activates. Activate PV14 before Core V24. Builds on the merged #5227; this PR now targets
v5.0-devdirectly.Issue being fixed or feature implemented
Core v24 reports extended-address platform port changes only in nested
addresses; its legacy scalar ports do not change. Drive's historical update path misses those changes. Related: #3227; replaces the network-port work in #3936 without changing the persisted masternode schema.What was done?
update_masternode_listv1 and its generation-owned state-update helper. Only PV14 selects v1, through the existing V10 method table.service. Nested placeholder hosts cannot become advertised validator hosts.SocketAddr, producing brackets for IPv6 and unchanged IPv4 text.In-place changes to shipped generations
The shared validator-set formatter changes only the
node_addressdialing string. It does not change stored validator membership, keys, or consensus identity; Tenderdash's validator-set hash uses the threshold public key and quorum hash. The historical masternode-update production code and flat-field constructor are restored unchanged; only frozen tests are pinned to PV13.How Has This Been Tested?
c233c6b7cc: Rust 18,425 passed / 1,411 skipped by existing filters; Swift 789 executed / 16 skipped / 0 failures. JavaScript package tests and the separate Kotlin workflow also passed. Existing changed-package/workflow skips remain; this run did not execute the PR-gated full-network suites.14b0eabf60, basecdcb4be5a2): both PV14 notes and both offline Swift fetch tests are retained; the historical SQLite test uses the deterministic snapshot rather than waiting for a released container. Upstream BLS code is unchanged. Fresh local checks passed: 32 masternode tests, activation/restart strategy, Swift syntax, formatting, diff checks, and two independent resolution reviews. CI is rerunning on this merge commit.Breaking Changes
New port-selection behavior activates at PV14; PV1–13 retain the historical flat-field rule. No stored-schema change.
Known limitations retained:
deprecatedrpc=serviceremain required. Their later removal, including retirement of legacy JS consumers, is tracked in Drive: read evonode platform ports only from addresses and drop the deprecated flat fields at the next protocol version #5254.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
PR Hygiene ·
14b0eabrs-drive-abci— you own itrust-sdk— you own itswift-sdk(packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashModelMigrationTests.swift,packages/swift-sdk/SwiftTests/SwiftDashSDKTests/SDKMethodTests.swift) — llbartekll or romchornyiWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit