fix(drive-abci)!: follow operator payout changes and repair operator keys at protocol 15

book/src/architecture/component-pipeline.md

@@ -397,7 +397,7 @@ relative to `core_based_updates/`):
 | Read | Asked of Core | Code | Kept in |
 |---|---|---|---|
 | Core fork activations, from protocol version 15 | The coinbase payload version of the block at `c`, until the fork is recorded | `update_core_info/v1/` | GroveDB `Misc`; see [Core Fork Activations](core-fork-activations.md) |
-| Masternode list | `protx listdiff` from the previous block's `c` to `c`; also the coinbase payload version at `c` when a reported operator key may only be re-encoded | `update_masternode_list/` | Platform state, and masternode identities in GroveDB |
+| Masternode list | `protx listdiff` from the previous block's `c` to `c`; up to protocol version 14 also the coinbase payload version at `c` when a reported operator key may only be re-encoded (from 15 the Core fork record answers); from protocol version 15 also `protx info` at `c` for a masternode whose operator key changed without a printed payout | `update_masternode_list/` | Platform state, and masternode identities in GroveDB |
 | Quorums | `quorum listextended` at `c`, `quorum info` for each new quorum | `update_quorum_info/` | Platform state: the validator sets and the chain lock and instant lock quorums |
 
 Other steps read Core at `c` or below too. The paths in the first two rows are
@@ -409,6 +409,17 @@ under `packages/rs-drive-abci/src/execution/platform_events/withdrawals/`:
 | 8 | The statuses of the asset unlocks Platform broadcast (the Core transactions that pay out withdrawals) | `getassetunlockstatuses` at `c`, in blocks that move `c` | `update_broadcasted_withdrawal_statuses/` | The withdrawal documents |
 | 11 | Chain asset lock proofs (Dash locked on Core to fund an identity, proven by the chain-locked height that includes it) | The asset lock transaction, which must be mined at or below the proof's core height, itself no higher than Platform's last committed core height | `packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/` | What the state transition writes |
 
+The first block of protocol version 15 reads Core once more, before step 1, in
+its protocol change events (`transition_to_version_15`, under
+`packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/`): Core's
+full masternode list, `protx listdiff` from the first block to the previous
+block's `c`, to reconcile the operator payouts Platform holds, and the operator
+identities' TRANSFER keys, with Core's (`reconcile_operator_payouts_with_core/`
+under `core_based_updates/`). That height is chain locked and an earlier block
+already read Core there, so every validator's Core holds it. Unlike the reads
+above, it runs whether or not the block moves `c`: a node whose Core cannot
+answer cannot execute that one block.
+
 ### When Core fails
 
 Core RPC calls retry for about 11 seconds on connection errors and on the

book/src/architecture/core-fork-activations.md

@@ -151,10 +151,15 @@ and `perform_events_on_first_block_of_protocol_change`, run before
 `update_core_info`, so they cannot rely on the record for a fork that the same
 upgrade starts tracking.
 
