Repository navigation
feat(drive-abci)!: apply block randomness through the ApplyBlockRandomness ABCI method - #5753
QuantumExplorer wants to merge 23 commits into
Conversation
…mness ABCI method Tenderdash recovers a block's beacon from its polka's prevote signatures and hands it to the application through the new ApplyBlockRandomness method, before the precommit. Drive draws every random document property and random token amount the block's transitions asked for, and returns the app hash after the draw, which the precommits and the commit sign. - abci: ApplyBlockRandomness handler; FinalizeBlock applies the commit's beacon when it did not see the polka, and checks the commit's app hash and state id against the drawn state. - block execution collects the random requests of the block's transitions and runs apply_random_beacon (document draws, token settlements). - activation at protocol version 15: Tenderdash consensus version 2, sys.random.uniform registration and version 1 token transfers and mints. The first block of protocol version 15 still runs consensus version 1, so it refuses random transitions (RandomBeaconUnavailableError, 10604). - mimic and replay call ApplyBlockRandomness; a strategy test draws random documents and token amounts block after block. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-11-09T11:18:44.305Z |
|
⛔ Final review complete — 2 blocking finding(s) (commit e0fc568) · triage: critical |
…activation # Conflicts: # packages/rs-platform-version/src/version/v15.rs
…activation Takes the random-index prepayment charged in create state validation, and adds an end-to-end regression: a create whose indexed name has the wrong type gets the schema's paid refusal and bumps the nonce. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…activation # Conflicts: # packages/rs-drive/src/drive/document/draw_random_document_properties/v0/mod.rs
…activation # Conflicts: # packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v11.rs # packages/rs-platform-version/src/version/v15.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Three blocking issues remain: deferred draws are not fully prepaid for processing, the ABCI dependency bump breaks the default Tenderdash startup handshake, and historical ABCI 1.4 Info records terminate log replay. Token lifecycle, signed-upgrade, and draw-assertion coverage also need strengthening. Current CI checks pass, but no dedicated Rust consensus-suite result is listed; this verification was static and ran no builds or tests.
🔴 3 blocking | 🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Keep historical Info records replayable after the ABCI version bump
packages/rs-drive-abci/src/replay/runner.rs:157-161
The dependency bump also breaks replay of ABCI 1.4 logs that the previous dependency accepted. log_ingest preserves RequestInfo.abci_version, and this branch forwards the recorded request unchanged to Drive's strict current-version handshake. A recorded abci_version: "1.4.0" now produces AbciVersionMismatch; the error propagates through execute_request and terminates the replay loop before subsequent blocks execute. Publishing a compatible Tenderdash image cannot repair existing log contents, so this needs a separate replay-layer fix. Handle historical transport-version negotiation there while retaining Info's state/root consistency checks, and add a regression that executes a replay containing a 1.4 Info record rather than only parsing it.
source: gpt-6.1-sol (phase2-reviewer: general)
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.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs alter consensus-critical beacon application, final app-hash validation, and random token settlement. - 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 quota lookup failed),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 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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
Activating these calls makes additional processing reachable without an upfront charge for that work. In `add_document_for_contract_operations_v1`, draw prepayment is added only when an index contains a random property; `prepay_drawn_index_entries_v0` likewise returns a default fee when no index is selected. A non-indexed random property therefore incurs no draw-specific prepayment, although drawing still fetches and deserializes the document, hashes its random-property seeds, rebuilds stored-document operations, and applies another GroveDB batch whose costs are discarded. Token reservation operations price the initial reservation and balance setup, while history prepayment prices amount-index entries rather than the subsequent settlement reads and balance/supply mutations; tokens without history bypass even that prepayment. Paying for the placeholder's storage does not pay for a second round of processing. Add deterministic upfront processing prepayment in Drive's owning operation/fee paths, including non-indexed documents and tokens without history, while keeping the post-beacon step free of fee deductions that would change the signed final root.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
The pinned dependency generates ABCI version 1.5.0 from Tenderdash commit `c0e8c7f`, and its `check_version` requires `^1.5`. Drive's Info handler calls that check unconditionally, independently of the Platform protocol version. Dashmate still defaults to `dashpay/tenderdash:1.8`, while the checked-out Tenderdash default reports ABCI 1.4.0; the linked ABCI 1.5 changes remain open upstream. Consequently, the default Drive/Tenderdash pairing fails its startup handshake immediately, including before protocol 15 activates. The documented future release and image update track the dependency but do not preserve the executable default pairing on this head. Coordinate an available compatible engine image and its configuration/migration with this pin, or provide explicitly safe pre-activation compatibility without weakening the protocol-15 requirement.
In `packages/rs-drive-abci/src/replay/runner.rs`:
- [BLOCKING] packages/rs-drive-abci/src/replay/runner.rs:157-161: Keep historical Info records replayable after the ABCI version bump
The dependency bump also breaks replay of ABCI 1.4 logs that the previous dependency accepted. `log_ingest` preserves `RequestInfo.abci_version`, and this branch forwards the recorded request unchanged to Drive's strict current-version handshake. A recorded `abci_version: "1.4.0"` now produces `AbciVersionMismatch`; the error propagates through `execute_request` and terminates the replay loop before subsequent blocks execute. Publishing a compatible Tenderdash image cannot repair existing log contents, so this needs a separate replay-layer fix. Handle historical transport-version negotiation there while retaining Info's state/root consistency checks, and add a regression that executes a replay containing a 1.4 Info record rather than only parsing it.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:84-90: Exercise token settlement through the block lifecycle
This strategy submits only document inserts. The token batch tests discard the processing result containing collected random requests, reconstruct `RandomTokenRequest` themselves, and call Drive settlement directly. They therefore remain green if the new block pipeline stops forwarding accepted token requests or omits settlement, leaving reservations committed instead of paid out. The handler's retry test also uses only a document: rewriting it deterministically can leave the same value and root, so that test does not prove settlement executes exactly once. Add random transfer and mint coverage through prepare/process, ApplyBlockRandomness, and finalize, checking reservation removal, balances, supply, history, proofs, and the committed root. Include an identical Apply retry followed by finalization with the same beacon, plus a finalize-only fallback case; token reservation removal makes accidental repeated settlement observable.
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:126-131: Exercise the protocol upgrade with signed commits
This strategy starts directly at the latest protocol version with signing, commit-signature verification, and platform-state persistence disabled by `default_minimal_verifications()`. The activation-gate tests manually set `switched_from_protocol_version`, so they do not exercise its creation during a real 14-to-15 upgrade and its subsequent clearing across committed blocks. Add an upgrade scenario with signing, commit verification, and persistence enabled: verify that random requests in the upgrade block are refused without charging, the following block draws and commits the final root, and that committed state survives reload. This covers the integration boundary between the protocol-15 consensus-parameter update, the one-block beacon withholding gate, and persisted post-draw state.
In `packages/rs-drive-abci/src/abci/handler/apply_block_randomness.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/abci/handler/apply_block_randomness.rs:496-501: Conditional root assertion makes the draw test pass vacuously
A regression that skips the draw entirely leaves the placeholder rarity at 1. That still passes the range assertion and skips the root-change assertion because it is guarded by `drawn != 1`; the remaining checks only establish that the unchanged root was returned and cached. Require an independently expected drawn value for this fixture, using a beacon whose expected result differs from the placeholder, and assert the stored value and root change unconditionally. The strategy test provides broader coverage, but this handler unit test currently does not establish the behavior its name promises.
| let drawn_documents = self.drive.draw_random_document_properties( | ||
| beacon, | ||
| &random_requests.documents, | ||
| block_info, | ||
| Some(transaction), | ||
| platform_version, | ||
| )?; | ||
|
|
||
| let settled_token_amounts = self.drive.settle_random_token_amounts( | ||
| beacon, | ||
| &random_requests.tokens, | ||
| block_info, | ||
| Some(transaction), | ||
| platform_version, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Prepay the processing work of the newly activated draws
Activating these calls makes additional processing reachable without an upfront charge for that work. In add_document_for_contract_operations_v1, draw prepayment is added only when an index contains a random property; prepay_drawn_index_entries_v0 likewise returns a default fee when no index is selected. A non-indexed random property therefore incurs no draw-specific prepayment, although drawing still fetches and deserializes the document, hashes its random-property seeds, rebuilds stored-document operations, and applies another GroveDB batch whose costs are discarded. Token reservation operations price the initial reservation and balance setup, while history prepayment prices amount-index entries rather than the subsequent settlement reads and balance/supply mutations; tokens without history bypass even that prepayment. Paying for the placeholder's storage does not pay for a second round of processing. Add deterministic upfront processing prepayment in Drive's owning operation/fee paths, including non-indexed documents and tokens without history, while keeping the post-beacon step free of fee deductions that would change the signed final root.
source: gpt-6.1-sol (phase2-reviewer: general)
| tenderdash-abci = { git = "https://github.com/dashpay/rs-tenderdash-abci", rev = "e80c8a7361eda12918d938acd4ca077478419902", features = [ | ||
| "grpc", | ||
| ] } |
There was a problem hiding this comment.
🔴 Blocking: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
The pinned dependency generates ABCI version 1.5.0 from Tenderdash commit c0e8c7f, and its check_version requires ^1.5. Drive's Info handler calls that check unconditionally, independently of the Platform protocol version. Dashmate still defaults to dashpay/tenderdash:1.8, while the checked-out Tenderdash default reports ABCI 1.4.0; the linked ABCI 1.5 changes remain open upstream. Consequently, the default Drive/Tenderdash pairing fails its startup handshake immediately, including before protocol 15 activates. The documented future release and image update track the dependency but do not preserve the executable default pairing on this head. Coordinate an available compatible engine image and its configuration/migration with this pin, or provide explicitly safe pre-activation compatibility without weakening the protocol-15 requirement.
source: gpt-6.1-sol (phase2-reviewer: general)
| operations: vec![Operation { | ||
| op_type: OperationType::Document(document_op), | ||
| frequency: Frequency { | ||
| times_per_block_range: 1..3, | ||
| chance_per_block: None, | ||
| }, | ||
| }], |
There was a problem hiding this comment.
✅ Resolved at
5d0f0ad7; see the replies below.
🟡 Suggestion: Exercise token settlement through the block lifecycle
This strategy submits only document inserts. The token batch tests discard the processing result containing collected random requests, reconstruct RandomTokenRequest themselves, and call Drive settlement directly. They therefore remain green if the new block pipeline stops forwarding accepted token requests or omits settlement, leaving reservations committed instead of paid out. The handler's retry test also uses only a document: rewriting it deterministically can leave the same value and root, so that test does not prove settlement executes exactly once. Add random transfer and mint coverage through prepare/process, ApplyBlockRandomness, and finalize, checking reservation removal, balances, supply, history, proofs, and the committed root. Include an identical Apply retry followed by finalization with the same beacon, plus a finalize-only fallback case; token reservation removal makes accidental repeated settlement observable.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
There was a problem hiding this comment.
Added in 5d0f0ad. New strategy test should_settle_every_random_token_amount_from_its_block_beacon sends random transfers and mints for 12 blocks through prepare, process, ApplyBlockRandomness, commit and finalize. It verifies each history proof against the committed root, with the settled amount inside its range. It also checks that balances and supply match the drawn amounts and that no reservation is left. The handler retry test now settles a random transfer too, so a second settlement would fail on the removed reservation. The finalize-only fallback test settles one from the commit's beacon.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Resolved (re-reviewed at 5d0f0ad7): Your 12-block token strategy exercises accepted transfers and mints through finalization and verifies history results, balances, supply, and reservation removal. The token-backed retry and commit-beacon fallback fixtures also make duplicate or missing settlement observable.
| testing_configs: PlatformTestConfig::default_minimal_verifications(), | ||
| ..Default::default() | ||
| }; | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .with_config(config.clone()) | ||
| .with_latest_protocol_version() |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the protocol upgrade with signed commits
This strategy starts directly at the latest protocol version with signing, commit-signature verification, and platform-state persistence disabled by default_minimal_verifications(). The activation-gate tests manually set switched_from_protocol_version, so they do not exercise its creation during a real 14-to-15 upgrade and its subsequent clearing across committed blocks. Add an upgrade scenario with signing, commit verification, and persistence enabled: verify that random requests in the upgrade block are refused without charging, the following block draws and commits the final root, and that committed state survives reload. This covers the integration boundary between the protocol-15 consensus-parameter update, the one-block beacon withholding gate, and persisted post-draw state.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
| assert_eq!(response.app_hash, root_after.to_vec()); | ||
| let drawn = rarity_in_block(&platform, &app, &contract); | ||
| assert!((1..=100).contains(&drawn)); | ||
| if drawn != 1 { | ||
| assert_ne!(root_before, root_after, "the drawn value moved the root"); | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
5d0f0ad7; see the replies below.
🟡 Suggestion: Conditional root assertion makes the draw test pass vacuously
A regression that skips the draw entirely leaves the placeholder rarity at 1. That still passes the range assertion and skips the root-change assertion because it is guarded by drawn != 1; the remaining checks only establish that the unchanged root was returned and cached. Require an independently expected drawn value for this fixture, using a beacon whose expected result differs from the placeholder, and assert the stored value and root change unconditionally. The strategy test provides broader coverage, but this handler unit test currently does not establish the behavior its name promises.
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 5d0f0ad: the test pins the rarity the fixture beacon draws (26, not the placeholder 1) and checks the root moved, unconditionally.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Resolved (re-reviewed at 5d0f0ad7): Your change pins the expected rarity to 26 and checks the root change unconditionally. A skipped draw now leaves placeholder 1 and fails the test.
…ycle - Replay stands in for Tenderdash, so it sends Info with this build's ABCI version: a log recorded under ABCI 1.4 replays again. - The ApplyBlockRandomness draw test pins the drawn rarity and checks the root moved, unconditionally. The retry test also settles a random transfer, whose removed reservation would fail a second settlement. - The finalize-only fallback test settles a random transfer from the commit's beacon: the recipient holds the drawn amount, once. - New strategy test: random transfers and mints over many blocks through prepare, process, ApplyBlockRandomness, commit and finalize, with proofs of the settled history and balances, supply and escrow checked against the drawn amounts. The strategy gains random_token_minimum_amount, and its proof check accepts a settled amount within the transition's range. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 5d0f0ad and the relevant upstream sources. The draw assertion, token lifecycle coverage, and historical Info replay issues are fixed; incomplete processing prepayment and the default Tenderdash runtime mismatch remain blocking, with signed upgrade coverage retained as a suggestion. Current reported PR checks pass; verification was static, with no local builds or tests run.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 9: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs alter consensus-critical beacon application, final app-hash validation, and random token settlement across the block lifecycle. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate deferred work whose processing costs are discarded rather than fully prepaid. In Drive, `prepay_drawn_index_entries_v0` estimates selected index insertions, and document insertion invokes it only when an index contains a random property. The `looseCard` regression test explicitly expects zero prepayment for an unindexed random property, although drawing it still fetches the document, deserializes it, hashes the beacon-derived seed, and rewrites the primary record. Token reservation operations likewise charge their initial balance/reservation writes, while settlement subsequently reads and removes the reservation and updates balances or supply; the additional history prepayment covers selected index entries, not this whole settlement. Both deferred paths discard their GroveDB cost accumulators, and `draw_from_beacon_v0` adds no hash-cost operation. The initial storage payment therefore does not establish payment for the additional processing. Add a protocol-15 processing prepayment for the complete bounded deferred work in Drive's originating insertion/reservation paths, shared by estimation and application and using the versioned fee schedule. Cover unindexed random documents and history-disabled tokens, and keep settlement itself unbilled after block fee accounting.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This dependency change immediately makes the configured node pair incompatible, independently of protocol-15 activation. The pinned Rust binding generates its interface from Tenderdash commit `c0e8c7f451414e8ec6c319ead45a9c76e3f56c9a`, which advertises ABCI 1.5.0. Its compatibility matcher explicitly rejects ABCI 1.4.0, and Drive's live `info` handler invokes that matcher before consulting platform state. Meanwhile, Dashmate still defaults to `dashpay/tenderdash:1.8`; the inspected Tenderdash 1.8 source advertises ABCI 1.4.0, also documented by the binding's compatibility tests. Thus the default stack cannot complete its startup handshake even on a pre-15 chain. The release-dependency section and linked upstream PRs correctly identify the prerequisite, but they do not resolve the startup regression introduced by changing the pin now. Coordinate a compatible Tenderdash image and its default/migration configuration with this dependency change, or hold the integration until the matched runtime is available; weakening the live handshake is not a substitute.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version, leave `upgrading_info` unset, and use `default_minimal_verifications()`. That configuration disables block signing, commit-signature verification, and platform-state persistence. The tests now establish drawing, settlement, and proof verification, but do not exercise the actual 14-to-15 handoff: the switching block still commits under Tenderdash consensus version 1, while the next block must authenticate the post-draw root under version 2. The gate tests manually set `switched_from_protocol_version`, so they cannot catch a regression in the normal upgrade path setting or clearing that marker. Add a deterministic strategy starting at protocol 14 and upgrading normally with signing and commit verification enabled. Assert the consensus-parameter update, unpaid refusal of random requests in the switching block, and successful drawing and finalization over the signed final root in the following block.
…activation # Conflicts: # packages/rs-platform-version/src/version/v15.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined findings and all six prior findings against head 2a41a85. Two blockers remain: incomplete processing-fee prepayment for deferred draws and an incompatible default Tenderdash/ABCI pairing; signed protocol-upgrade coverage also remains missing. All currently listed GitHub checks pass; this verification was static, with no local builds or tests run.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in rs-drive-abci's apply_block_randomness.rs and finalize_block.rs alter consensus-critical beacon application, final app-hash validation, and random token settlement. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate a second processing pass that the creating transitions do not fully prepay. Document insertion requests prepayment only when an index contains a random property, and `prepay_drawn_index_entries_v0` returns `FeeResult::default()` when no affected index exists. Nevertheless, an unindexed random document is fetched, deserialized, hashed per random property, serialized, and rewritten; the draw discards its operation costs through temporary accumulators. Token reservation execution likewise pays for the initial reservation and balance writes, while settlement performs additional escrow reads/removal and balance or supply updates whose costs are discarded. Its history-index prepayment is separate and is absent when history is disabled. Fixed serialized widths avoid additional persistent allocation, but do not account for this additional processing. This PR makes those previously dormant paths reachable at PV15. Add deterministic upfront processing prepayment in the owning Drive creation/reservation paths, including hashing, unindexed documents, and tokens without history, using versioned fee machinery and preserving earlier protocol behavior. Cover both fee estimation and applied execution; a request cap alone would not account for the omitted processing.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
The new binding pin is incompatible with the repository's default Tenderdash runtime. Its `proto/build.rs` selects Tenderdash commit `c0e8c7f`, whose `version/version.go` declares ABCI 1.5.0, and the pinned Rust `check_version` rejects 1.4.0 against that requirement. However, Dashmate still selects `dashpay/tenderdash:1.8` in `getBaseConfigFactory.js`, and the inspected Tenderdash 1.8 source advertises ABCI 1.4.0 through `RequestInfo`. Drive's `info` handler performs this compatibility check unconditionally, so the default pairing fails during startup even before PV15 activates. The linked Tenderdash #1582/#1583/#1584 and rs-tenderdash-abci #226 track the required upstream work, but documenting a later image update does not resolve this newly introduced startup regression. Coordinate a compatible runtime image and its configuration migration with this binding change; an unreleased revision pin is not itself the blocker, the incompatible default pairing is.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version, set `upgrading_info: None`, and use `default_minimal_verifications()`. That configuration disables block signing, commit-signature verification, and platform-state persistence. The gate test manually sets `switched_from_protocol_version`, and the finalize fallback fixture also disables commit-signature verification. Consequently, the existing coverage establishes steady-state settlement and root consistency, but not the actual PV14-to-PV15 handoff: the activation block runs under Tenderdash consensus version 1 without a beacon, and the following block uses version 2 with a commit authenticating the post-draw state. Add a multi-block upgrade strategy with signing and commit verification enabled, checking unpaid refusal in the activation block, successful state-changing draws afterward, and the committed root after reopening the platform. Beacon authentication should remain owned by Tenderdash rather than being duplicated in application code.
…activation v5.1-dev took v15 note 20 for version 2 asset unlocks: random document properties are note 21, random token amounts 22 and same-block randomness 23. DRIVE_ABCI_METHOD_VERSIONS_V11 keeps both the asset unlock withdrawal version and apply_random_beacon. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…activation v5.1-dev took v15 note 21 for contests keyed by top-level properties: random document properties are note 22, random token amounts 23 and same-block randomness 24. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified two blocking issues at 92a5bc0: incomplete draw-processing prepayment and an ABCI handshake mismatch with the configured Tenderdash runtime; signed-upgrade coverage and Rust API migration notes also remain missing. Three prior findings are fixed. This was static validation only: current CI reports the macOS generation check as failed, but its job metadata records cancellation with no available log, and the checks do not establish results for the targeted Rust suites.
🔴 2 blocking | 🟡 2 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs alter consensus-critical beacon application, final app-hash validation, and settlement of random token transfers and mints. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs:169-174: Document the unconditional Rust API changes in the unreleased changelog
The protocol-15 gate does not preserve Rust source compatibility: adding this public field changes exhaustive `DriveAbciBlockEndMethodVersions` literals regardless of the active protocol version. The PR also adds public context/commit fields, required methods to the platform-state and block-context traits, and `BasicError::RandomBeaconUnavailableError`. The unreleased changelog documents the stacked random-document and random-token API migrations but contains no migration note for these additions. Add a source-compatibility entry naming the new fields, required trait methods, and exhaustive-match arm so downstream Rust consumers can update independently of the consensus activation.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
Activating these calls makes additional processing mandatory without fully charging for it. Document insertion adds prepayment only when a random property participates in an index, and `prepay_drawn_index_entries_v0` returns a default fee when no index is selected. An unindexed random document therefore prepays no draw-specific processing, although drawing fetches and deserializes the document, hashes each requested property, and rewrites the primary record. Token reservation charges its initial operations, while the optional history prepayment estimates amount-index insertions rather than the subsequent escrow read/removal and balance or supply updates. Both settlement paths discard the later operation costs. Allocating the placeholder's storage does not pay for this second processing pass. Add deterministic processing prepayment in Drive's owning insertion/reservation paths, consistently for estimation and execution, including unindexed documents and tokens without history. Use the unreleased version's named fee schedule where new rates are needed, preserve historical schedules, and keep post-beacon settlement free of retrospective fee deductions.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This changes the binding from release v1.8.1 to e80c8a7, whose protobuf build selects Tenderdash c0e8c7f and ABCI 1.5.0. Its `check_version` rejects ABCI 1.4, and Drive's live `info` handler invokes that check before considering the active Platform protocol version. Meanwhile, Dashmate still selects `dashpay/tenderdash:1.8` in `getBaseConfigFactory.js:382`; the checked-out Tenderdash v1.8 default advertises ABCI 1.4.0, and the linked compatible upstream PRs remain open. The configured pairing therefore cannot complete its startup handshake, including on a pre-15 chain. This is a startup regression introduced by the dependency change, not merely a missing release tag. Coordinate the pin with an available compatible runtime and the corresponding Dashmate configuration/migration, or otherwise prevent the incompatible pairing from becoming the runnable default. The linked release prerequisites document the dependency but do not resolve the current mismatch.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version, set `upgrading_info: None`, and use `default_minimal_verifications()`, which disables block signing, commit-signature verification, and platform-state persistence. The activation tests manually assign `switched_from_protocol_version`, and the finalize-beacon tests also disable commit-signature verification. These tests cover individual decisions and settlement, but not the production sequence where the real 14-to-15 switch returns consensus version 2, the switching block receives no beacon, and the following block commits a signed post-draw root. `verify_state_transition_results` checks transition proofs; it does not re-enable commit authentication. Add a lifecycle strategy crossing that boundary with signing, commit verification, and persistence enabled. Assert unpaid refusal in the switching block, a root-changing draw in the following block, successful verification of the commit over that root, and agreement after reload and the next proposal.
| /// Fills a block's `sys.random` document properties and settles its random token amounts | ||
| /// from the block's random beacon, which Tenderdash hands over in `ApplyBlockRandomness` | ||
| /// once the block's prevotes combine into it. `None` before protocol version 15, whose | ||
| /// Tenderdash consensus version 2 is the first to send a beacon; a block then collects no | ||
| /// random requests and applies none. | ||
| pub apply_random_beacon: OptionalFeatureVersion, |
There was a problem hiding this comment.
✅ Resolved at
dc261cfc; see the replies below.
🟡 Suggestion: Document the unconditional Rust API changes in the unreleased changelog
The protocol-15 gate does not preserve Rust source compatibility: adding this public field changes exhaustive DriveAbciBlockEndMethodVersions literals regardless of the active protocol version. The PR also adds public context/commit fields, required methods to the platform-state and block-context traits, and BasicError::RandomBeaconUnavailableError. The unreleased changelog documents the stacked random-document and random-token API migrations but contains no migration note for these additions. Add a source-compatibility entry naming the new fields, required trait methods, and exhaustive-match arm so downstream Rust consumers can update independently of the consensus activation.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor, tenderdash-consensus-security)
There was a problem hiding this comment.
Added an Unreleased "platform-version, dpp, drive-abci (Rust API)" entry in dc261cf naming apply_random_beacon, RandomBeaconUnavailableError, the new PlatformState, BlockExecutionContextV0 and CleanedCommitInfo fields, and the new required trait methods.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Resolved (re-reviewed at dc261cfc): Your new Unreleased Rust API entry names the added public fields, required trait methods, and RandomBeaconUnavailableError arm, and explains how downstream consumers initialize them. This addresses the source-compatibility documentation independently of PV15 activation.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified dc261cf against the supplied findings and upstream sources. Two blockers remain: incomplete processing prepayment for the activated randomness phase and an incompatible default Tenderdash runtime; signed upgrade coverage also remains missing, while four prior findings are fixed. All currently reported CI checks pass; this was a static verification with no local builds or tests.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 13: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in rs-drive-abci's apply_block_randomness.rs and finalize_block.rs alter consensus-critical block execution, beacon-driven token settlement, and validation of the final app hash signed by precommits. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (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— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— tenderdash-consensus-security (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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate deferred processing that the originating transitions do not fully prepay. Document insertion adds draw prepayment only when a random property participates in an index, and `prepay_drawn_index_entries_v0` returns zero when no index is selected. Nevertheless, an unindexed random document is fetched, deserialized, hashed per random property, and rewritten. The draw discards its read/write cost accumulators. Token settlement likewise reads and removes escrow and updates balances or supply, while the extra prepayment covers selected history-index insertions and is absent when history is disabled. Paying for the original placeholder or reservation's storage does not pay the processing cost of these subsequent operations. This PR makes the previously inactive work reachable in normal consensus execution, allowing valid client requests to impose undercharged work on every validator. Add deterministic processing prepayment in Drive's originating create/reservation paths, using the unreleased version and fee tables where needed. Include it in estimation and execution, and cover unindexed random documents and tokens without history while keeping the beacon phase free of retrospective deductions.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This dependency pin requires ABCI 1.5 immediately, including when Drive runs an older Platform protocol version. The live `info` handler unconditionally calls the pinned library's `check_version`, whose `^1.5` requirement rejects ABCI 1.4. Dashmate still defaults to `dashpay/tenderdash:1.8`; the latest released Tenderdash v1.8.2 and the checked-out v1.8-dev source both advertise ABCI 1.4.0. The added replay regression also explicitly asserts that a live 1.4 handshake is rejected. Thus a node combining this Drive binary with the configured runtime cannot complete startup. The linked upstream PRs track the required release, but they do not make the current pairing compatible, and the failure precedes the PV15 activation gate. Coordinate the dependency change with an available compatible runtime and the necessary default-image/configuration migration. Do not simply relax the handshake to accept an engine that lacks the new lifecycle.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version, configure `upgrading_info: None`, and use `default_minimal_verifications()`, which disables block signing, commit-signature verification, and platform-state persistence. The activation-gate fixtures manually set `switched_from_protocol_version`; the existing PV14-to-latest evidence-policy strategy also disables commit verification and submits no random operations. Consequently, the tests do not exercise the combined boundary introduced here: a real upgrade block without a beacon, followed by a randomness-enabled block whose commit authenticates the post-draw root. Add a lifecycle scenario crossing PV14 to PV15 with signing and commit verification enabled. Check unpaid refusal of random requests in the switching block, acceptance and settlement in the next block, and agreement between the signed state ID and committed final root. Include reload or replay coverage for the transient activation marker and persisted root.
…activation v5.1-dev took PV15 note 22 (update_voter_identity 1, #5768), so random document properties are note 23, random token amounts note 24 and same-block randomness note 25. DRIVE_ABCI_METHOD_VERSIONS_V11 documents both update_voter_identity 1 and apply_random_beacon Some(0). Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification confirms two blockers: the new ABCI dependency rejects the currently configured Tenderdash runtime, and the activated draw/settlement pass performs processing that is not fully prepaid. Signed protocol-upgrade coverage also remains incomplete; the API documentation, draw assertion, token lifecycle coverage, and historical Info replay fixes are present. All currently listed CI checks pass; no local builds or tests were run.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes consensus-critical execution in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs, applying beacon-driven document draws and token settlements and validating the resulting app hash across voting, replay, and activation paths. - 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 quota lookup failed),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 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— tenderdash-consensus-security (completed, effort xhigh); 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/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This dependency change breaks startup with the default runtime pairing, independently of protocol-15 activation. The pinned library generates its protos from Tenderdash commit c0e8c7f, whose version/version.go declares ABCI 1.5.0. Its check_version rejects ABCI 1.4.0, and Drive's info handler returns AbciVersionMismatch before reading the active platform version. Meanwhile, packages/dashmate/configs/defaults/getBaseConfigFactory.js still selects dashpay/tenderdash:1.8, and the checked-out Tenderdash v1.8 development tip declares ABCI 1.4.0. The release-dependency paragraph and linked upstream PRs track the necessary work, but postponing the runtime update leaves this head's default node configuration unable to complete the handshake. Coordinate a runtime containing the required ABCI and consensus changes with this dependency update, or preserve a working pre-activation pairing while refusing randomness on unsupported runtimes. The problem is the incompatible pairing, not the use of an immutable commit pin itself.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate an additional execution pass whose processing is not fully covered by the creating transition. Document insertion adds prepayment only when a random property participates in an index, and prepay_drawn_index_entries_v0 returns a default FeeResult when no index qualifies. An unindexed random document nevertheless gets fetched, deserialized, hashed per property, serialized, and rewritten by draw_random_document_v0, with operation costs discarded through temporary accumulators. Token reservation creation similarly prices the initial writes, while settlement additionally reads and removes escrow, credits the recipient, refunds the sender or adjusts supply, and optionally rewrites history. Its history-index prepayment does not cover that remaining work, including for tokens without history. Paying for equal-sized placeholder storage does not pay processing for another execution. This PR makes those paths mandatory at PV15, so the gap is in scope even though the underlying Drive methods came from the stacked changes. Prepay the remaining processing in the owning Drive create/reservation paths, consistently during estimation and application and using the versioned fee model. Add accounting coverage for unindexed random documents and history-disabled tokens; the beacon hook can remain unbilled after this work is actually prepaid.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-151: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version, set upgrading_info to None, and use default_minimal_verifications(), which disables block signing, commit-signature verification, and platform-state persistence. The activation-gate tests manually set switched_from_protocol_version, while the existing PV14-to-latest upgrade/restart strategy also disables commit verification and submits no random operations. These tests therefore do not exercise the connection between the consensus-parameter switch, the activation block receiving no beacon, and the following block's commit authenticating the post-draw root. Add a focused 14-to-15 lifecycle test with signing and commit verification enabled: check unpaid refusal with error 10604 in the switching block, successful random document/token settlement in the following block, and acceptance of the next proposal against the committed final root. Enabling persistence and reopening at that boundary would also cover restoration of the final-root state.
…activation v5.1-dev took PV15 note 23 (IdentityCreate identity id check, #5770), so random document properties are note 24, random token amounts note 25 and same-block randomness note 26. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 3f1f338: deferred draw processing remains undercharged, and the ABCI dependency bump is incompatible with the configured Tenderdash runtime; signed protocol-upgrade coverage also remains missing. The changelog, deterministic draw assertion, token lifecycle coverage, and historical Info replay findings are fixed. This was a static review with no builds or tests run; current PR checks pass, but the listed checks do not establish execution of the Rust consensus suites.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate change directly modifies consensus execution in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs, deriving the committed app hash after random document draws and token settlement and changing protocol-version activation behavior. - 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 quota lookup failed),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 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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
Protocol 15 activates this second execution pass, but the originating transitions do not prepay all of its processing. Document insertion adds the existing prepayment only for indexes containing random properties, and `prepay_drawn_index_entries_v0` returns `FeeResult::default()` when none is selected. An unindexed random document nevertheless requires a document read/deserialization, per-property beacon hashing, and a primary-entry rewrite. Token reservation lowering similarly pays for the initial reservation and balance writes, while the extra history prepayment covers selected history index entries—not the subsequent reservation lookup/removal and balance/supply mutations. Both settlement implementations discard their read/write cost accumulators, and `draw_from_beacon_v0` hashes without a corresponding function-cost operation. Paying for the placeholder's retained storage does not pay for this additional processing. Add deterministic upfront processing prepayment in Drive's create/reservation lowering paths used by estimation and execution, preserving earlier fee schedules and using the protocol-15 named schedule where rates are needed. Cover unindexed random documents and random tokens without history; keep this post-prevote hook prepaid rather than deducting fees after block fee accounting.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
The pin changes the live handshake immediately, not only when protocol 15 activates. The pinned bindings require ABCI 1.5 and explicitly test rejection of ABCI 1.4; Drive's `info` handler propagates that mismatch. Dashmate still selects `dashpay/tenderdash:1.8` in `configs/defaults/getBaseConfigFactory.js:382`, and the checked-out Tenderdash default advertises ABCI 1.4.0. The base used bindings tagged `v1.8.1`, so this PR introduces a startup failure for the repository's configured runtime even at protocol 14 or earlier. The linked Tenderdash and Rust-bindings PRs track the required capability, but the documented future image update does not resolve this newly introduced incompatibility. Coordinate a compatible Tenderdash runtime, its Dashmate default and existing-config migration with this dependency change before integrating it; retain the strict handshake rather than accepting an engine that lacks the required lifecycle.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-151: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start directly at the latest protocol version with `upgrading_info: None` and `default_minimal_verifications()`. That configuration disables block signing, commit-signature verification, and platform-state persistence. The gate unit tests manually populate `switched_from_protocol_version`, while the consensus-parameter tests check the update separately, so neither establishes that the actual upgrade populates and clears the marker correctly or that the first post-draw commit verifies. Add a lifecycle strategy starting at protocol 14 with signing, commit verification, and persistence enabled. Cross the upgrade, verify that the switching block retains consensus-version-1 commit semantics and refuses random requests unpaid without reservations, then verify that the following block settles randomness and accepts a commit binding the final app hash and state ID. Exercise that committed root in the next proposal and after reload. Actual beacon recovery and BLS authentication should remain tested in Tenderdash.
…activation Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against d3450ee and the relevant upstream source. Two blockers remain: deferred randomness processing is not fully prepaid, and the new ABCI dependency rejects the configured Tenderdash runtime; signed activation-boundary coverage is also missing. Four prior findings are fixed, all currently listed CI checks pass, and no builds or tests were run during this static review.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes consensus execution in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs, applying beacon-dependent state changes and validating the final app hash signed by commits. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate additional processing after transition fees have been calculated, but the native draw and settlement paths discard their operation costs. The existing prepayment does charge processing and storage for prospective index insertions; it does not cover the complete deferred operation. In particular, document creation invokes that prepayment only for indexed random properties, and `prepay_drawn_index_entries_v0` returns a default fee when no affected index exists. An unindexed random document still incurs a contract/document lookup, decoding, per-property hashing, and another document-update batch. Token reservation execution similarly pays for its initial writes, while settlement subsequently reads and removes escrow, updates balances or supply, and optionally rewrites history without charging those additional operations. This PR makes that previously inactive work reachable in block execution, outside the proposer's transition-processing time check. Extend the originating transition's deterministic prepayment in Drive to cover the deferred processing in both estimation and execution, including unindexed documents and tokens without history. Preserve the existing index prepayment rather than charging it twice, and do not introduce fee-dependent mutations after prevotes.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
The dependency change breaks the default live startup handshake independently of protocol-15 activation. The pinned binding generates its protocol from Tenderdash commit `c0e8c7f`, whose `ABCISemVer` is `1.5.0`; its `check_version` requires `^1.5` and rejects `1.4.0`. Drive's `info` handler calls that check unconditionally. Meanwhile, `packages/dashmate/configs/defaults/getBaseConfigFactory.js:382` still selects `dashpay/tenderdash:1.8`, which advertises ABCI 1.4. Thus the configured node fails with `AbciVersionMismatch` even when running an earlier platform protocol. The linked Tenderdash and Rust-binding PRs track the required upstream capability, but leaving the image update until release does not resolve this newly introduced startup regression. Coordinate the Rust pin and Dashmate runtime configuration with an available Tenderdash build containing the compatible ABCI and consensus changes.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:146-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies initialize directly at the latest protocol version, set `upgrading_info: None`, and use `default_minimal_verifications()`, which disables block signing and commit-signature verification. The gate tests manually install the upgrade marker, and the finalization fixtures also bypass signature verification. Consequently, the new coverage does not exercise the actual 14-to-15 switch together with authentication of the post-draw state ID. Add a lifecycle scenario starting at protocol 14 with commit signing and verification enabled: execute the switching block without a beacon, verify that random requests are refused unpaid there, then settle random requests in the following block and finalize a commit binding the final app hash and state ID. Check that the next proposal sees the committed final root. This protects the interaction between next-height consensus-parameter activation, clearing the transient upgrade marker, and the changed commit-signing preimage.
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.
- Cap the random requests a block can collect — The collector has no dedicated cap, but the finding establishes neither an unbounded request source nor a concrete failure requiring an additional consensus limit beyond existing block bounds. Its demonstrated concern—deferred processing outside the proposer time check without complete marginal pricing—is covered by the retained prepayment blocker. A separate cap or mandatory tracking issue is speculative hardening, not an independently established defect.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
…activation Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of head 5141f2b confirms two blockers: incomplete processing-fee prepayment for activated draws and an ABCI dependency incompatible with the configured default Tenderdash runtime. Four prior findings are fixed; signed protocol-upgrade coverage remains missing. Current CI reports passing code checks and a pending review check, but provides no completed Rust-suite result; no builds or tests were run locally.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate change directly modifies consensus execution in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs, applying beacon-driven state mutations and validating the final app hash across voting, replay, and upgrade paths. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate additional processing that the originating transitions do not fully prepay. Document insertion invokes `prepay_random_document_index_entries` only when a random property participates in an index, and `prepay_drawn_index_entries_v0` returns zero when no index is selected. An unindexed random document still requires contract/document retrieval, deserialization, a SHA-256 draw for each random property, and another applied database batch. Token reservations likewise charge their initial operations, while the optional history prepayment estimates amount-index insertions rather than the later escrow lookup/removal and balance/supply updates; history-disabled tokens bypass that prepayment entirely. The draw and settlement implementations discard the later operation costs through temporary accumulators. Fixed-size replacement avoids additional persistent bytes, not the processing cost of a second execution. This PR makes that previously inactive work reachable on every validator. Add deterministic upfront processing prepayment in Drive's owning estimation/execution paths, with new rates in the named versioned fee schedule, and cover unindexed document draws and history-disabled token settlements explicitly.
In `packages/rs-drive-abci/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This pin changes startup compatibility before any protocol-15 gate applies. The pinned dependency generates its protobufs from Tenderdash `c0e8c7f`, whose `version/version.go` declares ABCI 1.5.0; its `check_version` consequently requires `^1.5`. Drive's live `info` handler calls that check unconditionally. Dashmate still selects `dashpay/tenderdash:1.8` in `getBaseConfigFactory.js:382`, while Tenderdash v1.8.0 and the checked-out default branch declare ABCI 1.4.0. The configured pairing therefore fails its initial Info handshake even on a pre-PV15 chain. The release-dependency links correctly identify the upstream work, but those PRs remain open and the default runtime configuration has not changed. The replay adapter's version substitution fixes offline logs only. Coordinate an available compatible Tenderdash artifact, the default image, and the applicable configuration migration with this pin; do not weaken the handshake check to conceal the mismatch.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-152: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start at the latest protocol version, set `upgrading_info: None`, and use `default_minimal_verifications()`, which disables block signing, commit-signature verification, and platform-state persistence. They establish drawing and settlement, but not the new activation boundary: the 14-to-15 switching block must withhold random requests while returning consensus version 2 for the following height, whose commit must then bind the post-draw root. The activation-gate test manually sets the switching marker, and the finalize fallback tests disable commit-signature verification, so those tests do not establish that the lifecycle and signing paths compose correctly. Add a multi-block upgrade regression with signing and commit verification enabled, checking unpaid rejection in the switching block, document/token settlement in the following block, and continued execution against its committed final root. Enable persistence and reopen storage to cover restoration of that root as well.
…activation PV15 notes follow the base: random document properties 28, random token amounts 29, same-block randomness 30. The ApplyBlockRandomness handler's tests drop the removed previous_fee_versions argument. Co-Authored-By: Claude Opus 5.5 <[email protected]>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against e0fc568 and the relevant upstream sources. Two blockers remain: the default Tenderdash pairing fails the new ABCI handshake, and activated randomness processing is not fully prepaid; signed protocol-upgrade coverage is also missing. This was static-only verification: current CI has no failed checks, the review check is pending, and no dedicated Rust consensus-suite result is shown.
🔴 2 blocking | 🟡 1 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 9: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff directly changes consensus execution in rs-drive-abci/src/abci/handler/apply_block_randomness.rs and finalize_block.rs, applying beacon-driven state mutations and validating the final app hash across voting, replay, and upgrade paths. - 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 quota lookup failed),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 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— ffi-engineer (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— tenderdash-consensus-security (completed, effort xhigh); 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/Cargo.toml`:
- [BLOCKING] packages/rs-drive-abci/Cargo.toml:54-56: Coordinate the ABCI 1.5 pin with a compatible Tenderdash runtime
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657726)
This pin raises the live handshake requirement before protocol 15 activates. The pinned adapter generates its protocol from Tenderdash c0e8c7f, whose version/version.go declares ABCI 1.5.0; check_version requires ^1.5, and Drive's Info handler invokes it unconditionally. Dashmate still defaults to dashpay/tenderdash:1.8 in configs/defaults/getBaseConfigFactory.js:382, while the current Tenderdash default declares ABCI 1.4.0. The adapter's compatibility tests and this PR's replay regression explicitly confirm that a live 1.4 handshake is rejected. Consequently, the default engine/application pairing cannot start, including on chains below protocol 15.
The linked upstream PRs track the required release, but they do not resolve this newly introduced startup incompatibility. Coordinate an available compatible Tenderdash artifact and its Dashmate configuration/migration with this dependency change. Alternatively, implement safe pre-activation interoperability in the owning negotiation layer while preventing protocol 15 from running on an engine without consensus version 2.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/apply_random_beacon/v0/mod.rs:27-41: Prepay the processing work of the newly activated draws
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657708)
These calls activate a second execution pass whose processing costs are discarded rather than fully prepaid. Document insertion adds randomness prepayment only for affected indexes, and prepay_drawn_index_entries_v0 returns FeeResult::default() when none exists. Nevertheless, an unindexed random document is fetched, decoded, hashed per random property, and rewritten; the draw discards its read and batch-operation cost accumulators. Random token settlement likewise reads and removes escrow and updates balances or supply while discarding its costs. Tokens without history receive no history-index prepayment, and that prepayment would not cover the complete settlement anyway.
Paying for the placeholder or reservation's initial write funds that execution and storage allocation, not the subsequent processing pass. This introduces unpriced validator CPU and database work after proposal processing. Keep beacon application unbilled, but precharge a deterministic bound for the complete deferred work in Drive's originating create/reservation paths, using the appropriate versioned fee schedule and matching estimation/application logic. Cover unindexed random documents and transfers/mints without history so index prepayment cannot conceal the missing processing charge.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/random_beacon_tests.rs:145-151: Exercise the protocol upgrade with signed commits
(existing thread: https://github.com/dashpay/platform/pull/5753#discussion_r4464657739)
Both randomness strategies start at the latest protocol version, set upgrading_info to None, and use default_minimal_verifications(), which disables block signing, commit-signature verification, and platform-state persistence. Their transition-proof checks therefore do not verify a signed commit over the changed post-draw root. The activation-gate tests manually set switched_from_protocol_version, while the separate upgrade/restart coverage uses minimal verification without random requests.
Add a lifecycle strategy crossing the actual 14-to-15 upgrade with signing, commit verification, and persistence enabled. Assert unpaid refusal of random requests in the switching block, successful drawing and token settlement at the following height, a commit signing the final root, and consistent state after reload. This connects the activation marker, consensus-parameter update, first beacon, and finalization instead of assuming their isolated tests compose correctly.
Basic explanation
What this does: Switches same-block randomness on at protocol version 15. From the block after the upgrade, Tenderdash runs consensus version 2. It recovers each block's random beacon from the prevotes and hands it to Drive through the new
ApplyBlockRandomnessABCI method. Drive then draws the block'ssys.random.uniformdocument properties (#5749) and settles its random token transfers and mints (#5750). It returns the app hash after those draws, and the precommits and the commit sign that hash.Before, with #5749 and #5750 alone, every protocol version refuses
sys.random.uniformand version 1 token transfers and mints, and nothing calls the draw:After, at protocol version 15:
A node that did not vote (block sync, replay, a full node) gets the same call from Tenderdash with the commit's beacon before
FinalizeBlock. If Tenderdash skips the call,FinalizeBlockapplies the commit's beacon itself.Value: Games and other fair apps can draw a card's rarity or a token payout in the same block as the transition that asks for it. Nobody can predict or pick the value, and the player can prove it from the proof of their own transition.
Risks:
RandomBeaconUnavailableError, 10604), and the next block accepts it.A_final) and fix(statesync): restore LastAppHash of the snapshot height, not the next one tenderdash#1584, plus feat!: add the ApplyBlockRandomness application method rs-tenderdash-abci#226. Until those are released,tenderdash-abciandtenderdash-protoare pinned to feat/js dash sdk logger #226's head commite80c8a7, and the pin must move to the release tag before 5.1 ships. Dashmate's Tenderdash image moves with that release.Stacked on #5750 (random token amounts), which is stacked on #5749 (random document properties).
Issue being fixed or feature implemented
Refs #4682.
#5749 and #5750 define the random values and their draw, but no block execution draws them. This PR wires the draw into block execution, activates it at protocol version 15, and turns registration of random values on there. Unlike #4682's request-then-later-lookup model, the value is fixed when the block that asked for it commits.
What was done?
abci
ApplyBlockRandomnesshandler (abci/handler/apply_block_randomness.rs), served by the consensus and full applications. It checks that the request names the block this node processed in that round, then runsapply_random_beaconin the block's transaction and returns the root after the draws. A second request for the same round and beacon gets the same hash without a second draw, and another beacon for that round is refused. Before protocol version 15 it draws nothing and returns the block's own hash.FinalizeBlockapplies the commit's beacon when this node has not applied it yet. It refuses a commit whose app hash orblock_id.state_iddiffers from the state after the draws, and recordsA_finalas the block's app hash (ExtendedBlockInfo.app_hash), so the disk-root checks of the next proposal hold.block execution
process_raw_state_transitionscollects the random requests of the block's accepted transitions (BlockRandomRequests: document draws and token settlements), and the block execution context carries them toapply_random_beacon.apply_random_beacon(DriveAbciBlockEndMethodVersions.apply_random_beacon,Some(0)from protocol version 15) callsDrive::draw_random_document_propertiesandDrive::settle_random_token_amounts. Neither is billed: the transitions prepaid them.batch/transformer/v0/random_beacon_gate.rs): refuses a random document create, transfer or mint in the block that switches to protocol version 15.PlatformState.switched_from_protocol_versionis in memory only and never saved.activation at protocol version 15
ConsensusVersions.tenderdash_consensus_version2.consensus_params_updatesetsVersionParams.consensus_versionfrom it, and an emergency update from 15 no longer lowers it.CONTRACT_VERSIONS_V7:random_generated_fromSome(0), sosys.random.uniformregisters.STATE_TRANSITION_SERIALIZATION_VERSIONS_V4(protocol version 15 only):random_token_amountSome(0), so version 1 token transfers and mints decode.tenderdash-abciandtenderdash-protopinned to feat!: add the ApplyBlockRandomness application method rs-tenderdash-abci#226 (e80c8a7, ABCI 1.5.0).test harness
mimicandreplaycallApplyBlockRandomnessafter the vote extensions and before signing the commit, and their app-hash checks expectA_final. Replay sendsInfowith this build's ABCI version, so a log recorded under ABCI 1.4 still replays.process_raw_state_transitionsat the latest version:batch/tests/document/generated_from_random.rsandbatch/tests/token/random_amount.random_beacon_tests: random documents and token amounts block after block, through prepare, process, ApplyBlockRandomness, commit and finalize, with every drawn value checked against its range. The strategy gainsrandom_token_minimum_amount, and its proof check accepts a settled history amount inside the transition's range.How Has This Been Tested?
New tests:
apply_block_randomness: draws and returns the root after the draws (pinned draw of 26 for the fixture beacon); answers the same round again without a second draw or a second settlement of a random transfer; refuses a block this node did not process in the round; draws nothing before protocol version 15.finalize_block: applies a commit beacon this node did not apply, drawing its card and settling its random transfer once; refuses a commit signing another app hash; refuses a block id carrying another state id.replay::runner: replays anInforequest recorded under ABCI 1.4.consensus_params_update: raises Tenderdash to consensus version 2 on the switch to 15; an emergency update from 15 does not lower it.random_beacon_gate: withholds the beacon only from the block switching to protocol version 15.should_draw_every_random_property_from_its_block_beaconandshould_settle_every_random_token_amount_from_its_block_beacon(the latter checks balances, supply and escrow against the settled amounts).Runs (targeted; CI runs the suites):
cargo test -p dpp --all-features --lib -- generated_from_random random_token version_1_token codes basic_error history_document_id: 31 passedcargo test -p drive --all-features --lib -- draw_random_document_properties document_create_transition_action::v0::transformer settle_random_token_amounts random_amount_tests random_beacon: 24 passedcargo test -p drive-abci --lib -- apply_block_randomness finalize_block consensus_params_update random_beacon_gate batch::tests::document::generated_from_random batch::tests::token::random_amount nullifier_batch_guard: 86 passedcargo test -p drive-abci --test strategy_tests -- random_beacon_tests: 1 passedcargo check --all-features --all-targetson drive-abci, drive-proof-verifier and dapi-grpc against the re-pinned protos;cargo clippy --all-features --all-targets -- -D warningson dpp, drive and drive-abci: cleanBreaking Changes
Consensus, at protocol version 15 only (unreleased): Tenderdash consensus version 2, the draw and settlement of random values in block execution, and registration of
sys.random.uniformand version 1 token transfers and mints. ABCI: newApplyBlockRandomnessmethod, and the commit's app hash is the one after the draws. New basic consensus errorRandomBeaconUnavailableError(10604).In-place changes to shipped generations
Each change below is inert before protocol version 15, which is unreleased:
run_block_proposalv0,finalize_block_proposalv0 andprocess_raw_state_transitionsv0: they carry the block's random requests and the applied randomness. Both stay empty unless the block's protocol version draws (apply_random_beaconisNonebefore 15), andExtendedBlockInfo.app_hashthen equals the block's own root as before.apply_random_beaconisNone.update_state_cachev0 clearsPlatformState.switched_from_protocol_versionon commit. That field is in memory only, and only the activation-block gate reads it.consensus_params_updatev2: is selected only from protocol version 15. The emergency update now keeps the version's Tenderdash consensus version instead of setting 1, and that version is 1 at every version that selects it before this change.BlockExecutionContextV0andPlatformStategain in-memory fields (random_requests,applied_randomness,switched_from_protocol_version). They are never serialized, and the saved state formats are unchanged.CONTRACT_VERSIONS_V7is selected only by protocol version 15, so it is amended in place.STATE_TRANSITION_SERIALIZATION_VERSIONS_V4is new, and protocol version 14 keeps V3.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code