-Closing the window would mean asking Core in the first block of the new protocol
-version whatever its `c`. That would put Core on the upgrade block's critical
-path: a node whose Core failed could not execute the upgrade block, and the
-upgrade would wait for every Core that was down.
+Closing the window would mean asking Core about the fork in the first block of
+the new protocol version whatever its `c`. That would put Core on the upgrade
+block's critical path: a node whose Core failed could not execute the upgrade
+block, and the upgrade would wait for every Core that was down. The upgrade to
+protocol version 15 pays that cost once for another reason: it reconciles the
+operator payouts Platform holds with Core's full masternode list (see
+[Syncing with Dash Core](component-pipeline.md#syncing-with-dash-core)), which
+nothing short of that list can do. The fork record does not need it: its window
+is short, and every consumer must be safe in it anyway.
 
 ## V24's signal
 
@@ -199,11 +204,13 @@ it failed, which is safe only for version 1. Pooling runs after
 records V24 is already version 2. In the window before the record, withdrawals
 stay at version 1, which Core always accepts. Pooling never asks Core about V24.
 
-One piece of code still asks Core about V24 directly: the masternode list
-update's `drop_unchanged_operator_keys`, which keeps a masternode's operator key
-when Core re-encodes it after V24. It runs under every protocol version, and
-from protocol version 15 it runs after the record is updated, in blocks that
-move `c`, so it gets the same answer as the record.
+The masternode list update keeps a masternode's operator key when Core
+re-encodes it after V24. From protocol version 15, `update_masternode_list` 2
+reads V24 from the record. It runs inside `update_core_info` v1, after the
+record step at the same `c` and only in blocks that move `c`, so `None` there
+means Core reported V24 inactive at this very `c`. `update_masternode_list` 0
+and 1, up to protocol version 14, still ask Core directly
+(`drop_unchanged_operator_keys`), as the blocks they executed did.
 
 ## Adding a fork
 

packages/dashmate/configs/defaults/getBaseConfigFactory.js

@@ -16,7 +16,7 @@
  */
 export default function getBaseConfigFactory() {
   const prereleaseTag = semver.prerelease(version) === null ? '' : `-${semver.prerelease(version)[0]}`;
   const dockerImageVersion = `${semver.major(version)}${prereleaseTag}`;
 
   /**
    * @typedef {function} getBaseConfig
@@ -94,7 +94,7 @@
                 'getbestchainlock', 'getblockchaininfo', 'getrawtransaction', 'submitchainlock',
                 'verifychainlock', 'protxlistdiff', 'quorumlistextended', 'quoruminfo',
                 'getassetunlockstatuses', 'sendrawtransaction', 'mnsyncstatus', 'getblockheader', 'getblockhash',
-                'getspecialtxes', 'getdeploymentinfo',
+                'getspecialtxes', 'getdeploymentinfo', 'protxinfo',
               ],
               lowPriority: false,
             },

packages/dashmate/configs/getConfigFileMigrationsFactory.js

@@ -1868,6 +1868,22 @@ export default function getConfigFileMigrationsFactory(homeDir, defaultConfigs)
 
         return configFile;
       },
+      '5.1.0-beta.1': (configFile) => {
+        Object.entries(configFile.configs)
+          .forEach(([, options]) => {
+            // From protocol version 15 Drive reads, with protx info, the operator payout of a
+            // masternode whose operator key changed. Core refuses the method to the consensus
+            // user until its whitelist names it, and loads the whitelist only when Core itself
+            // restarts. Added to the operator's list rather than replacing it; a user without a
+            // whitelist may already call every method.
+            const whitelist = options.core?.rpc?.users?.drive_consensus?.whitelist;
+            if (Array.isArray(whitelist) && !whitelist.includes('protxinfo')) {
+              whitelist.push('protxinfo');
+            }
+          });
+
+        return configFile;
+      },
     };
   }
 

packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js

@@ -51,6 +51,10 @@ describe('migrateConfigFileFactory', () => {
         whitelist.splice(whitelist.indexOf(method), 1);
       };
 
+      if (semver.lt(release, '5.1.0-beta.1')) {
+        removeFromWhitelist('protxinfo');
+      }
+
       if (semver.lt(release, '5.0.0-rc.2')) {
         removeFromWhitelist('getdeploymentinfo');
       }
@@ -644,6 +648,40 @@ describe('migrateConfigFileFactory', () => {
     }
   });
 
+  it('should let Drive call protx info on Core from a 5.0.3 config', async () => {
+    // From protocol version 15 Drive reads, with protx info, the operator payout of a
+    // masternode whose operator key changed, and Core answers a method missing from the
+    // consensus user's whitelist with HTTP 403.
+    const fromVersion = '5.0.3';
+    const { version } = JSON.parse(fs.readFileSync(path.join(PACKAGE_ROOT_DIR, 'package.json'), 'utf8'));
+
+    const configFileData = createConfigFile().toObject();
+    configFileData.configFormatVersion = fromVersion;
+    for (const options of Object.values(configFileData.configs)) {
+      const { whitelist } = options.core.rpc.users.drive_consensus;
+      expect(whitelist).to.include('protxinfo');
+      whitelist.splice(whitelist.indexOf('protxinfo'), 1);
+    }
+    configFileData.configs.testnet.core.rpc.users.drive_consensus.whitelist.push('getblockcount');
+    configFileData.configs.mainnet.core.rpc.users.drive_consensus.whitelist = null;
+
+    const migrated = migrateConfigFile(configFileData, fromVersion, version);
+
+    expect(migrated.configs.base.core.rpc.users.drive_consensus.whitelist)
+      .to.include('protxinfo');
+    expect(migrated.configs.local.core.rpc.users.drive_consensus.whitelist)
+      .to.include('protxinfo');
+    // A method the operator allowed stays allowed.
+    expect(migrated.configs.testnet.core.rpc.users.drive_consensus.whitelist)
+      .to.include.members(['protxinfo', 'getblockcount']);
+    // A user without a whitelist may already call every method.
+    expect(migrated.configs.mainnet.core.rpc.users.drive_consensus.whitelist).to.equal(null);
+
+    for (const [name, options] of Object.entries(migrated.configs)) {
+      expect(() => new Config(name, options), `migrated ${name} config does not load`).to.not.throw();
+    }
+  });
+
   it('should keep an operator image that predates the 4.0.0 re-pin', async () => {
     // Every config older than 4.0.0 crosses the unconditional re-pin in that
     // migration, so it is the first place operator intent can be respected. It

packages/rs-drive-abci/src/core/wait_for_core_to_sync/v0/mod.rs

@@ -1,14 +1,16 @@
 use tenderdash_abci::CancellationToken;
 
 use crate::error::Error;
-use crate::rpc::core::CoreRPCLike;
+use crate::rpc::core::{is_http_forbidden, CoreRPCLike};
+use dpp::dashcore::ProTxHash;
 use std::fmt::Debug;
 use std::time::Duration;
 
 const CORE_SYNC_STATUS_CHECK_TIMEOUT: Duration = Duration::from_secs(5);
 
 /// Blocks execution until Core is synced, then checks that Core answers the credit pool balance
-/// read block execution needs from protocol version 14.
+/// read block execution needs from protocol version 14, and does not refuse the `protx info`
+/// read it needs from protocol version 15.
 /// This isn't in consensus, however we still version it just in case we will upgrade it on a
 /// version
 pub fn wait_for_core_to_sync_v0<C: CoreRPCLike + Debug>(
@@ -61,5 +63,99 @@ pub fn wait_for_core_to_sync_v0<C: CoreRPCLike + Debug>(
         return Err(error.into());
     }
 
+    // From protocol version 15 a block reads, with `protx info`, the operator payout of a
+    // masternode whose operator key changed, for the same reason refused until Core restarts
+    // with a whitelist that names it. Asked for a masternode Core does not list, an allowed
+    // `protx info` answers that it has none, and a refused one answers HTTP 403.
+    if let Err(error) =
+        core_rpc.get_masternode_state(&ProTxHash::all_zeros(), chain_lock.block_height)
+    {
+        if is_http_forbidden(&error) {
+            tracing::error!(
+                ?error,
+                "core refuses protx info; restart core so it loads the rpc whitelist of drive's consensus user"
+            );
+            return Err(error.into());
+        }
+        tracing::debug!(
+            ?error,
+            "core answers protx info for a masternode it does not list"
+        );
+    }
+
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::wait_for_core_to_sync_v0;
+    use crate::error::Error;
+    use crate::rpc::core::MockCoreRPCLike;
+    use dpp::dashcore::bls_sig_utils::BLSSignature;
+    use dpp::dashcore::hashes::Hash;
+    use dpp::dashcore::{BlockHash, ChainLock};
+    use dpp::dashcore_rpc::dashcore_rpc_json::{MnSyncAssetName, MnSyncStatus};
+    use dpp::dashcore_rpc::jsonrpc::error::{Error as JsonRpcError, RpcError};
+    use dpp::dashcore_rpc::jsonrpc::simple_http::Error as HttpError;
+    use dpp::dashcore_rpc::Error as CoreRpcError;
+    use tenderdash_abci::CancellationToken;
+
+    /// A synced Core that reads the credit pool balance, and answers `protx info` with
+    /// `masternode_state`.
+    fn synced_core(
+        masternode_state: impl Fn() -> CoreRpcError + Send + 'static,
+    ) -> MockCoreRPCLike {
+        let mut core = MockCoreRPCLike::new();
+        core.expect_get_best_chain_lock().returning(|| {
+            Ok(ChainLock {
+                block_height: 100,
+                block_hash: BlockHash::all_zeros(),
+                signature: BLSSignature::from([0; 96]),
+            })
+        });
+        core.expect_masternode_sync_status().returning(|| {
+            Ok(MnSyncStatus {
+                asset_id: 999,
+                asset_name: MnSyncAssetName::Finished,
+                asset_start_time: 0,
+                attempt: 0,
+                is_blockchain_synced: true,
+                is_synced: true,
+            })
+        });
+        core.expect_get_credit_pool_balance().returning(|_| Ok(0));
+        core.expect_get_masternode_state()
+            .returning(move |_, _| Err(masternode_state()));
+        core
+    }
+
+    /// Core answers `protx info` for a masternode it does not list with an error; drive-abci
+    /// starts.
+    #[test]
+    fn should_start_when_core_answers_protx_info() {
+        let core = synced_core(|| {
+            CoreRpcError::JsonRpc(JsonRpcError::Rpc(RpcError {
+                code: -8,
+                message: "not found".to_string(),
+                data: None,
+            }))
+        });
+        assert!(wait_for_core_to_sync_v0(&core, CancellationToken::new()).is_ok());
+    }
+
+    /// A Core that has not loaded a whitelist naming `protx info` refuses it with HTTP 403, and
+    /// drive-abci does not start, instead of failing the first protocol version 15 block with
+    /// an operator key change.
+    #[test]
+    fn should_not_start_when_core_refuses_protx_info() {
+        let core = synced_core(|| {
+            CoreRpcError::JsonRpc(JsonRpcError::Transport(Box::new(HttpError::HttpErrorCode(
+                403,
+            ))))
+        });
+        assert!(matches!(
+            wait_for_core_to_sync_v0(&core, CancellationToken::new()),
+            Err(Error::CoreRpc(_))
+        ));
+    }
+}

packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs

@@ -158,6 +158,7 @@ Your software version: {}, latest supported protocol version: {}."#,
                 // This is for events like adding stuff to the root tree, or making structural changes/fixes
                 self.perform_events_on_first_block_of_protocol_change(
                     platform_state,
+                    &mut block_platform_state,
                     &block_info,
                     transaction,
                     old_protocol_version,

packages/rs-drive-abci/src/execution/platform_events/core_based_updates/mod.rs

@@ -1,3 +1,4 @@
+mod reconcile_operator_payouts_with_core;
 mod update_core_info;
 mod update_masternode_identities;
 mod update_masternode_list;